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

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.

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();
>