Re: git: f29942229d24 - main - Read the arm64 far early in el0 exceptions

On 2 Feb 2023, at 21:00, Jessica Clarke <jrtc27@FreeBSD.org> wrote:
> 
> On 2 Feb 2023, at 16:48, Andrew Turner <andrew@FreeBSD.org> wrote:
>> 
>> The branch main has been updated by andrew:
>> 
>> URL: https://cgit.FreeBSD.org/src/commit/?id=f29942229d24ebb8b98f8c5d02f3c8632648007e
>> 
>> commit f29942229d24ebb8b98f8c5d02f3c8632648007e
>> Author:     Andrew Turner <andrew@FreeBSD.org>
>> AuthorDate: 2023-01-25 17:47:39 +0000
>> Commit:     Andrew Turner <andrew@FreeBSD.org>
>> CommitDate: 2023-02-02 16:43:15 +0000
>> 
>>   Read the arm64 far early in el0 exceptions
>> 
>>   When handling userspace exceptions on arm64 we need to dereference the
>>   current thread pointer. If this is being promoted/demoted there is a
>>   small window where it will cause another exception to be hit. As this
>>   second exception will set the fault address register we will read the
>>   incorrect value in the userspace exception handler.
>> 
>>   Fix this be always reading the fault address before dereferencing the
>>   current thread pointer.
>> 
>>   Reported by:    olivier@
>>   Reviewed by:    markj
>>   Sponsored by:   Arm Ltd
>>   Differential Revision:  https://reviews.freebsd.org/D38196
>> ---
>> sys/arm64/arm64/exception.S | 15 +++++++++++++++
>> sys/arm64/arm64/trap.c      | 26 +++++++-------------------
>> 2 files changed, 22 insertions(+), 19 deletions(-)
>> 
>> diff --git a/sys/arm64/arm64/exception.S b/sys/arm64/arm64/exception.S
>> index 4a74358afeb9..55bac5e5228a 100644
>> --- a/sys/arm64/arm64/exception.S
>> +++ b/sys/arm64/arm64/exception.S
>> @@ -212,10 +212,25 @@ ENTRY(handle_el1h_irq)
>> END(handle_el1h_irq)
>> 
>> ENTRY(handle_el0_sync)
>> +	/*
>> +	 * Read the fault address early. The current thread structure may
>> +	 * be transiently unmapped if it is part of a memory range being
>> +	 * promoted or demoted to/from a superpage. As this involves a
>> +	 * break-before-make sequence there is a short period of time where
>> +	 * an access will raise an exception. If this happens the fault
>> +	 * address will be changed to the kernel address so a later read of
>> +	 * far_el1 will give the wrong value.
>> +	 *
>> +	 * The earliest memory access that could trigger a fault is in a
>> +	 * function called by the save_registers macro so this is the latest
>> +	 * we can read the userspace value.
>> +	 */
>> +	mrs	x19, far_el1
>> 	save_registers 0
>> 	ldr	x0, [x18, #PC_CURTHREAD]
>> 	mov	x1, sp
>> 	str	x1, [x0, #TD_FRAME]
>> +	mov	x2, x19
>> 	bl	do_el0_sync
>> 	do_ast
>> 	restore_registers 0
>> diff --git a/sys/arm64/arm64/trap.c b/sys/arm64/arm64/trap.c
>> index 4e54a06548cc..1b33d7aa60c4 100644
>> --- a/sys/arm64/arm64/trap.c
>> +++ b/sys/arm64/arm64/trap.c
>> @@ -76,7 +76,7 @@ __FBSDID("$FreeBSD$");
>> 
>> /* Called from exception.S */
>> void do_el1h_sync(struct thread *, struct trapframe *);
> 
> This did not address my feedback regarding EL1 debug exceptions also
> clobbering FAR.

Ping, now after this has been MFC’ed without so much as a reply to my
feedback here nor on the Phabricator review.

Jess

>> -void do_el0_sync(struct thread *, struct trapframe *);
>> +void do_el0_sync(struct thread *, struct trapframe *, uint64_t far);
>> void do_el0_error(struct trapframe *);
>> void do_serror(struct trapframe *);
>> void unhandled_exception(struct trapframe *);
>> @@ -559,11 +559,11 @@ do_el1h_sync(struct thread *td, struct trapframe *frame)
>> }
>> 
>> void
>> -do_el0_sync(struct thread *td, struct trapframe *frame)
>> +do_el0_sync(struct thread *td, struct trapframe *frame, uint64_t far)
>> {
>> 	pcpu_bp_harden bp_harden;
>> 	uint32_t exception;
>> -	uint64_t esr, far;
>> +	uint64_t esr;
>> 	int dfsc;
>> 
>> 	/* Check we have a sane environment when entering from userland */
>> @@ -573,27 +573,15 @@ do_el0_sync(struct thread *td, struct trapframe *frame)
>> 
>> 	esr = frame->tf_esr;
>> 	exception = ESR_ELx_EXCEPTION(esr);
>> -	switch (exception) {
>> -	case EXCP_INSN_ABORT_L:
>> -		far = READ_SPECIALREG(far_el1);
>> -
>> +	if (exception == EXCP_INSN_ABORT_L && far > VM_MAXUSER_ADDRESS) {
>> 		/*
>> 		 * Userspace may be trying to train the branch predictor to
>> 		 * attack the kernel. If we are on a CPU affected by this
>> 		 * call the handler to clear the branch predictor state.
>> 		 */
>> -		if (far > VM_MAXUSER_ADDRESS) {
>> -			bp_harden = PCPU_GET(bp_harden);
>> -			if (bp_harden != NULL)
>> -				bp_harden();
>> -		}
>> -		break;
>> -	case EXCP_UNKNOWN:
>> -	case EXCP_DATA_ABORT_L:
>> -	case EXCP_DATA_ABORT:
>> -	case EXCP_WATCHPT_EL0:
>> -		far = READ_SPECIALREG(far_el1);
>> -		break;
>> +		bp_harden = PCPU_GET(bp_harden);
>> +		if (bp_harden != NULL)
>> +			bp_harden();
>> 	}
>> 	intr_enable();