linux-kernel - Re: [PATCH 1/2] arm64: cpufeatures: Fix handling of CONFIG

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 25 Feb 2021 14:04:19 +0000
From:   Will Deacon <will@...nel.org>
To:     Marc Zyngier <maz@...nel.org>
Cc:     linux-kernel@...r.kernel.org, Max Uvarov <muvarov@...il.com>,
        Rob Herring <robh@...nel.org>,
        Ard Biesheuvel <ardb@...nel.org>,
        Doug Anderson <dianders@...omium.org>,
        Tyler Hicks <tyhicks@...ux.microsoft.com>,
        Frank Rowand <frowand.list@...il.com>,
        Arnd Bergmann <arnd@...db.de>,
        Palmer Dabbelt <palmer@...belt.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Catalin Marinas <catalin.marinas@....com>,
        kernel-team@...roid.com, linux-arm-kernel@...ts.infradead.org,
        devicetree@...r.kernel.org
Subject: Re: [PATCH 1/2] arm64: cpufeatures: Fix handling of CONFIG_CMDLINE
 for idreg overrides

On Thu, Feb 25, 2021 at 01:53:56PM +0000, Marc Zyngier wrote:
> On Thu, 25 Feb 2021 12:59:20 +0000,
> Will Deacon <will@...nel.org> wrote:
> > 
> > The built-in kernel commandline (CONFIG_CMDLINE) can be configured in
> > three different ways:
> > 
> >   1. CMDLINE_FORCE: Use CONFIG_CMDLINE instead of any bootloader args
> >   2. CMDLINE_EXTEND: Append the bootloader args to CONFIG_CMDLINE
> >   3. CMDLINE_FROM_BOOTLOADER: Only use CONFIG_CMDLINE if there aren't
> >      any bootloader args.
> > 
> > The early cmdline parsing to detect idreg overrides gets (2) and (3)
> > slightly wrong: in the case of (2) the bootloader args are parsed first
> > and in the case of (3) the CMDLINE is always parsed.
> > 
> > Fix these issues by moving the bootargs parsing out into a helper
> > function and following the same logic as that used by the EFI stub.
> > 
> > Cc: Marc Zyngier <maz@...nel.org>
> > Fixes: 33200303553d ("arm64: cpufeature: Add an early command-line cpufeature override facility")
> > Signed-off-by: Will Deacon <will@...nel.org>
> > ---
> >  arch/arm64/kernel/idreg-override.c | 44 +++++++++++++++++-------------
> >  1 file changed, 25 insertions(+), 19 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/idreg-override.c b/arch/arm64/kernel/idreg-override.c
> > index dffb16682330..cc071712c6f9 100644
> > --- a/arch/arm64/kernel/idreg-override.c
> > +++ b/arch/arm64/kernel/idreg-override.c
> > @@ -163,33 +163,39 @@ static __init void __parse_cmdline(const char *cmdline, bool parse_aliases)
> >  	} while (1);
> >  }
> >  
> > -static __init void parse_cmdline(void)
> > +static __init const u8 *get_bootargs_cmdline(void)
> >  {
> > -	if (!IS_ENABLED(CONFIG_CMDLINE_FORCE)) {
> > -		const u8 *prop;
> > -		void *fdt;
> > -		int node;
> > +	const u8 *prop;
> > +	void *fdt;
> > +	int node;
> >  
> > -		fdt = get_early_fdt_ptr();
> > -		if (!fdt)
> > -			goto out;
> > +	fdt = get_early_fdt_ptr();
> > +	if (!fdt)
> > +		return NULL;
> >  
> > -		node = fdt_path_offset(fdt, "/chosen");
> > -		if (node < 0)
> > -			goto out;
> > +	node = fdt_path_offset(fdt, "/chosen");
> > +	if (node < 0)
> > +		return NULL;
> >  
> > -		prop = fdt_getprop(fdt, node, "bootargs", NULL);
> > -		if (!prop)
> > -			goto out;
> > +	prop = fdt_getprop(fdt, node, "bootargs", NULL);
> > +	if (!prop)
> > +		return NULL;
> >  
> > -		__parse_cmdline(prop, true);
> > +	return strlen(prop) ? prop : NULL;
> > +}
> >  
> > -		if (!IS_ENABLED(CONFIG_CMDLINE_EXTEND))
> > -			return;
> > +static __init void parse_cmdline(void)
> > +{
> > +	const u8 *prop = get_bootargs_cmdline();
> > +
> > +	if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) ||
> > +	    IS_ENABLED(CONFIG_CMDLINE_FORCE) ||
> > +	    !prop) {
> 
> The logic hurts, but I think I grok it now. The last term is actually
> a reduction of
> 
> 	(IS_ENABLED(CONFIG_CMDLINE_FROM_BOOTLOADER) && !prop)
> 
> and we know for sure that if none of the other two terms are true,
> then CMDLINE_FROM_BOOTLOADER *must* be enabled.

Yes, with one gotcha: when CONFIG_CMDLINE is "", I don't think any of the
CONFIG_CMDLINE_* are set, but the behaviour ends up being the same as
CMDLINE_FROM_BOOTLOADER.

> 
> > +		__parse_cmdline(CONFIG_CMDLINE, true);
> >  	}
> >  
> > -out:
> > -	__parse_cmdline(CONFIG_CMDLINE, true);
> > +	if (!IS_ENABLED(CONFIG_CMDLINE_FORCE) && prop)
> > +		__parse_cmdline(prop, true);
> >  }
> >  
> >  /* Keep checkers quiet */
> 
> I don't think we need to backport anything to stable for the nokaslr
> handling, do we?

No, I don't think so. There isn't a "kaslr" or "nonokaslr", so the ordering
doesn't matter afaict.

> Reviewed-by: Marc Zyngier <maz@...nel.org>

Cheers!

Will