[PATCH] kaslr: x86: fixes log message nokaslr

[PATCH] kaslr: x86: fixes log message nokaslr

[Mahmoud Younes]

Unknown kernel command line parameters nokaslr message will be printed
to kernel log buffer if nokaslr option exists in boot command line.
nokaslr gets consumed earlier and this message becomes confusing.
impact is that user gets confused whether kaslr is enabled or not.

Signed-off-by: Mahmoud Younes <m.younesbadr@gmail.com>
---
 arch/x86/mm/kaslr.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/x86/mm/kaslr.c b/arch/x86/mm/kaslr.c
index 37db264866b6..a62cb0675e22 100644
--- a/arch/x86/mm/kaslr.c
+++ b/arch/x86/mm/kaslr.c
@@ -179,3 +179,9 @@ void __meminit init_trampoline_kaslr(void)
 			__pgd(_KERNPG_TABLE | __pa(pud_page_tramp));
 	}
 }
+
+static int __init parse_nokaslr(char *_)
+{
+	return 0;
+}
+early_param("nokaslr", parse_nokaslr);
-- 
2.34.1

Re: [PATCH] kaslr: x86: fixes log message nokaslr

[Borislav Petkov]

On Sun, Mar 31, 2024 at 10:05:46PM +0200, Mahmoud Younes wrote:
> Unknown kernel command line parameters nokaslr message will be printed
> to kernel log buffer if nokaslr option exists in boot command line.
> nokaslr gets consumed earlier and this message becomes confusing.
> impact is that user gets confused whether kaslr is enabled or not.

Well, my dmesg has here:

---
...
trampoline_32bit: 0x0000000000000000


KASLR disabled: 'nokaslr' on cmdline.


Decompressing Linux... Parsing ELF... No relocation needed... done.
...
---

so the notification for the user is there.

> Signed-off-by: Mahmoud Younes <m.younesbadr@gmail.com>
> ---
>  arch/x86/mm/kaslr.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/x86/mm/kaslr.c b/arch/x86/mm/kaslr.c
> index 37db264866b6..a62cb0675e22 100644
> --- a/arch/x86/mm/kaslr.c
> +++ b/arch/x86/mm/kaslr.c
> @@ -179,3 +179,9 @@ void __meminit init_trampoline_kaslr(void)
>  			__pgd(_KERNPG_TABLE | __pa(pud_page_tramp));
>  	}
>  }
> +
> +static int __init parse_nokaslr(char *_)
> +{
> +	return 0;
> +}
> +early_param("nokaslr", parse_nokaslr);

This piece of code without any comments explaining why it is there is
not less confusing to whoever stares at it.

I'd prefer if print_unknown_bootoptions() would filter out those options
which are parsed earlier and not warn about them instead of having such
dummy stubs.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] kaslr: x86: fixes log message nokaslr

[Mahmoud Younes]

Thanks a lot for the reply. here is my thought process:


On Sun, 31 Mar 2024 at 22:28, Borislav Petkov <bp@alien8.de> wrote:
>
> On Sun, Mar 31, 2024 at 10:05:46PM +0200, Mahmoud Younes wrote:
> > Unknown kernel command line parameters nokaslr message will be printed
> > to kernel log buffer if nokaslr option exists in boot command line.
> > nokaslr gets consumed earlier and this message becomes confusing.
> > impact is that user gets confused whether kaslr is enabled or not.
>
> Well, my dmesg has here:
>
> ---
> ...
> trampoline_32bit: 0x0000000000000000
>
>
> KASLR disabled: 'nokaslr' on cmdline.
>
>
> Decompressing Linux... Parsing ELF... No relocation needed... done.
> ...
> ---
>
> so the notification for the user is there.
>

I don't think this gets printed after executing dmesg. This gets
printed to console if earlyprintks are captured to the console and the
kernel is configured with printing low level debug info. that is not
the default behavior and requires manipulating the kernel
configuration and command line to get this message. Specifically,
enabling DEBUG_LL and EARLY_PRINTK and adding "earlyprintk=ttyS0" or
whatever console in use. the message is just not visible in dmesg even
though it would exist in kernel log buffer, just the console wouldn't
be initialized at that moment. moreover, this is not visible when
booting a real hardware.

> > Signed-off-by: Mahmoud Younes <m.younesbadr@gmail.com>
> > ---
> >  arch/x86/mm/kaslr.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/arch/x86/mm/kaslr.c b/arch/x86/mm/kaslr.c
> > index 37db264866b6..a62cb0675e22 100644
> > --- a/arch/x86/mm/kaslr.c
> > +++ b/arch/x86/mm/kaslr.c
> > @@ -179,3 +179,9 @@ void __meminit init_trampoline_kaslr(void)
> >                       __pgd(_KERNPG_TABLE | __pa(pud_page_tramp));
> >       }
> >  }
> > +
> > +static int __init parse_nokaslr(char *_)
> > +{
> > +     return 0;
> > +}
> > +early_param("nokaslr", parse_nokaslr);
>
> This piece of code without any comments explaining why it is there is
> not less confusing to whoever stares at it.
>
> I'd prefer if print_unknown_bootoptions() would filter out those options
> which are parsed earlier and not warn about them instead of having such
> dummy stubs.
>
as far as I understand, nokaslr is not treated as any type of param.
IOW semantically it is an early param, but it will not be defined
during runtime in any of the sections specified for kernel parameters.
I am thinking of three ways to handle this issue of filtering
parameters that are parsed earlier. First way, include them in a
section (.init.setup?, __param?, new_section? the first being more
logical). Thinking about it, nokaslr is semantically an early_param
(consumed early in the boot process -- probably way too early). Hence,
it's logical to add definition for a corresponding obs_kernel_param
object for nokaslr to .init.setup via the early_param macro. The issue
is, by the time all params in this section are handled, kaslr init
code will have been executed and nokaslr won't have an effect. The not
so elegant way to handle this is the empty stub submitted and here as
well https://lore.kernel.org/linux-arm-kernel/20230412043258.397455-1-quic_pkondeti@quicinc.com/T/.
The elegant way is to define nokaslr as early_param, write a handler
that sets a boolean flag whether to enable or disable kaslr, call
parse_early_param before setup_arch (I believe this is not possible
due to the dependency; the code in early param handlers is dependent
on setup_arch being executed first) and replace the code that checks
for nokaslr in the command line with just checking the flag.
second way, provide a static array of params known to be consumed
during early boot but are not known to the kernel which does not sound
elegant to me.
Third way, leave everything as is and ignore the issue.

Since I am new to the kernel code base, I would appreciate some
guidance on how to move forward. Thank you!

Best,
M. Younes

Re: [PATCH] kaslr: x86: fixes log message nokaslr

[Borislav Petkov]

On Mon, Apr 01, 2024 at 07:38:17AM +0200, Mahmoud Younes wrote:
> I don't think this gets printed after executing dmesg. This gets
> printed to console if earlyprintks are captured to the console and the
> kernel is configured with printing low level debug info. that is not
> the default behavior and requires manipulating the kernel
> configuration and command line to get this message. Specifically,
> enabling DEBUG_LL and EARLY_PRINTK and adding "earlyprintk=ttyS0" or
> whatever console in use. the message is just not visible in dmesg even
> though it would exist in kernel log buffer, just the console wouldn't
> be initialized at that moment.

If it is in the log buffer, it should come out at some point.

> this is not visible when booting a real hardware.

I don't think so - otherwise earlyprintk is broken.

> Since I am new to the kernel code base, I would appreciate some
> guidance on how to move forward. Thank you!

Since those very early params are a handful and need special treatment,
I'd prefer if they're handled explicitly as every arch does its own
thing.

So print_unknown_bootoptions() is perhaps not the best place as it is
arch-agnostic.

For this particular one:

void choose_random_location(unsigned long input,
                            unsigned long input_size,
                            unsigned long *output,
                            unsigned long output_size,
                            unsigned long *virt_addr)
{
        unsigned long random_addr, min_addr;

        if (cmdline_find_option_bool("nokaslr")) {
                warn("KASLR disabled: 'nokaslr' on cmdline.");
                return;
        }

        boot_params_ptr->hdr.loadflags |= KASLR_FLAG;

So your dummy stub could look at the loader flags and at least warn when
there's a mismatch.

And it should have a big fat comment explaining why this stub is there.

I guess that would be a good compromise between overengineering this for
no good reason and not doing anything at all and confusing users.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette