Re: [PATCH v4 1/2 fix] arm64: refactor the rodata=xxx

[Ard Biesheuvel]

Hello Huang Shije,

On Fri, 13 Dec 2024 at 06:32, Huang Shijie
<shijie@xxxxxxxxxxxxxxxxxxxxxx> wrote:
>
> As per admin guide documentation, "rodata=on" should be the default on
> platforms. Documentation/admin-guide/kernel-parameters.txt describes
> these options as
>
>    rodata=         [KNL,EARLY]
>            on      Mark read-only kernel memory as read-only (default).
>            off     Leave read-only kernel memory writable for debugging.
>            full    Mark read-only kernel memory and aliases as read-only
>                    [arm64]
>
> But on arm64 platform, "rodata=full" is the default instead. This patch
> implements the following changes.
>
>  - Make "rodata=on" behaviour same as the original "rodata=full"
>  - Make "rodata=noalias" (new) behaviour same as the original "rodata=on"
>  - Drop the original "rodata=full"
>  - Add comment for arch_parse_debug_rodata()
>  - Update kernel-parameters.txt as required
>
> After this patch, the "rodata=on" will be the default on arm64 platform
> as well.
>
> Signed-off-by: Huang Shijie <shijie@xxxxxxxxxxxxxxxxxxxxxx>
> ---
> Add more descriptions for "noalias":
>   It is not a security feature yet.

Why did you add that?

How do you envisage 'noalias' becoming a security feature? The point
of 'full' rodata was to harden the read-only regions in the vmalloc
space against inadvertent modification via the writeable linear alias,
so 'noalias' is less secure than rodata=full, and should be documented
as such.

> ---
>  .../admin-guide/kernel-parameters.txt         |  3 ++-
>  arch/arm64/include/asm/setup.h                | 27 +++++++++++++++++--
>  2 files changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index a22b7e621007..f5db01eecbd3 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -5901,7 +5901,8 @@
>         rodata=         [KNL,EARLY]
>                 on      Mark read-only kernel memory as read-only (default).
>                 off     Leave read-only kernel memory writable for debugging.
> -               full    Mark read-only kernel memory and aliases as read-only
> +               noalias Use more block mappings,may have better performance.
> +                       But this is not a security feature.
>                         [arm64]
>
>         rockchip.usb_uart
> diff --git a/arch/arm64/include/asm/setup.h b/arch/arm64/include/asm/setup.h
> index ba269a7a3201..0ef57d19fc2a 100644
> --- a/arch/arm64/include/asm/setup.h
> +++ b/arch/arm64/include/asm/setup.h
> @@ -13,6 +13,29 @@
>  extern phys_addr_t __fdt_pointer __initdata;
>  extern u64 __cacheline_aligned boot_args[4];
>
> +/*
> + * rodata=on (default)
> + *
> + *    This applies read-only attributes to VM areas and to the linear
> + *    alias of the backing pages as well. This prevents code or read-
> + *    only data from being modified (inadvertently or intentionally),
> + *    via another mapping for the same memory page.
> + *
> + *    But this might cause linear map region to be mapped down to base
> + *    pages, which may adversely affect performance in some cases.
> + *
> + * rodata=off
> + *
> + *    This provides more block mappings and contiguous hints for linear
> + *    map region which would minimize TLB footprint. This also leaves
> + *    read-only kernel memory writable for debugging.
> + *
> + * rodata=noalias
> + *
> + *    This provides more block mappings and contiguous hints for linear
> + *    map region which would minimize TLB footprint. This is not a
> + *    security feature yet.

Better replace the last sentence with

"This leaves the linear alias of read-only mappings in the vmalloc
space writeable, making them susceptible to inadvertent modification
by software."