Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 25 Feb 2021 13:53:56 +0000
Message-ID: <87zgzsz217.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Will Deacon <will@kernel.org>
Cc:     linux-kernel@vger.kernel.org, Max Uvarov <muvarov@gmail.com>,
        Rob Herring <robh@kernel.org>,
        Ard Biesheuvel <ardb@kernel.org>,
        Doug Anderson <dianders@chromium.org>,
        Tyler Hicks <tyhicks@linux.microsoft.com>,
        Frank Rowand <frowand.list@gmail.com>,
        Arnd Bergmann <arnd@arndb.de>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Catalin Marinas <catalin.marinas@arm.com>,
        kernel-team@android.com, linux-arm-kernel@lists.infradead.org,
        devicetree@vger.kernel.org
Subject: Re: [PATCH 1/2] arm64: cpufeatures: Fix handling of CONFIG_CMDLINE for idreg overrides
In-Reply-To: <20210225125921.13147-2-will@kernel.org>
References: <20210225125921.13147-1-will@kernel.org>
        <20210225125921.13147-2-will@kernel.org>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Thu, 25 Feb 2021 12:59:20 +0000,
Will Deacon <will@kernel.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@kernel.org>
> Fixes: 33200303553d ("arm64: cpufeature: Add an early command-line cpufeature override facility")
> Signed-off-by: Will Deacon <will@kernel.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.

> +		__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?

Otherwise,

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

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.