| 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
| ||
|
Message-ID: <aAl_HRk49lnseiio@debug.ba.rivosinc.com>
Date: Wed, 23 Apr 2025 17:00:29 -0700
From: Deepak Gupta <debug@...osinc.com>
To: Radim Krčmář <rkrcmar@...tanamicro.com>
Cc: Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
Borislav Petkov <bp@...en8.de>,
Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
"H. Peter Anvin" <hpa@...or.com>,
Andrew Morton <akpm@...ux-foundation.org>,
"Liam R. Howlett" <Liam.Howlett@...cle.com>,
Vlastimil Babka <vbabka@...e.cz>,
Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
Paul Walmsley <paul.walmsley@...ive.com>,
Palmer Dabbelt <palmer@...belt.com>,
Albert Ou <aou@...s.berkeley.edu>, Conor Dooley <conor@...nel.org>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Arnd Bergmann <arnd@...db.de>,
Christian Brauner <brauner@...nel.org>,
Peter Zijlstra <peterz@...radead.org>,
Oleg Nesterov <oleg@...hat.com>,
Eric Biederman <ebiederm@...ssion.com>, Kees Cook <kees@...nel.org>,
Jonathan Corbet <corbet@....net>, Shuah Khan <shuah@...nel.org>,
Jann Horn <jannh@...gle.com>, Conor Dooley <conor+dt@...nel.org>,
linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
linux-mm@...ck.org, linux-riscv@...ts.infradead.org,
devicetree@...r.kernel.org, linux-arch@...r.kernel.org,
linux-doc@...r.kernel.org, linux-kselftest@...r.kernel.org,
alistair.francis@....com, richard.henderson@...aro.org,
jim.shu@...ive.com, andybnac@...il.com, kito.cheng@...ive.com,
charlie@...osinc.com, atishp@...osinc.com, evan@...osinc.com,
cleger@...osinc.com, broonie@...nel.org, rick.p.edgecombe@...el.com,
Zong Li <zong.li@...ive.com>,
linux-riscv <linux-riscv-bounces@...ts.infradead.org>
Subject: Re: [PATCH v12 05/28] riscv: usercfi state for task and save/restore
of CSR_SSP on trap entry/exit
On Thu, Apr 10, 2025 at 01:04:39PM +0200, Radim Krčmář wrote:
>2025-03-14T14:39:24-07:00, Deepak Gupta <debug@...osinc.com>:
>> diff --git a/arch/riscv/include/asm/thread_info.h b/arch/riscv/include/asm/thread_info.h
>> @@ -62,6 +62,9 @@ struct thread_info {
>> long user_sp; /* User stack pointer */
>> int cpu;
>> unsigned long syscall_work; /* SYSCALL_WORK_ flags */
>> +#ifdef CONFIG_RISCV_USER_CFI
>> + struct cfi_status user_cfi_state;
>> +#endif
>
>I don't think it makes sense to put all the data in thread_info.
>kernel_ssp and user_ssp is more than enough and the rest can comfortably
>live elsewhere in task_struct.
>
>thread_info is supposed to be as small as possible -- just spanning
>multiple cache-lines could be noticeable.
I can change it to only include only `user_ssp`, base and size.
But before we go there, see below:
$ pahole -C thread_info kbuild/vmlinux
struct thread_info {
long unsigned int flags; /* 0 8 */
int preempt_count; /* 8 4 */
/* XXX 4 bytes hole, try to pack */
long int kernel_sp; /* 16 8 */
long int user_sp; /* 24 8 */
int cpu; /* 32 4 */
/* XXX 4 bytes hole, try to pack */
long unsigned int syscall_work; /* 40 8 */
struct cfi_status user_cfi_state; /* 48 32 */
/* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */
long unsigned int a0; /* 80 8 */
long unsigned int a1; /* 88 8 */
long unsigned int a2; /* 96 8 */
/* size: 104, cachelines: 2, members: 10 */
/* sum members: 96, holes: 2, sum holes: 8 */
/* last cacheline: 40 bytes */
};
If we were to remove entire `cfi_status`, it would still be 72 bytes (88 bytes
if shadow call stack were enabled) and already spans across two cachelines. I
did see the comment above that it should fit inside a cacheline. Although I
assumed its stale comment given that it already spans across cacheline and I
didn't see any special mention in commit messages of changes which grew this
structure above one cacheline. So I assumed this was a stale comment.
On the other hand, whenever enable/lock bits are checked, there is a high
likelyhood that user_ssp and other fields are going to be accessed and
thus it actually might be helpful to have it all in one cacheline during
runtime.
So I am not sure if its helpful sticking to the comment which already is stale.
>
>> diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
>> @@ -147,6 +147,20 @@ SYM_CODE_START(handle_exception)
>>
>> REG_L s0, TASK_TI_USER_SP(tp)
>> csrrc s1, CSR_STATUS, t0
>> + /*
>> + * If previous mode was U, capture shadow stack pointer and save it away
>> + * Zero CSR_SSP at the same time for sanitization.
>> + */
>> + ALTERNATIVE("nop; nop; nop; nop",
>> + __stringify( \
>> + andi s2, s1, SR_SPP; \
>> + bnez s2, skip_ssp_save; \
>> + csrrw s2, CSR_SSP, x0; \
>> + REG_S s2, TASK_TI_USER_SSP(tp); \
>> + skip_ssp_save:),
>> + 0,
>> + RISCV_ISA_EXT_ZICFISS,
>> + CONFIG_RISCV_USER_CFI)
>
>(I'd prefer this closer to the user_sp and kernel_sp swap, it's breaking
> the flow here. We also already know if we've returned from userspace
> or not even without SR_SPP, but reusing the information might tangle
> the logic.)
>
>> csrr s2, CSR_EPC
>> csrr s3, CSR_TVAL
>> csrr s4, CSR_CAUSE
>> @@ -236,6 +250,18 @@ SYM_CODE_START_NOALIGN(ret_from_exception)
>> csrw CSR_SCRATCH, tp
>> +
>> + /*
>> + * Going back to U mode, restore shadow stack pointer
>> + */
>
>Are we? I think we can be just as well returning back to kernel-space.
>Similar to how we can enter the exception handler from kernel-space.
>
>> + ALTERNATIVE("nop; nop",
>> + __stringify( \
>> + REG_L s3, TASK_TI_USER_SSP(tp); \
>> + csrw CSR_SSP, s3),
>> + 0,
>> + RISCV_ISA_EXT_ZICFISS,
>> + CONFIG_RISCV_USER_CFI)
>> +
>
>Thanks.
Powered by blists - more mailing lists