| 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
| ||
|
Date: Sun, 12 May 2024 22:12:33 +0200
From: Alexandre Ghiti <alex@...ti.fr>
To: Deepak Gupta <debug@...osinc.com>, linux-riscv@...ts.infradead.org,
linux-kernel@...r.kernel.org, llvm@...ts.linux.dev
Cc: paul.walmsley@...ive.com, palmer@...belt.com, aou@...s.berkeley.edu,
nathan@...nel.org, ndesaulniers@...gle.com, morbo@...gle.com,
justinstitt@...gle.com, andy.chiu@...ive.com, hankuan.chen@...ive.com,
guoren@...nel.org, greentime.hu@...ive.com, samitolvanen@...gle.com,
cleger@...osinc.com, apatel@...tanamicro.com, ajones@...tanamicro.com,
conor.dooley@...rochip.com, mchitale@...tanamicro.com,
dbarboza@...tanamicro.com, waylingii@...il.com, sameo@...osinc.com,
alexghiti@...osinc.com, akpm@...ux-foundation.org,
shikemeng@...weicloud.com, rppt@...nel.org, charlie@...osinc.com,
xiao.w.wang@...el.com, willy@...radead.org, jszhang@...nel.org,
leobras@...hat.com, songshuaishuai@...ylab.org, haxel@....de,
samuel.holland@...ive.com, namcaov@...il.com, bjorn@...osinc.com,
cuiyunhui@...edance.com, wangkefeng.wang@...wei.com, falcon@...ylab.org,
viro@...iv.linux.org.uk, bhe@...hat.com, chenjiahao16@...wei.com,
hca@...ux.ibm.com, arnd@...db.de, kent.overstreet@...ux.dev,
boqun.feng@...il.com, oleg@...hat.com, paulmck@...nel.org,
broonie@...nel.org, rick.p.edgecombe@...el.com
Subject: Re: [RFC PATCH 07/12] riscv/mm: prepare shadow stack for init task
for kernel cfi
On 09/04/2024 08:10, Deepak Gupta wrote:
> Under CONFIG_SHADOW_CALL_STACK, shadow call stack goes into data section.
> Although with CONFIG_DYNAMIC_SCS on riscv, hardware assisted shadow stack
> are used. Hardware assisted shadow stack on riscv uses PTE.R=0, PTE.W=1 &
> PTE.X=0 encodings. Without CONFIG_DYNAMIC_SCS, shadow stack for init is
> placed in data section and thus regular read/write encodings are applied
> to it. Although with with CONFIG_DYNAMIC_SCS, they need to go into
> different section. This change places it into `.shadowstack` section.
> As part of this change early boot code (`setup_vm`), applies appropriate
> PTE encodings to shadow call stack for init placed in `.shadowstack`
> section.
>
> Signed-off-by: Deepak Gupta <debug@...osinc.com>
> ---
> arch/riscv/include/asm/pgtable.h | 4 ++++
> arch/riscv/include/asm/sections.h | 22 +++++++++++++++++++++
> arch/riscv/include/asm/thread_info.h | 10 ++++++++--
> arch/riscv/kernel/vmlinux.lds.S | 12 ++++++++++++
> arch/riscv/mm/init.c | 29 +++++++++++++++++++++-------
> 5 files changed, 68 insertions(+), 9 deletions(-)
>
> diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> index 9f8ea0e33eb1..3409b250390d 100644
> --- a/arch/riscv/include/asm/pgtable.h
> +++ b/arch/riscv/include/asm/pgtable.h
> @@ -197,6 +197,10 @@ extern struct pt_alloc_ops pt_ops __initdata;
> #define PAGE_KERNEL_READ_EXEC __pgprot((_PAGE_KERNEL & ~_PAGE_WRITE) \
> | _PAGE_EXEC)
>
> +#ifdef CONFIG_DYNAMIC_SCS
> +#define PAGE_KERNEL_SHADOWSTACK __pgprot(_PAGE_KERNEL & ~(_PAGE_READ | _PAGE_EXEC))
> +#endif
> +
Not sure the ifdefs are necessary here, but I'll let others jump in. We
have a lot of them, so we should try not to add.
> #define PAGE_TABLE __pgprot(_PAGE_TABLE)
>
> #define _PAGE_IOREMAP ((_PAGE_KERNEL & ~_PAGE_MTMASK) | _PAGE_IO)
> diff --git a/arch/riscv/include/asm/sections.h b/arch/riscv/include/asm/sections.h
> index a393d5035c54..4c4154d0021e 100644
> --- a/arch/riscv/include/asm/sections.h
> +++ b/arch/riscv/include/asm/sections.h
> @@ -14,6 +14,10 @@ extern char __init_data_begin[], __init_data_end[];
> extern char __init_text_begin[], __init_text_end[];
> extern char __alt_start[], __alt_end[];
> extern char __exittext_begin[], __exittext_end[];
> +#ifdef CONFIG_DYNAMIC_SCS
> +extern char __init_shstk_start[], __init_shstk_end[];
> +#endif
> +extern char __end_srodata[];
>
> static inline bool is_va_kernel_text(uintptr_t va)
> {
> @@ -31,4 +35,22 @@ static inline bool is_va_kernel_lm_alias_text(uintptr_t va)
> return va >= start && va < end;
> }
>
> +#ifdef CONFIG_DYNAMIC_SCS
> +static inline bool is_va_init_shadow_stack_early(uintptr_t va)
> +{
> + uintptr_t start = (uintptr_t)(kernel_mapping_pa_to_va(__init_shstk_start));
> + uintptr_t end = (uintptr_t)(kernel_mapping_pa_to_va(__init_shstk_end));
> +
> + return va >= start && va < end;
> +}
> +
> +static inline bool is_va_init_shadow_stack(uintptr_t va)
> +{
> + uintptr_t start = (uintptr_t)(__init_shstk_start);
> + uintptr_t end = (uintptr_t)(__init_shstk_end);
> +
> + return va >= start && va < end;
> +}
> +#endif
You could have used an early flag and have only one function but that's
up to you.
> +
> #endif /* __ASM_SECTIONS_H */
> diff --git a/arch/riscv/include/asm/thread_info.h b/arch/riscv/include/asm/thread_info.h
> index 5d473343634b..7ae28d627f84 100644
> --- a/arch/riscv/include/asm/thread_info.h
> +++ b/arch/riscv/include/asm/thread_info.h
> @@ -63,12 +63,18 @@ struct thread_info {
> };
>
> #ifdef CONFIG_SHADOW_CALL_STACK
> +#ifdef CONFIG_DYNAMIC_SCS
> #define INIT_SCS \
> - .scs_base = init_shadow_call_stack, \
> + .scs_base = init_shadow_call_stack, \
> + .scs_sp = &init_shadow_call_stack[SCS_SIZE / sizeof(long)],
> +#else
> +#define INIT_SCS \
> + .scs_base = init_shadow_call_stack, \
> .scs_sp = init_shadow_call_stack,
> +#endif /* CONFIG_DYNAMIC_SCS */
> #else
> #define INIT_SCS
> -#endif
> +#endif /* CONFIG_SHADOW_CALL_STACK */
>
> /*
> * macros/functions for gaining access to the thread information structure
> diff --git a/arch/riscv/kernel/vmlinux.lds.S b/arch/riscv/kernel/vmlinux.lds.S
> index 002ca58dd998..cccc51f845ab 100644
> --- a/arch/riscv/kernel/vmlinux.lds.S
> +++ b/arch/riscv/kernel/vmlinux.lds.S
> @@ -126,6 +126,18 @@ SECTIONS
> *(.srodata*)
> }
>
> + . = ALIGN(SECTION_ALIGN);
> + __end_srodata = .;
> +
> +#ifdef CONFIG_DYNAMIC_SCS
> + .shadowstack : AT(ADDR(.shadowstack) - LOAD_OFFSET){
> + __init_shstk_start = .;
> + KEEP(*(.shadowstack..init))
> + . = __init_shstk_start + PAGE_SIZE;
> + __init_shstk_end = .;
> + }
> +#endif
> +
> . = ALIGN(SECTION_ALIGN);
> _data = .;
>
> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> index fe8e159394d8..5b6f0cfa5719 100644
> --- a/arch/riscv/mm/init.c
> +++ b/arch/riscv/mm/init.c
> @@ -713,14 +713,22 @@ static __init pgprot_t pgprot_from_va(uintptr_t va)
> if (IS_ENABLED(CONFIG_64BIT) && is_va_kernel_lm_alias_text(va))
> return PAGE_KERNEL_READ;
>
> +#ifdef CONFIG_DYNAMIC_SCS
> + /* If init task's shadow stack va, return write only page protections */
> + if (IS_ENABLED(CONFIG_64BIT) && is_va_init_shadow_stack(va)) {
> + pr_info("Shadow stack protections are being applied to for init\n");
> + return PAGE_KERNEL_SHADOWSTACK;
> + }
> +#endif
To avoid the ifdef here, I would hide it inis_va_init_shadow_stack().
> +
> return PAGE_KERNEL;
> }
>
> void mark_rodata_ro(void)
> {
> - set_kernel_memory(__start_rodata, _data, set_memory_ro);
> + set_kernel_memory(__start_rodata, __end_srodata, set_memory_ro);
> if (IS_ENABLED(CONFIG_64BIT))
> - set_kernel_memory(lm_alias(__start_rodata), lm_alias(_data),
> + set_kernel_memory(lm_alias(__start_rodata), lm_alias(__end_srodata),
> set_memory_ro);
> }
> #else
> @@ -913,14 +921,21 @@ static void __init create_kernel_page_table(pgd_t *pgdir,
> static void __init create_kernel_page_table(pgd_t *pgdir, bool early)
> {
> uintptr_t va, end_va;
> + pgprot_t prot;
>
> end_va = kernel_map.virt_addr + kernel_map.size;
> - for (va = kernel_map.virt_addr; va < end_va; va += PMD_SIZE)
> + for (va = kernel_map.virt_addr; va < end_va; va += PMD_SIZE) {
> + prot = PAGE_KERNEL_EXEC;
> +#ifdef CONFIG_DYNAMIC_SCS
> + if (early && is_va_init_shadow_stack_early(va))
> + prot = PAGE_KERNEL_SHADOWSTACK;
> +#endif
Ditto here to avoid the ifdef, hide it intois_va_init_shadow_stack_early().
> create_pgd_mapping(pgdir, va,
> - kernel_map.phys_addr + (va - kernel_map.virt_addr),
> - PMD_SIZE,
> - early ?
> - PAGE_KERNEL_EXEC : pgprot_from_va(va));
> + kernel_map.phys_addr + (va - kernel_map.virt_addr),
> + PMD_SIZE,
> + early ?
The 3 lines above are not modified, so no need to indent them.
> + prot : pgprot_from_va(va));
> + }
> }
> #endif
>
Apart from the nits above, you can add:
Reviewed-by: Alexandre Ghiti <alexghiti@...osinc.com>
Thanks,
Alex
Powered by blists - more mailing lists