| Message ID | 20241213053049.7592-1-shijie@os.amperecomputing.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,1/2,fix] arm64: refactor the rodata=xxx | expand |
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? 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."
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
On Fri, 13 Dec 2024 at 09:23, Shijie Huang <shijie@amperemail.onmicrosoft.com> wrote: > > 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. > There is nothing wrong with the word 'security' as long as you are clear about the fact that rodata=noalias decreases it. > > > > > 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"? > No, it is not. Why would we bother with rodata=full (which is costly in terms of TLB pressure) if rodata=noalias is equally safe?
On 2024/12/13 16:26, Ard Biesheuvel wrote: >>> 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"? >> > No, it is not. Why would we bother with rodata=full (which is costly > in terms of TLB pressure) if rodata=noalias is equally safe? okay, thanks. I will wait for two days, and send out the new patch if there no other comment. Thanks Huang Shijie
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. + */ static inline bool arch_parse_debug_rodata(char *arg) { extern bool rodata_enabled; @@ -21,7 +44,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 +54,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;
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. --- .../admin-guide/kernel-parameters.txt | 3 ++- arch/arm64/include/asm/setup.h | 27 +++++++++++++++++-- 2 files changed, 27 insertions(+), 3 deletions(-)