[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <mhng-32bb45f6-c7eb-4afb-a42a-a167a83ca760@palmer-ri-x1c9>
Date: Wed, 08 Nov 2023 19:47:49 -0800 (PST)
From: Palmer Dabbelt <palmer@...belt.com>
To: keescook@...omium.org
CC: songshuaishuai@...ylab.org,
Paul Walmsley <paul.walmsley@...ive.com>,
aou@...s.berkeley.edu, guoren@...nel.org,
Bjorn Topel <bjorn@...osinc.com>, jszhang@...nel.org,
Conor Dooley <conor.dooley@...rochip.com>,
andy.chiu@...ive.com, samitolvanen@...gle.com,
coelacanthushex@...il.com, linux-riscv@...ts.infradead.org,
linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH] riscv: Support RANDOMIZE_KSTACK_OFFSET
On Wed, 08 Nov 2023 15:52:34 PST (-0800), keescook@...omium.org wrote:
> On Wed, Nov 01, 2023 at 02:44:23PM +0800, Song Shuai wrote:
>> Inspired from arm64's implement -- commit 70918779aec9
>> ("arm64: entry: Enable random_kstack_offset support")
>>
>> Add support of kernel stack offset randomization while handling syscall,
>> the offset is defaultly limited by KSTACK_OFFSET_MAX() (i.e. 10 bits).
>>
>> In order to avoid trigger stack canaries (due to __builtin_alloca) and
>> slowing down the entry path, use __no_stack_protector attribute to
>> disable stack protector for do_trap_ecall_u() at the function level.
>>
>> Signed-off-by: Song Shuai <songshuaishuai@...ylab.org>
>
> I can't speak to the correctness of the entropy level, but the usage of
> the helpers looks correct to me.
As far as I can tell the comment matches how the system behaves. I'm
not sure if that much entropy is useful. I was poking around for a bit
to try and figure that out, but after reading that comment at the top of
include/linux/randomize_kstack.h I decided to stop ;)
So aside from those whitespace errors Damien pointed out,
Reviewed-by: Palmer Dabbelt <palmer@...osinc.com>
It's too late for the merge window for me, but
Acked-by: Palmer Dabbelt <palmer@...osinc.com>
in case someone else wants to take it still. Otherwise I'll try and
remember to pick it up right after the merge window, but with Plumbers
things might be a bit clunky.
> Reviewed-by: Kees Cook <keescook@...omium.org>
>
> -Kees
>
>> ---
>> Testing with randomize_kstack_offset=y cmdline, lkdtm/stack-entropy.sh
>> showed appropriate stack offset instead of zero.
>> ---
>> arch/riscv/Kconfig | 1 +
>> arch/riscv/kernel/traps.c | 18 +++++++++++++++++-
>> 2 files changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>> index d607ab0f7c6d..0e843de33f0c 100644
>> --- a/arch/riscv/Kconfig
>> +++ b/arch/riscv/Kconfig
>> @@ -100,6 +100,7 @@ config RISCV
>> select HAVE_ARCH_KGDB_QXFER_PKT
>> select HAVE_ARCH_MMAP_RND_BITS if MMU
>> select HAVE_ARCH_MMAP_RND_COMPAT_BITS if COMPAT
>> + select HAVE_ARCH_RANDOMIZE_KSTACK_OFFSET
>> select HAVE_ARCH_SECCOMP_FILTER
>> select HAVE_ARCH_THREAD_STRUCT_WHITELIST
>> select HAVE_ARCH_TRACEHOOK
>> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
>> index 19807c4d3805..3f869b2d47c3 100644
>> --- a/arch/riscv/kernel/traps.c
>> +++ b/arch/riscv/kernel/traps.c
>> @@ -6,6 +6,7 @@
>> #include <linux/cpu.h>
>> #include <linux/kernel.h>
>> #include <linux/init.h>
>> +#include <linux/randomize_kstack.h>
>> #include <linux/sched.h>
>> #include <linux/sched/debug.h>
>> #include <linux/sched/signal.h>
>> @@ -296,9 +297,11 @@ asmlinkage __visible __trap_section void do_trap_break(struct pt_regs *regs)
>> }
>> }
>>
>> -asmlinkage __visible __trap_section void do_trap_ecall_u(struct pt_regs *regs)
>> +asmlinkage __visible __trap_section __no_stack_protector
>> +void do_trap_ecall_u(struct pt_regs *regs)
>> {
>> if (user_mode(regs)) {
>> +
>> long syscall = regs->a7;
>>
>> regs->epc += 4;
>> @@ -308,10 +311,23 @@ asmlinkage __visible __trap_section void do_trap_ecall_u(struct pt_regs *regs)
>>
>> syscall = syscall_enter_from_user_mode(regs, syscall);
>>
>> + add_random_kstack_offset();
>> +
>> if (syscall >= 0 && syscall < NR_syscalls)
>> syscall_handler(regs, syscall);
>> else if (syscall != -1)
>> regs->a0 = -ENOSYS;
>> + /*
>> + * Ultimately, this value will get limited by KSTACK_OFFSET_MAX(),
>> + * so the maximum stack offset is 1k bytes (10 bits).
>> + *
>> + * The actual entropy will be further reduced by the compiler when
>> + * applying stack alignment constraints: 16-byte (i.e. 4-bit) aligned
>> + * for RV32I or RV64I.
>> + *
>> + * The resulting 6 bits of entropy is seen in SP[9:4].
>> + */
>> + choose_random_kstack_offset(get_random_u16());
>>
>> syscall_exit_to_user_mode(regs);
>> } else {
>> --
>> 2.20.1
>>
Powered by blists - more mailing lists