[v3,0/4] PAN for ARM32 using LPAE

Message

Linus Walleij March 12, 2024, 12:52 p.m. UTC

This is a patch set from Catalin that ended up on the back burner.

Since LPAE systems, i.e. ARM32 systems with a lot of physical memory,
will be with us for a while more, this is a pretty straight-forward
hardening measure that we should support.

The last patch explains the mechanism: since PAN using CPU domains
isn't available when using the LPAE MMU tables, we use the split
between the two translation base tables instead: TTBR0 is for
userspace pages and TTBR1 is for kernelspace tables. When executing
in kernelspace: we protect userspace by simply disabling page
walks in TTBR0.

The simplest way to test a PAN crash:
- Enable CONFIG_LKDTM
- echo "ACCESS_USERSPACE" | cat >/sys/kernel/debug/provoke-crash/DIRECT
- echo "EXEC_USERSPACE" | cat >/sys/kernel/debug/provoke-crash/DIRECT

This was also tested by a simple hack in the ELF loader:

create_elf_tables()
+       unsigned char *test;
(...)
        if (copy_to_user(u_rand_bytes, k_rand_bytes, sizeof(k_rand_bytes)))
                return -EFAULT;
+       /* Cause a kernelspace access to userspace memory */
+       test = (char *)u_rand_bytes;
+       pr_info("Some byte: %02x\n", *test);

This tries to read a byte from userspace memory right after the
first unconditional copy_to_user(), a function that carefully
switches access permissions if we're using PAN.

Without LPAE PAN this will just happily print these bytes from
userspace but with LPAE PAN it will cause a predictable
crash:

Run /init as init process
Some byte: ac
8<--- cut here ---
Unable to handle kernel paging request at virtual address 7ec59f6b when read
[7ec59f6b] *pgd=82c3b003, *pmd=82863003, *pte=e00000882f6f5f
Internal error: Oops: 206 [#1] SMP ARM
CPU: 0 PID: 47 Comm: rc.init Not tainted 6.7.0-rc1+ #25
Hardware name: ARM-Versatile Express
PC is at create_elf_tables+0x13c/0x608

Thus we can show that LPAE PAN does its job.

Signed-off-by: Linus Walleij <[e-mail]>
---
Changes in v3:
- Drop leftover uaccess_disabled() stub.
- Drop unnecesary volatile from the asm(mcr) call.
- Move all changes of ifdef to if defined() to the last patch
  and keep the preceding patch to its subject.
- Link to v2: https://lore.kernel.org/r/[e-mail]

Changes in v1 (from Catalins original patch set):
- Use IS_ENABLED() to avoid some ifdefs
Changes in v2:
- Make the TTBCR a separate field in struct svc_pt_regs as requested
  by Russell. Adjust code accordingly.
- Push the MM page fault permission check into a local function
  and avoid the too generic uaccess_disabled() as requested by Ard.
- Link to v1: https://lore.kernel.org/r/[e-mail]

---
Catalin Marinas (4):
      ARM: Add TTBCR_* definitions to pgtable-3level-hwdef.h
      ARM: Move asm statements accessing TTBCR into C functions
      ARM: Reduce the number of #ifdef CONFIG_CPU_SW_DOMAIN_PAN
      ARM: Implement PAN for LPAE by TTBR0 page table walks disablement

 arch/arm/Kconfig                            | 22 +++++++++--
 arch/arm/include/asm/assembler.h            |  1 +
 arch/arm/include/asm/pgtable-3level-hwdef.h | 26 +++++++++++++
 arch/arm/include/asm/proc-fns.h             | 12 ++++++
 arch/arm/include/asm/ptrace.h               |  1 +
 arch/arm/include/asm/uaccess-asm.h          | 58 +++++++++++++++++++++++++++--
 arch/arm/include/asm/uaccess.h              | 45 +++++++++++++++++++---
 arch/arm/kernel/asm-offsets.c               |  1 +
 arch/arm/kernel/suspend.c                   |  8 ++++
 arch/arm/lib/csumpartialcopyuser.S          | 20 +++++++++-
 arch/arm/mm/fault.c                         | 29 +++++++++++++++
 arch/arm/mm/mmu.c                           |  7 ++--
 12 files changed, 212 insertions(+), 18 deletions(-)
---
base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
change-id: 20231216-arm32-lpae-pan-56125ab63d63

Best regards,

Comments

Florian Fainelli March 12, 2024, 5:45 p.m. UTC | #1

Hi Linus,

On 3/12/24 05:52, Linus Walleij wrote:
> This is a patch set from Catalin that ended up on the back burner.
> 
> Since LPAE systems, i.e. ARM32 systems with a lot of physical memory,
> will be with us for a while more, this is a pretty straight-forward
> hardening measure that we should support.
> 
> The last patch explains the mechanism: since PAN using CPU domains
> isn't available when using the LPAE MMU tables, we use the split
> between the two translation base tables instead: TTBR0 is for
> userspace pages and TTBR1 is for kernelspace tables. When executing
> in kernelspace: we protect userspace by simply disabling page
> walks in TTBR0.
> 
> The simplest way to test a PAN crash:
> - Enable CONFIG_LKDTM
> - echo "ACCESS_USERSPACE" | cat >/sys/kernel/debug/provoke-crash/DIRECT
> - echo "EXEC_USERSPACE" | cat >/sys/kernel/debug/provoke-crash/DIRECT
> 
> This was also tested by a simple hack in the ELF loader:
> 
> create_elf_tables()
> +       unsigned char *test;
> (...)
>          if (copy_to_user(u_rand_bytes, k_rand_bytes, sizeof(k_rand_bytes)))
>                  return -EFAULT;
> +       /* Cause a kernelspace access to userspace memory */
> +       test = (char *)u_rand_bytes;
> +       pr_info("Some byte: %02x\n", *test);
> 
> This tries to read a byte from userspace memory right after the
> first unconditional copy_to_user(), a function that carefully
> switches access permissions if we're using PAN.
> 
> Without LPAE PAN this will just happily print these bytes from
> userspace but with LPAE PAN it will cause a predictable
> crash:
> 
> Run /init as init process
> Some byte: ac
> 8<--- cut here ---
> Unable to handle kernel paging request at virtual address 7ec59f6b when read
> [7ec59f6b] *pgd=82c3b003, *pmd=82863003, *pte=e00000882f6f5f
> Internal error: Oops: 206 [#1] SMP ARM
> CPU: 0 PID: 47 Comm: rc.init Not tainted 6.7.0-rc1+ #25
> Hardware name: ARM-Versatile Express
> PC is at create_elf_tables+0x13c/0x608
> 
> Thus we can show that LPAE PAN does its job.
> 
> Signed-off-by: Linus Walleij <[e-mail]>

Tested-by: Florian Fainelli <[e-mail]>

On ARCH_BRCMSTB, there was no noticeable performance drop with:

stress-ng --fault 0 --perf -t 1m

thanks a bunch for getting those patches out!

Linus Walleij March 13, 2024, 8:13 a.m. UTC | #2

On Tue, Mar 12, 2024 at 6:45 PM Florian Fainelli <[e-mail]> wrote:

> Tested-by: Florian Fainelli <[e-mail]>
>
> On ARCH_BRCMSTB, there was no noticeable performance drop with:
>
> stress-ng --fault 0 --perf -t 1m
>
> thanks a bunch for getting those patches out!

Thanks a lot Florian!

I think actually both the mitigations I have in the pipe (this one and
CFI) have low performance impact and are just nice to have (TM),
albeit CFI requires LLVM CLANG. Hopefully it will be possible to turn
on both.

Yours,
Linus Walleij