Re: git: ae92ace05fd4 - main - Per-thread stack canary on arm64

> On 30 Nov 2021, at 14:23, Andrew Turner <andrew@freebsd.org> wrote:
> 
> 
> 
>> On 30 Nov 2021, at 13:30, Jessica Clarke <jrtc27@freebsd.org> wrote:
>> 
>> On 26 Nov 2021, at 14:51, Andrew Turner <andrew@FreeBSD.org> wrote:
>>> 
>>> The branch main has been updated by andrew:
>>> 
>>> URL: https://cgit.FreeBSD.org/src/commit/?id=ae92ace05fd4fcf64e3bb787951578f655b1fa5f
>>> 
>>> commit ae92ace05fd4fcf64e3bb787951578f655b1fa5f
>>> Author:     Andrew Turner <andrew@FreeBSD.org>
>>> AuthorDate: 2021-11-22 15:20:51 +0000
>>> Commit:     Andrew Turner <andrew@FreeBSD.org>
>>> CommitDate: 2021-11-26 14:44:00 +0000
>>> 
>>>   Per-thread stack canary on arm64
>>> 
>>>   With the update to llvm 13 we are able to tell the compiler it can find
>>>   the SSP canary relative to the register that holds the userspace stack
>>>   pointer. As this is unused in most of the kernel it can be used here
>>>   to point to a per-thread SSP canary.
>>> 
>>>   As the kernel could be built with an old toolchain, e.g. when upgrading
>>>   from 13, add a warning that the options was enabled but the compiler
>>>   doesn't support it to both the build and kernel boot.
>>> 
>>>   Discussed with: emaste
>>>   Sponsored by:   The FreeBSD Foundation
>>>   Differential Revision: https://reviews.freebsd.org/D33079
>>> ---
>>> sys/arm64/arm64/exception.S  |  7 +++++++
>>> sys/arm64/arm64/genassym.c   |  1 +
>>> sys/arm64/arm64/locore.S     | 14 ++++++++++++++
>>> sys/arm64/arm64/machdep.c    | 22 ++++++++++++++++++++++
>>> sys/arm64/arm64/pmap.c       |  4 ++++
>>> sys/arm64/arm64/vm_machdep.c | 10 ++++++++++
>>> sys/arm64/conf/std.arm64     |  1 +
>>> sys/arm64/include/proc.h     |  1 +
>>> sys/conf/Makefile.arm64      | 14 ++++++++++++++
>>> sys/conf/options.arm64       |  4 ++++
>>> 10 files changed, 78 insertions(+)
>>> 
>>> diff --git a/sys/arm64/arm64/exception.S b/sys/arm64/arm64/exception.S
>>> index 22f6b7ce6145..d81bbce0efc7 100644
>>> --- a/sys/arm64/arm64/exception.S
>>> +++ b/sys/arm64/arm64/exception.S
>>> @@ -66,6 +66,13 @@ __FBSDID("$FreeBSD$");
>>> 	mrs	x18, tpidr_el1
>>> 	add	x29, sp, #(TF_SIZE)
>>> .if \el == 0
>>> +#if defined(PERTHREAD_SSP)
>>> +	/* Load the SSP canary to sp_el0 */
>>> +	ldr	x1, [x18, #(PC_CURTHREAD)]
>>> +	add	x1, x1, #(TD_MD_CANARY)
>>> +	msr	sp_el0, x1
>>> +#endif
>>> +
>>> 	/* Apply the SSBD (CVE-2018-3639) workaround if needed */
>>> 	ldr	x1, [x18, #PC_SSBD]
>>> 	cbz	x1, 1f
>>> diff --git a/sys/arm64/arm64/genassym.c b/sys/arm64/arm64/genassym.c
>>> index 1575a0158dec..8e3ddc48317b 100644
>>> --- a/sys/arm64/arm64/genassym.c
>>> +++ b/sys/arm64/arm64/genassym.c
>>> @@ -73,6 +73,7 @@ ASSYM(TD_PCB, offsetof(struct thread, td_pcb));
>>> ASSYM(TD_FLAGS, offsetof(struct thread, td_flags));
>>> ASSYM(TD_FRAME, offsetof(struct thread, td_frame));
>>> ASSYM(TD_LOCK, offsetof(struct thread, td_lock));
>>> +ASSYM(TD_MD_CANARY, offsetof(struct thread, td_md.md_canary));
>>> 
>>> ASSYM(TF_SIZE, sizeof(struct trapframe));
>>> ASSYM(TF_SP, offsetof(struct trapframe, tf_sp));
>>> diff --git a/sys/arm64/arm64/locore.S b/sys/arm64/arm64/locore.S
>>> index 92415aab1555..bc9a7271e93a 100644
>>> --- a/sys/arm64/arm64/locore.S
>>> +++ b/sys/arm64/arm64/locore.S
>>> @@ -116,6 +116,13 @@ virtdone:
>>> 	cmp	x15, x14
>>> 	b.lo	1b
>>> 
>>> +#if defined(PERTHREAD_SSP)
>>> +	/* Set sp_el0 to the boot canary for early per-thread SSP to work */
>>> +	adrp	x15, boot_canary
>>> +	add	x15, x15, :lo12:boot_canary
>>> +	msr	sp_el0, x15
>>> +#endif
>>> +
>>> 	/* Backup the module pointer */
>>> 	mov	x1, x0
>>> 
>>> @@ -200,6 +207,13 @@ mp_virtdone:
>>> 	ldr	x4, [x4]
>>> 	mov	sp, x4
>>> 
>>> +#if defined(PERTHREAD_SSP)
>>> +	/* Set sp_el0 to the boot canary for early per-thread SSP to work */
>>> +	adrp	x15, boot_canary
>>> +	add	x15, x15, :lo12:boot_canary
>>> +	msr	sp_el0, x15
>>> +#endif
>>> +
>>> 	/* Load the kernel ttbr0 pagetable */
>>> 	msr	ttbr0_el1, x27
>>> 	isb
>>> diff --git a/sys/arm64/arm64/machdep.c b/sys/arm64/arm64/machdep.c
>>> index 59a634f4d30c..821d9ba19022 100644
>>> --- a/sys/arm64/arm64/machdep.c
>>> +++ b/sys/arm64/arm64/machdep.c
>>> @@ -109,6 +109,14 @@ enum arm64_bus arm64_bus_method = ARM64_BUS_NONE;
>>> */
>>> struct pcpu pcpu0;
>>> 
>>> +#if defined(PERTHREAD_SSP)
>>> +/*
>>> + * The boot SSP canary. Will be replaced with a per-thread canary when
>>> + * scheduling has started.
>>> + */
>>> +uintptr_t boot_canary = 0x49a2d892bc05a0b1ul;
>> 
>> Is it *really* a pointer? That sure looks like it’s really a size_t or
>> unsigned long. I doubt you’d want a capability on CHERI (well, you’d
>> turn the feature off because it’s a waste of time there, but still).
>> 
>> Jess
> 
> It’s a uintptr_t in glibc. It’s also sitting beside other pointers (the return address, etc) so any wasted space on the stack would be from the size increase, not due to padding for alignment.

Just because space gets wasted on the stack doesn’t mean it’s a
uintptr_t, and GNU code is hardly known for its correct use of
integer-vs-pointer types. In CHERI-LLVM the stack protector (if you use
the hidden option to allow using it) is an integer, not a pointer, and
only of size 32/64 bits. Loading 64 bits from a uintptr_t on a 64-bit
little-endian CHERI architecture happens to be ok, but it’s
conceptually the wrong thing, and on big-endian CHERI would load the
metadata half of the value which, being null-derived, would always be 0
no matter the value of the integer address part.

So, no, this should be size_t or (unsigned) long.

Jess