From nobody Mon Feb 20 08:37:02 2023 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4PKwmx0hNZz3sC24 for ; Mon, 20 Feb 2023 08:38:41 +0000 (UTC) (envelope-from bT.1fxwx6od30=goeikkb00wa3=8c508hg6c4@em790814.fubar.geek.nz) Received: from e2i580.smtp2go.com (e2i580.smtp2go.com [103.2.142.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 4PKwmw10BYz4CBB for ; Mon, 20 Feb 2023 08:38:40 +0000 (UTC) (envelope-from bT.1fxwx6od30=goeikkb00wa3=8c508hg6c4@em790814.fubar.geek.nz) Authentication-Results: mx1.freebsd.org; dkim=none ("invalid DKIM record") header.d=smtpservice.net header.s=mgy720.a1-4.dyn header.b=ZaLuhP9d; dkim=pass header.d=fubar.geek.nz header.s=s790814 header.b=IMZEHnd9; spf=pass (mx1.freebsd.org: domain of "bT.1fxwx6od30=goeikkb00wa3=8c508hg6c4@em790814.fubar.geek.nz" designates 103.2.142.68 as permitted sender) smtp.mailfrom="bT.1fxwx6od30=goeikkb00wa3=8c508hg6c4@em790814.fubar.geek.nz"; dmarc=pass (policy=none) header.from=fubar.geek.nz DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=smtpservice.net; s=mgy720.a1-4.dyn; x=1676883220; h=Feedback-ID: X-Smtpcorp-Track:To:Date:Subject:Message-Id:From:Reply-To:Sender: List-Unsubscribe; bh=amHu9OdxrEC9iyodXPla7AD71gqGAUsEp895wRuiFAA=; b=ZaLuhP9d B3HnL2XmHTxn1iRdiUPcT8ZPs/+gT5ogFtv6FTBp/8S5iQauVIGtgbyHXZLmn68BOA0VDnwuCOCBf 5mSfY94sENWjVx7pWnL/HOxPa4n0yAWRNaIS8onosp31HZmU0UwF1HoN9n2k53fKTDUDOHO2WOJjx Gzy5D/FUMf1lfXNgiG6lYcLFBr4n0pjgGuRg37/MK3lXvOuwL0CszjSnpVWDAYvEfb8K57nx+cddA rpamD8pMup76WyBSHAk6wkeNkkTKOQq0EOQag3HYoOLpEj+EQKW4JNzaB21z77wF8se8sDKq+n71n BVnD5PcTEM42BQVMiCYkUE5jaA==; DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=fubar.geek.nz; i=@fubar.geek.nz; q=dns/txt; s=s790814; t=1676882320; h=from : subject : to : message-id : date; bh=amHu9OdxrEC9iyodXPla7AD71gqGAUsEp895wRuiFAA=; b=IMZEHnd9HVAaimWN88VwYz1z8liAB6CWy8MJY96RCgRjL5y/jkJvYxra2AKjR8AfWdldw ZZDXNcGTMqRGIJnOIG7+/l37nsr8ofc9NfbKrbjJcaKnGylfdbk+AK+ybJjVc+UGN21QEVH ZqzrP1Kc6biqLU0n6Kef8GTkRy2ihV6aT964fQApEmLscaiJ+0TR9ZN3FvJu/qdYbqQLGNW /P6ulpmgvE4iJR/FlxH+ml2+q82uX3ZOByxVYFA5Q5SuA0jP8ZItIWKnGptPPQKL1l3NTuQ LPhYGktse84TQmaudbWzuDCuWWNtLrSz71ozBqyIoR+W6MIbl4HTJpiC1/iw== Received: from [10.176.58.103] (helo=SmtpCorp) by smtpcorp.com with esmtpsa (TLS1.3:ECDHE_SECP256R1__RSA_PSS_RSAE_SHA256__AES_256_GCM:256) (Exim 4.94.2-S2G) (envelope-from ) id 1pU1h0-qt4JL3-Hu; Mon, 20 Feb 2023 08:38:38 +0000 Received: from [10.162.55.164] (helo=morbo.fubar.geek.nz) by smtpcorp.com with esmtpsa (TLS1.3:ECDHE_X25519__RSA_PSS_RSAE_SHA256__AES_256_GCM:256) (Exim 4.96-S2G) (envelope-from ) id 1pU1h0-9ESI49-0v; Mon, 20 Feb 2023 08:38:38 +0000 Received: from smtpclient.apple (cpc91210-cmbg18-2-0-cust37.5-4.cable.virginm.net [81.102.44.38]) by morbo.fubar.geek.nz (Postfix) with ESMTPSA id AE6771D3EA; Mon, 20 Feb 2023 08:38:33 +0000 (UTC) From: Andrew Turner Message-Id: <4754F213-95F0-425D-AB48-E0C694B0C3CB@fubar.geek.nz> Content-Type: multipart/alternative; boundary="Apple-Mail=_2099DA77-5483-4ED9-AF5B-4D930D5143AF" List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.400.51.1.1\)) Subject: Re: git: f29942229d24 - main - Read the arm64 far early in el0 exceptions Date: Mon, 20 Feb 2023 08:37:02 +0000 In-Reply-To: <0E3AAA1E-0B3E-4C4F-A425-CEE13BAE8723@freebsd.org> Cc: "src-committers@freebsd.org" , "dev-commits-src-all@freebsd.org" , "dev-commits-src-main@freebsd.org" To: Jessica Clarke References: <202302021648.312GmSXI049747@gitrepo.freebsd.org> <0E3AAA1E-0B3E-4C4F-A425-CEE13BAE8723@freebsd.org> X-Mailer: Apple Mail (2.3731.400.51.1.1) X-Smtpcorp-Track: 1pl1h09ESm490v.8AGeByCQ74usq Feedback-ID: 790814m:790814amQcrys:790814sWeT5H4KTL X-Report-Abuse: Please forward a copy of this message, including all headers, to X-Spamd-Result: default: False [-2.90 / 15.00]; URI_COUNT_ODD(1.00)[9]; NEURAL_HAM_MEDIUM(-1.00)[-1.000]; NEURAL_HAM_LONG(-1.00)[-1.000]; NEURAL_HAM_SHORT(-1.00)[-1.000]; MV_CASE(0.50)[]; DMARC_POLICY_ALLOW(-0.50)[fubar.geek.nz,none]; RCVD_DKIM_ARC_DNSWL_MED(-0.50)[]; FORGED_SENDER(0.30)[andrew@fubar.geek.nz,bT.1fxwx6od30=goeikkb00wa3=8c508hg6c4@em790814.fubar.geek.nz]; RCVD_IN_DNSWL_MED(-0.20)[103.2.142.68:from]; R_SPF_ALLOW(-0.20)[+ip4:103.2.140.0/22:c]; R_DKIM_ALLOW(-0.20)[fubar.geek.nz:s=s790814]; MIME_GOOD(-0.10)[multipart/alternative,text/plain]; RCPT_COUNT_THREE(0.00)[4]; FROM_HAS_DN(0.00)[]; TO_DN_EQ_ADDR_SOME(0.00)[]; R_DKIM_PERMFAIL(0.00)[smtpservice.net:s=mgy720.a1-4.dyn]; TO_MATCH_ENVRCPT_SOME(0.00)[]; MLMMJ_DEST(0.00)[dev-commits-src-all@FreeBSD.org]; ARC_NA(0.00)[]; MIME_TRACE(0.00)[0:+,1:+,2:~]; MID_RHS_MATCH_FROM(0.00)[]; DKIM_MIXED(0.00)[]; TO_DN_SOME(0.00)[]; RCVD_VIA_SMTP_AUTH(0.00)[]; FROM_NEQ_ENVFROM(0.00)[andrew@fubar.geek.nz,bT.1fxwx6od30=goeikkb00wa3=8c508hg6c4@em790814.fubar.geek.nz]; RCVD_COUNT_THREE(0.00)[4]; RWL_MAILSPIKE_POSSIBLE(0.00)[103.2.142.68:from]; DKIM_TRACE(0.00)[smtpservice.net:~,fubar.geek.nz:+]; ASN(0.00)[asn:23352, ipnet:103.2.140.0/22, country:US]; RCVD_TLS_ALL(0.00)[] X-Rspamd-Queue-Id: 4PKwmw10BYz4CBB X-Spamd-Bar: -- X-ThisMailContainsUnwantedMimeParts: N --Apple-Mail=_2099DA77-5483-4ED9-AF5B-4D930D5143AF Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 (Resend as previous attempt appears to have failed) > On 2 Feb 2023, at 21:00, Jessica Clarke > wrote: >=20 > On 2 Feb 2023, at 16:48, Andrew Turner > wrote: >>=20 >> The branch main has been updated by andrew: >>=20 >> URL: = https://cgit.FreeBSD.org/src/commit/?id=3Df29942229d24ebb8b98f8c5d02f3c863= 2648007e = >>=20 >> commit f29942229d24ebb8b98f8c5d02f3c8632648007e >> Author: Andrew Turner > >> AuthorDate: 2023-01-25 17:47:39 +0000 >> Commit: Andrew Turner > >> CommitDate: 2023-02-02 16:43:15 +0000 >>=20 >> Read the arm64 far early in el0 exceptions >>=20 >> 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. >>=20 >> Fix this be always reading the fault address before dereferencing = the >> current thread pointer. >>=20 >> 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(-) >>=20 >> 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) >>=20 >> 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$"); >>=20 >> /* Called from exception.S */ >> void do_el1h_sync(struct thread *, struct trapframe *); >=20 > 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=E2=80=99m planning on looking into the issue, but I=E2=80=99m unlikely = to have time in the next few weeks. A such I=E2=80=99d prefer to get a = fix for an issue that is being hit and I=E2=80=99m able to reproduce in = now, and to MFC it for 13.2 rather than wait until I have a fix for = both. Andrew= --Apple-Mail=_2099DA77-5483-4ED9-AF5B-4D930D5143AF Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8
(Resend as previous attempt appears = to have failed)

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=3Df29942229d24eb= b8b98f8c5d02f3c8632648007e

commit = f29942229d24ebb8b98f8c5d02f3c8632648007e
Author: =     Andrew Turner <andrew@FreeBSD.org>
AuthorDat= e: 2023-01-25 17:47:39 +0000
Commit:     Andrew = Turner <andrew@FreeBSD.org>
CommitDat= e: 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/D3= 8196
---
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_syn= c)
+ = /*
+  * 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=E2=80=99m planning on looking into = the issue, but I=E2=80=99m unlikely to have time in the next few weeks. = A such I=E2=80=99d prefer to get a fix for an issue that is being hit = and I=E2=80=99m able to reproduce in now, and to MFC it for 13.2 rather = than wait until I have a fix for both.

Andrew
= --Apple-Mail=_2099DA77-5483-4ED9-AF5B-4D930D5143AF--