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

[Shijie Huang <shijie@amperemail.onmicrosoft.com>]

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@os.amperecomputing.com> 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@os.amperecomputing.com>
>> ---
>> 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