linux-kernel - Re: [PATCH v3 1/2] arm64: refactor the rodata=xxx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d13f1879-7fbc-458d-8fd3-4340b51165fd@arm.com>
Date: Tue, 10 Dec 2024 12:47:32 +0530
From: Anshuman Khandual <anshuman.khandual@....com>
To: Huang Shijie <shijie@...amperecomputing.com>, catalin.marinas@....com,
 will@...nel.org, corbet@....net
Cc: patches@...erecomputing.com, cl@...ux.com, akpm@...ux-foundation.org,
 thuth@...hat.com, rostedt@...dmis.org, xiongwei.song@...driver.com,
 ardb@...nel.org, inux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v3 1/2] arm64: refactor the rodata=xxx

Hello Huang,

On 12/9/24 12:50, Huang Shijie wrote:
>>>From Documentation/admin-guide/kernel-parameters.txt:
> 	rodata=		[KNL,EARLY]
> 		on	Mark read-only kernel memory as read-only (default).
> 
> So "rodata=on" is the default.
> 
> But the current code does not follow the document, it makes "rodata=full"
> as the default.
> 
> This patch refactors the "rodata=xxx" :
>       1.) Make rodata=on behaviour be the original rodata=full.
>           (Drop the original rodata=full.)
>       2.) Introduce "rodata=noalias" which is the original "rodata=on"
>       3.) Add more comment for arch_parse_debug_rodata(),
>           update kernel-parameters.txt.
> 
> After this patch, the rodata=on will be the default.

Could this commit message be reworded/reformatted something like the
following instead ? Although feel free to improve this as required.

-----------------------------------------------------------------------
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@...amperecomputing.com>
> ---
>  .../admin-guide/kernel-parameters.txt         |  2 +-
>  arch/arm64/include/asm/setup.h                | 25 +++++++++++++++++--
>  2 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index a22b7e621007..51bce7b9d805 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -5901,7 +5901,7 @@
>  	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.
>  		        [arm64]
>  
>  	rockchip.usb_uart
> diff --git a/arch/arm64/include/asm/setup.h b/arch/arm64/include/asm/setup.h
> index ba269a7a3201..b3063326b79a 100644
> --- a/arch/arm64/include/asm/setup.h
> +++ b/arch/arm64/include/asm/setup.h
> @@ -13,6 +13,27 @@
>  extern phys_addr_t __fdt_pointer __initdata;
>  extern u64 __cacheline_aligned boot_args[4];
>  
> +/*
> + * rodata=on (default):
> + *    Apply read-only attributes of VM areas 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 of the same memory page.
> + *
> + *    This requires the linear region to be mapped down to pages,
> + *    which may adversely affect performance in some cases.
> + *
> + * rodata=off:
> + *    It provides us more block mappings and contiguous hits
> + *    to map the linear region which minimize the TLB footprint.
> + *    Leave read-only kernel memory writable for debugging.
> + *
> + * rodata=noalias:
> + *    It provides us more block mappings and contiguous hits
> + *    to map the linear region which minimize the TLB footprint.
> + *    And the linear aliases of pages belonging to read-only mappings
> + *    in vmalloc region are also marked as read-only.
> + */

Reformatted and cleaned up the above comment a bit but feel free to
improve it further.

/*
 * 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. Linear aliases of
 *    pages belonging to read-only mappings in vmalloc region are also
 *    marked as read-only.


>  static inline bool arch_parse_debug_rodata(char *arg)
>  {
>  	extern bool rodata_enabled;
> @@ -21,7 +42,7 @@ static inline bool arch_parse_debug_rodata(char *arg)
>  	if (!arg)
>  		return false;
>  
> -	if (!strcmp(arg, "full")) {
> +	if (!strcmp(arg, "on")) {
>  		rodata_enabled = rodata_full = true;
>  		return true;
>  	}
> @@ -31,7 +52,7 @@ static inline bool arch_parse_debug_rodata(char *arg)
>  		return true;
>  	}
>  
> -	if (!strcmp(arg, "on")) {
> +	if (!strcmp(arg, "noalias")) {
>  		rodata_enabled = true;
>  		rodata_full = false;
>  		return true;