arm64: mm: Fix "rodata=on" when CONFIG_RODATA_FULL_DEFAULT_ENABLED=y

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 <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Mark Rutland <[email protected]>

Signed-off-by Will Deacon <[email protected]>

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 <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Mark Rutland <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/include/asm/setup.h | 17 +++++++++++++++--
 arch/arm64/mm/pageattr.c       |  7 +++----
 2 files changed, 18 insertions(+), 6 deletions(-)

arch/arm64/include/asm/setup.h +15 -2

index f4af547ef54c..2e4d7da74fb8 100644
@@ -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;
        }
 

arch/arm64/mm/pageattr.c +3 -5

index 8e2017ba5f1b..924843f1f661 100644
@@ -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

Quoted 8 lines · Will Deacon

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".

Quoted 5 lines

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).

Quoted 15 lines · Ard Biesheuvel

Quoted 8 lines · Will Deacon

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".

Quoted 5 lines

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.

Quoted 3 lines

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").

Quoted 3 lines

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.

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 <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Mark Rutland <[email protected]>

Signed-off-by Will Deacon <[email protected]>

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".

[...]