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

> 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 <mailto: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.

Andrew