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

(Resend as previous attempt appears to have failed)

> On 2 Feb 2023, at 21:00, Jessica Clarke <jrtc27@freebsd.org <mailto:jrtc27@freebsd.org>> wrote:
> 
> On 2 Feb 2023, at 16:48, 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=f29942229d24ebb8b98f8c5d02f3c8632648007e <https://cgit.freebsd.org/src/commit/?id=f29942229d24ebb8b98f8c5d02f3c8632648007e>
>> 
>> commit f29942229d24ebb8b98f8c5d02f3c8632648007e
>> Author:     Andrew Turner <andrew@FreeBSD.org <mailto:andrew@FreeBSD.org>>
>> AuthorDate: 2023-01-25 17:47:39 +0000
>> Commit:     Andrew Turner <andrew@FreeBSD.org <mailto: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.

That is outside of the scope of what I was trying to fix in this change. Having recently talked with one of the Linux Arm kernel developers about exception handlers raising debug exceptions I believe there are subtle issues and moving saving the far register earlier is not enough.

I’m planning on looking into the issue, but I’m unlikely to have time in the next few weeks. A such I’d prefer to get a fix for an issue that is being hit and I’m able to reproduce in now, and to MFC it for 13.2 rather than wait until I have a fix for both.

Andrew