| Message ID | 20231117131422.29663-1-will@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: mm: Fix "rodata=on" when CONFIG_RODATA_FULL_DEFAULT_ENABLED=y | expand |
On Fri, Nov 17, 2023 at 01:14:22PM +0000, Will Deacon wrote: > When CONFIG_RODATA_FULL_DEFAULT_ENABLED=y, passing "rodata=on" on the > kernel command-line (rather than "rodata=full") should turn off the > "full" behaviour, leaving writable linear aliases of read-only kernel > memory. Unfortunately, the option has no effect in this situation and > the only way to disable the "rodata=full" behaviour is to disable rodata > protection entirely by passing "rodata=off". > > Fix this by parsing the "on" and "off" options in the arch code, > additionally enforcing that 'rodata_full' cannot be set without also > setting 'rodata_enabled', allowing us to simplify a couple of checks > in the process. > > Cc: Ard Biesheuvel <ardb@kernel.org> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Mark Rutland <mark.rutland@arm.com> > Signed-off-by: Will Deacon <will@kernel.org> Ouch. Looks correct to me, so... Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Thanks!
On Fri, 17 Nov 2023 at 08:14, Will Deacon <will@kernel.org> wrote: > > When CONFIG_RODATA_FULL_DEFAULT_ENABLED=y, passing "rodata=on" on the > kernel command-line (rather than "rodata=full") should turn off the > "full" behaviour, leaving writable linear aliases of read-only kernel > memory. Unfortunately, the option has no effect in this situation and > the only way to disable the "rodata=full" behaviour is to disable rodata > protection entirely by passing "rodata=off". > > Fix this by parsing the "on" and "off" options in the arch code, > additionally enforcing that 'rodata_full' cannot be set without also > setting 'rodata_enabled', allowing us to simplify a couple of checks > in the process. > I think this got broken when this code was refactored to use the generic parsing, which uses strtobool() and so it will match rodata=0/1, yes, no etc, and only 'full' is handled as a special case. Not sure whether this matters, and I'd much prefer supporting only 'on', 'off' and 'full' because we'll need to parse rodata= early too (for my big head.S / LPA2 refactor). > Cc: Ard Biesheuvel <ardb@kernel.org> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Mark Rutland <mark.rutland@arm.com> > Signed-off-by: Will Deacon <will@kernel.org> > --- > arch/arm64/include/asm/setup.h | 17 +++++++++++++++-- > arch/arm64/mm/pageattr.c | 7 +++---- > 2 files changed, 18 insertions(+), 6 deletions(-) > > diff --git a/arch/arm64/include/asm/setup.h b/arch/arm64/include/asm/setup.h > index f4af547ef54c..2e4d7da74fb8 100644 > --- a/arch/arm64/include/asm/setup.h > +++ b/arch/arm64/include/asm/setup.h > @@ -21,9 +21,22 @@ static inline bool arch_parse_debug_rodata(char *arg) > extern bool rodata_enabled; > extern bool rodata_full; > > - if (arg && !strcmp(arg, "full")) { > + if (!arg) > + return false; > + > + if (!strcmp(arg, "full")) { > + rodata_enabled = rodata_full = true; > + return true; > + } > + > + if (!strcmp(arg, "off")) { > + rodata_enabled = rodata_full = false; > + return true; > + } > + > + if (!strcmp(arg, "on")) { > rodata_enabled = true; > - rodata_full = true; > + rodata_full = false; > return true; > } > > diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c > index 8e2017ba5f1b..924843f1f661 100644 > --- a/arch/arm64/mm/pageattr.c > +++ b/arch/arm64/mm/pageattr.c > @@ -29,8 +29,8 @@ bool can_set_direct_map(void) > * > * KFENCE pool requires page-granular mapping if initialized late. > */ > - return (rodata_enabled && rodata_full) || debug_pagealloc_enabled() || > - arm64_kfence_can_set_direct_map(); > + return rodata_full || debug_pagealloc_enabled() || > + arm64_kfence_can_set_direct_map(); > } > > static int change_page_range(pte_t *ptep, unsigned long addr, void *data) > @@ -105,8 +105,7 @@ static int change_memory_common(unsigned long addr, int numpages, > * If we are manipulating read-only permissions, apply the same > * change to the linear mapping of the pages that back this VM area. > */ > - if (rodata_enabled && > - rodata_full && (pgprot_val(set_mask) == PTE_RDONLY || > + if (rodata_full && (pgprot_val(set_mask) == PTE_RDONLY || > pgprot_val(clear_mask) == PTE_RDONLY)) { > for (i = 0; i < area->nr_pages; i++) { > __change_memory_common((u64)page_address(area->pages[i]), > -- > 2.43.0.rc0.421.g78406f8d94-goog >
On Fri, Nov 17, 2023 at 10:09:04AM -0500, Ard Biesheuvel wrote: > On Fri, 17 Nov 2023 at 08:14, Will Deacon <will@kernel.org> wrote: > > > > When CONFIG_RODATA_FULL_DEFAULT_ENABLED=y, passing "rodata=on" on the > > kernel command-line (rather than "rodata=full") should turn off the > > "full" behaviour, leaving writable linear aliases of read-only kernel > > memory. Unfortunately, the option has no effect in this situation and > > the only way to disable the "rodata=full" behaviour is to disable rodata > > protection entirely by passing "rodata=off". > > > > Fix this by parsing the "on" and "off" options in the arch code, > > additionally enforcing that 'rodata_full' cannot be set without also > > setting 'rodata_enabled', allowing us to simplify a couple of checks > > in the process. > > > > I think this got broken when this code was refactored to use the > generic parsing, which uses strtobool() and so it will match > rodata=0/1, yes, no etc, and only 'full' is handled as a special case. strtobool() is such a hack! I don't think the core code uses it anymore though: set_debug_rodata() uses good ol' strcmp() since 2e8cff0a0eee ("arm64: fix rodata=full"). > Not sure whether this matters, and I'd much prefer supporting only > 'on', 'off' and 'full' because we'll need to parse rodata= early too > (for my big head.S / LPA2 refactor). I think that matches the current behaviour (minus the bug I'm fixing here) and even the documentation in kernel-parameters.txt only talks about those three values. Will
On Tue, 21 Nov 2023 at 10:03, Will Deacon <will@kernel.org> wrote: > > On Fri, Nov 17, 2023 at 10:09:04AM -0500, Ard Biesheuvel wrote: > > On Fri, 17 Nov 2023 at 08:14, Will Deacon <will@kernel.org> wrote: > > > > > > When CONFIG_RODATA_FULL_DEFAULT_ENABLED=y, passing "rodata=on" on the > > > kernel command-line (rather than "rodata=full") should turn off the > > > "full" behaviour, leaving writable linear aliases of read-only kernel > > > memory. Unfortunately, the option has no effect in this situation and > > > the only way to disable the "rodata=full" behaviour is to disable rodata > > > protection entirely by passing "rodata=off". > > > > > > Fix this by parsing the "on" and "off" options in the arch code, > > > additionally enforcing that 'rodata_full' cannot be set without also > > > setting 'rodata_enabled', allowing us to simplify a couple of checks > > > in the process. > > > > > > > I think this got broken when this code was refactored to use the > > generic parsing, which uses strtobool() and so it will match > > rodata=0/1, yes, no etc, and only 'full' is handled as a special case. > > strtobool() is such a hack! I don't think the core code uses it anymore > though: set_debug_rodata() uses good ol' strcmp() since 2e8cff0a0eee > ("arm64: fix rodata=full"). > > > Not sure whether this matters, and I'd much prefer supporting only > > 'on', 'off' and 'full' because we'll need to parse rodata= early too > > (for my big head.S / LPA2 refactor). > > I think that matches the current behaviour (minus the bug I'm fixing here) > and even the documentation in kernel-parameters.txt only talks about those > three values. > OK I didn't realize this was the current behavior already. Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
On Fri, Nov 17, 2023 at 01:14:22PM +0000, Will Deacon wrote: > When CONFIG_RODATA_FULL_DEFAULT_ENABLED=y, passing "rodata=on" on the > kernel command-line (rather than "rodata=full") should turn off the > "full" behaviour, leaving writable linear aliases of read-only kernel > memory. Unfortunately, the option has no effect in this situation and > the only way to disable the "rodata=full" behaviour is to disable rodata > protection entirely by passing "rodata=off". > > Fix this by parsing the "on" and "off" options in the arch code, > additionally enforcing that 'rodata_full' cannot be set without also > setting 'rodata_enabled', allowing us to simplify a couple of checks > in the process. > > Cc: Ard Biesheuvel <ardb@kernel.org> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Mark Rutland <mark.rutland@arm.com> > Signed-off-by: Will Deacon <will@kernel.org> I'll add a: Fixes: 2e8cff0a0eee ("arm64: fix rodata=full") My quick grep shows that rodata_full = false was removed by the above commit, leaving the default set by the config option.
On Fri, 17 Nov 2023 13:14:22 +0000, Will Deacon wrote: > When CONFIG_RODATA_FULL_DEFAULT_ENABLED=y, passing "rodata=on" on the > kernel command-line (rather than "rodata=full") should turn off the > "full" behaviour, leaving writable linear aliases of read-only kernel > memory. Unfortunately, the option has no effect in this situation and > the only way to disable the "rodata=full" behaviour is to disable rodata > protection entirely by passing "rodata=off". > > [...] Applied to arm64 (for-next/fixes), thanks! [1/1] arm64: mm: Fix "rodata=on" when CONFIG_RODATA_FULL_DEFAULT_ENABLED=y https://git.kernel.org/arm64/c/acfa60dbe038
diff --git a/arch/arm64/include/asm/setup.h b/arch/arm64/include/asm/setup.h index f4af547ef54c..2e4d7da74fb8 100644 --- a/arch/arm64/include/asm/setup.h +++ b/arch/arm64/include/asm/setup.h @@ -21,9 +21,22 @@ static inline bool arch_parse_debug_rodata(char *arg) extern bool rodata_enabled; extern bool rodata_full; - if (arg && !strcmp(arg, "full")) { + if (!arg) + return false; + + if (!strcmp(arg, "full")) { + rodata_enabled = rodata_full = true; + return true; + } + + if (!strcmp(arg, "off")) { + rodata_enabled = rodata_full = false; + return true; + } + + if (!strcmp(arg, "on")) { rodata_enabled = true; - rodata_full = true; + rodata_full = false; return true; } diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c index 8e2017ba5f1b..924843f1f661 100644 --- a/arch/arm64/mm/pageattr.c +++ b/arch/arm64/mm/pageattr.c @@ -29,8 +29,8 @@ bool can_set_direct_map(void) * * KFENCE pool requires page-granular mapping if initialized late. */ - return (rodata_enabled && rodata_full) || debug_pagealloc_enabled() || - arm64_kfence_can_set_direct_map(); + return rodata_full || debug_pagealloc_enabled() || + arm64_kfence_can_set_direct_map(); } static int change_page_range(pte_t *ptep, unsigned long addr, void *data) @@ -105,8 +105,7 @@ static int change_memory_common(unsigned long addr, int numpages, * If we are manipulating read-only permissions, apply the same * change to the linear mapping of the pages that back this VM area. */ - if (rodata_enabled && - rodata_full && (pgprot_val(set_mask) == PTE_RDONLY || + if (rodata_full && (pgprot_val(set_mask) == PTE_RDONLY || pgprot_val(clear_mask) == PTE_RDONLY)) { for (i = 0; i < area->nr_pages; i++) { __change_memory_common((u64)page_address(area->pages[i]),
When CONFIG_RODATA_FULL_DEFAULT_ENABLED=y, passing "rodata=on" on the kernel command-line (rather than "rodata=full") should turn off the "full" behaviour, leaving writable linear aliases of read-only kernel memory. Unfortunately, the option has no effect in this situation and the only way to disable the "rodata=full" behaviour is to disable rodata protection entirely by passing "rodata=off". Fix this by parsing the "on" and "off" options in the arch code, additionally enforcing that 'rodata_full' cannot be set without also setting 'rodata_enabled', allowing us to simplify a couple of checks in the process. Cc: Ard Biesheuvel <ardb@kernel.org> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Mark Rutland <mark.rutland@arm.com> Signed-off-by: Will Deacon <will@kernel.org> --- arch/arm64/include/asm/setup.h | 17 +++++++++++++++-- arch/arm64/mm/pageattr.c | 7 +++---- 2 files changed, 18 insertions(+), 6 deletions(-)