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

[Shijie Huang]

Hi Ard,

On 2024/12/13 15:30, Ard Biesheuvel wrote:
> 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?
The following is the current status of "rodata=noalias" by checking the 
kernel page table in my machine:

   1) the kernel code keeps read-only in both the "vmalloc area" and 
the "linear area".

   2) But there is a read-only memory range which is read-only in 
"vmalloc area", but its linear alias is read-write in the "linear area".


Maybe the "security" is not a proper word.


> How do you envisage 'noalias' becoming a security feature? The point

for the case 2) above, if its linear alias is also mapped as read-only,

can we think it is safe as the original "rodata=full"?


> 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]

What's about change it to:

   diff --git a/Documentation/admin-guide/kernel-parameters.txt 
b/Documentation/admin-guide/kernel-parameters.txt
index a22b7e621007..3d4aef0d0811 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.
+                        It is less secure then "rodata=on".
                        [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."

No problem.


Thanks

Huang Shijie