Re: [PATCH 1/2] arm64: cpufeatures: Fix handling of CONFIG_CMDLINE for idreg overrides
From: Will Deacon
Date: Thu Feb 25 2021 - 09:05:14 EST
On Thu, Feb 25, 2021 at 01:53:56PM +0000, Marc Zyngier wrote:
>
On 2021年2月25日 12:59:20 +0000,
>
Will Deacon <will@xxxxxxxxxx> 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@xxxxxxxxxx>
>
> Fixes: 33200303553d ("arm64: cpufeature: Add an early command-line cpufeature override facility")
>
> Signed-off-by: Will Deacon <will@xxxxxxxxxx>
>
> ---
>
> 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@xxxxxxxxxx>
Cheers!
Will