[PATCH] arm64: mm: Fix "rodata=on" when CONFIG_RODATA_FULL_DEFAULT_ENABLED=y

[Ard Biesheuvel]

On Fri, 17 Nov 2023 at 08:14, Will Deacon <will at kernel.org> wrote:
>
> When CONFIG_RODATA_FULL_DEFAULT_ENABLED=y, passing "rodata=on" on the
> kernel command-line (rather than "rodata=full") should turn off the
> "full" behaviour, leaving writable linear aliases of read-only kernel
> memory. Unfortunately, the option has no effect in this situation and
> the only way to disable the "rodata=full" behaviour is to disable rodata
> protection entirely by passing "rodata=off".
>
> Fix this by parsing the "on" and "off" options in the arch code,
> additionally enforcing that 'rodata_full' cannot be set without also
> setting 'rodata_enabled', allowing us to simplify a couple of checks
> in the process.
>

I think this got broken when this code was refactored to use the
generic parsing, which uses strtobool() and so it will match
rodata=0/1, yes, no etc, and only 'full' is handled as a special case.

Not sure whether this matters, and I'd much prefer supporting only
'on', 'off' and 'full' because we'll need to parse rodata= early too
(for my big head.S / LPA2 refactor).



> Cc: Ard Biesheuvel <ardb at kernel.org>
> Cc: Catalin Marinas <catalin.marinas at arm.com>
> Cc: Mark Rutland <mark.rutland at arm.com>
> Signed-off-by: Will Deacon <will at kernel.org>
> ---
>  arch/arm64/include/asm/setup.h | 17 +++++++++++++++--
>  arch/arm64/mm/pageattr.c       |  7 +++----
>  2 files changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm64/include/asm/setup.h b/arch/arm64/include/asm/setup.h
> index f4af547ef54c..2e4d7da74fb8 100644
> --- a/arch/arm64/include/asm/setup.h
> +++ b/arch/arm64/include/asm/setup.h
> @@ -21,9 +21,22 @@ static inline bool arch_parse_debug_rodata(char *arg)
>         extern bool rodata_enabled;
>         extern bool rodata_full;
>
> -       if (arg && !strcmp(arg, "full")) {
> +       if (!arg)
> +               return false;
> +
> +       if (!strcmp(arg, "full")) {
> +               rodata_enabled = rodata_full = true;
> +               return true;
> +       }
> +
> +       if (!strcmp(arg, "off")) {
> +               rodata_enabled = rodata_full = false;
> +               return true;
> +       }
> +
> +       if (!strcmp(arg, "on")) {
>                 rodata_enabled = true;
> -               rodata_full = true;
> +               rodata_full = false;
>                 return true;
>         }
>
> diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
> index 8e2017ba5f1b..924843f1f661 100644
> --- a/arch/arm64/mm/pageattr.c
> +++ b/arch/arm64/mm/pageattr.c
> @@ -29,8 +29,8 @@ bool can_set_direct_map(void)
>          *
>          * KFENCE pool requires page-granular mapping if initialized late.
>          */
> -       return (rodata_enabled && rodata_full) || debug_pagealloc_enabled() ||
> -               arm64_kfence_can_set_direct_map();
> +       return rodata_full || debug_pagealloc_enabled() ||
> +              arm64_kfence_can_set_direct_map();
>  }
>
>  static int change_page_range(pte_t *ptep, unsigned long addr, void *data)
> @@ -105,8 +105,7 @@ static int change_memory_common(unsigned long addr, int numpages,
>          * If we are manipulating read-only permissions, apply the same
>          * change to the linear mapping of the pages that back this VM area.
>          */
> -       if (rodata_enabled &&
> -           rodata_full && (pgprot_val(set_mask) == PTE_RDONLY ||
> +       if (rodata_full && (pgprot_val(set_mask) == PTE_RDONLY ||
>                             pgprot_val(clear_mask) == PTE_RDONLY)) {
>                 for (i = 0; i < area->nr_pages; i++) {
>                         __change_memory_common((u64)page_address(area->pages[i]),
> --
> 2.43.0.rc0.421.g78406f8d94-goog
>