Date: Mon, 20 Feb 2023 08:37:02 +0000 From: Andrew Turner <andrew@fubar.geek.nz> To: Jessica Clarke <jrtc27@freebsd.org> Cc: "src-committers@freebsd.org" <src-committers@FreeBSD.org>, "dev-commits-src-all@freebsd.org" <dev-commits-src-all@FreeBSD.org>, "dev-commits-src-main@freebsd.org" <dev-commits-src-main@FreeBSD.org> Subject: Re: git: f29942229d24 - main - Read the arm64 far early in el0 exceptions Message-ID: <4754F213-95F0-425D-AB48-E0C694B0C3CB@fubar.geek.nz> In-Reply-To: <0E3AAA1E-0B3E-4C4F-A425-CEE13BAE8723@freebsd.org> References: <202302021648.312GmSXI049747@gitrepo.freebsd.org> <0E3AAA1E-0B3E-4C4F-A425-CEE13BAE8723@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--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 <jrtc27@freebsd.org = <mailto:jrtc27@freebsd.org>> wrote: >=20 > On 2 Feb 2023, at 16:48, Andrew Turner <andrew@FreeBSD.org = <mailto:andrew@FreeBSD.org>> wrote: >>=20 >> The branch main has been updated by andrew: >>=20 >> URL: = https://cgit.FreeBSD.org/src/commit/?id=3Df29942229d24ebb8b98f8c5d02f3c863= 2648007e = <https://cgit.freebsd.org/src/commit/?id=3Df29942229d24ebb8b98f8c5d02f3c86= 32648007e> >>=20 >> 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 >>=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 <html><head><meta charset=3D"UTF-8"><meta http-equiv=3D"content-type" = content=3D"text/html; charset=3Dutf-8"></head><body = style=3D"overflow-wrap: break-word; -webkit-nbsp-mode: space; = line-break: after-white-space;"><div style=3D"font-family: Helvetica; = font-size: 12px; font-style: normal; font-variant-caps: normal; = font-weight: 400; letter-spacing: normal; orphans: auto; text-align: = start; text-indent: 0px; text-transform: none; white-space: normal; = widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; = -webkit-text-stroke-width: 0px; text-decoration: none; caret-color: = rgb(0, 0, 0); color: rgb(0, 0, 0);">(Resend as previous attempt appears = to have failed)<br class=3D"Apple-interchange-newline"><br><blockquote = type=3D"cite"><div>On 2 Feb 2023, at 21:00, Jessica Clarke <<a = href=3D"mailto:jrtc27@freebsd.org">jrtc27@freebsd.org</a>> = wrote:</div><br class=3D"Apple-interchange-newline"><div><span = style=3D"caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: = 12px; font-style: normal; font-variant-caps: normal; font-weight: 400; = letter-spacing: normal; text-align: start; text-indent: 0px; = text-transform: none; white-space: normal; word-spacing: 0px; = -webkit-text-stroke-width: 0px; text-decoration: none; float: none; = display: inline !important;">On 2 Feb 2023, at 16:48, Andrew Turner = <</span><a href=3D"mailto:andrew@FreeBSD.org" style=3D"font-family: = Helvetica; font-size: 12px; font-style: normal; font-variant-caps: = normal; font-weight: 400; letter-spacing: normal; text-align: start; = text-indent: 0px; text-transform: none; white-space: normal; = word-spacing: 0px; -webkit-text-stroke-width: = 0px;">andrew@FreeBSD.org</a><span style=3D"caret-color: rgb(0, 0, 0); = font-family: Helvetica; font-size: 12px; font-style: normal; = font-variant-caps: normal; font-weight: 400; letter-spacing: normal; = text-align: start; text-indent: 0px; text-transform: none; white-space: = normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; = text-decoration: none; float: none; display: inline !important;">> = wrote:</span><br style=3D"caret-color: rgb(0, 0, 0); font-family: = Helvetica; font-size: 12px; font-style: normal; font-variant-caps: = normal; font-weight: 400; letter-spacing: normal; text-align: start; = text-indent: 0px; text-transform: none; white-space: normal; = word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: = none;"><blockquote type=3D"cite" style=3D"font-family: Helvetica; = font-size: 12px; font-style: normal; font-variant-caps: normal; = font-weight: 400; letter-spacing: normal; text-align: start; = text-indent: 0px; text-transform: none; white-space: normal; = word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: = none;"><br>The branch main has been updated by andrew:<br><br>URL:<span = class=3D"Apple-converted-space"> </span><a = href=3D"https://cgit.FreeBSD.org/src/commit/?id=3Df29942229d24ebb8b98f8c5d= 02f3c8632648007e">https://cgit.FreeBSD.org/src/commit/?id=3Df29942229d24eb= b8b98f8c5d02f3c8632648007e</a><br><br>commit = f29942229d24ebb8b98f8c5d02f3c8632648007e<br>Author: = Andrew Turner <<a = href=3D"mailto:andrew@FreeBSD.org">andrew@FreeBSD.org</a>><br>AuthorDat= e: 2023-01-25 17:47:39 +0000<br>Commit: Andrew = Turner <<a = href=3D"mailto:andrew@FreeBSD.org">andrew@FreeBSD.org</a>><br>CommitDat= e: 2023-02-02 16:43:15 +0000<br><br> Read the arm64 far early = in el0 exceptions<br><br> When handling userspace exceptions = on arm64 we need to dereference the<br> current thread = pointer. If this is being promoted/demoted there is = a<br> small window where it will cause another exception to = be hit. As this<br> second exception will set the fault = address register we will read the<br> incorrect value in the = userspace exception handler.<br><br> Fix this be always = reading the fault address before dereferencing = the<br> current thread pointer.<br><br> Reported = by: olivier@<br> Reviewed by: = markj<br> Sponsored by: Arm = Ltd<br> Differential Revision: <a = href=3D"https://reviews.freebsd.org/D38196">https://reviews.freebsd.org/D3= 8196</a><br>---<br>sys/arm64/arm64/exception.S | 15 = +++++++++++++++<br>sys/arm64/arm64/trap.c = | 26 +++++++-------------------<br>2 files = changed, 22 insertions(+), 19 deletions(-)<br><br>diff --git = a/sys/arm64/arm64/exception.S b/sys/arm64/arm64/exception.S<br>index = 4a74358afeb9..55bac5e5228a 100644<br>--- = a/sys/arm64/arm64/exception.S<br>+++ b/sys/arm64/arm64/exception.S<br>@@ = -212,10 +212,25 @@ = ENTRY(handle_el1h_irq)<br>END(handle_el1h_irq)<br><br>ENTRY(handle_el0_syn= c)<br>+<span class=3D"Apple-tab-span" style=3D"white-space: pre;"> = </span>/*<br>+<span class=3D"Apple-tab-span" style=3D"white-space: = pre;"> </span><span class=3D"Apple-converted-space"> </span>* Read = the fault address early. The current thread structure may<br>+<span = class=3D"Apple-tab-span" style=3D"white-space: pre;"> </span><span = class=3D"Apple-converted-space"> </span>* be transiently unmapped = if it is part of a memory range being<br>+<span class=3D"Apple-tab-span" = style=3D"white-space: pre;"> </span><span = class=3D"Apple-converted-space"> </span>* promoted or demoted = to/from a superpage. As this involves a<br>+<span class=3D"Apple-tab-span"= style=3D"white-space: pre;"> </span><span = class=3D"Apple-converted-space"> </span>* break-before-make = sequence there is a short period of time where<br>+<span = class=3D"Apple-tab-span" style=3D"white-space: pre;"> </span><span = class=3D"Apple-converted-space"> </span>* an access will raise an = exception. If this happens the fault<br>+<span class=3D"Apple-tab-span" = style=3D"white-space: pre;"> </span><span = class=3D"Apple-converted-space"> </span>* address will be changed = to the kernel address so a later read of<br>+<span = class=3D"Apple-tab-span" style=3D"white-space: pre;"> </span><span = class=3D"Apple-converted-space"> </span>* far_el1 will give the = wrong value.<br>+<span class=3D"Apple-tab-span" style=3D"white-space: = pre;"> </span><span = class=3D"Apple-converted-space"> </span>*<br>+<span = class=3D"Apple-tab-span" style=3D"white-space: pre;"> </span><span = class=3D"Apple-converted-space"> </span>* The earliest memory = access that could trigger a fault is in a<br>+<span = class=3D"Apple-tab-span" style=3D"white-space: pre;"> </span><span = class=3D"Apple-converted-space"> </span>* function called by the = save_registers macro so this is the latest<br>+<span = class=3D"Apple-tab-span" style=3D"white-space: pre;"> </span><span = class=3D"Apple-converted-space"> </span>* we can read the userspace = value.<br>+<span class=3D"Apple-tab-span" style=3D"white-space: pre;"> = </span><span class=3D"Apple-converted-space"> </span>*/<br>+<span = class=3D"Apple-tab-span" style=3D"white-space: pre;"> </span>mrs<span = class=3D"Apple-tab-span" style=3D"white-space: pre;"> </span>x19, = far_el1<br><span class=3D"Apple-tab-span" style=3D"white-space: pre;"> = </span>save_registers 0<br><span class=3D"Apple-tab-span" = style=3D"white-space: pre;"> </span>ldr<span class=3D"Apple-tab-span" = style=3D"white-space: pre;"> </span>x0, [x18, #PC_CURTHREAD]<br><span = class=3D"Apple-tab-span" style=3D"white-space: pre;"> </span>mov<span = class=3D"Apple-tab-span" style=3D"white-space: pre;"> </span>x1, = sp<br><span class=3D"Apple-tab-span" style=3D"white-space: pre;"> = </span>str<span class=3D"Apple-tab-span" style=3D"white-space: pre;"> = </span>x1, [x0, #TD_FRAME]<br>+<span class=3D"Apple-tab-span" = style=3D"white-space: pre;"> </span>mov<span class=3D"Apple-tab-span" = style=3D"white-space: pre;"> </span>x2, x19<br><span = class=3D"Apple-tab-span" style=3D"white-space: pre;"> </span>bl<span = class=3D"Apple-tab-span" style=3D"white-space: pre;"> = </span>do_el0_sync<br><span class=3D"Apple-tab-span" style=3D"white-space:= pre;"> </span>do_ast<br><span class=3D"Apple-tab-span" = style=3D"white-space: pre;"> </span>restore_registers 0<br>diff --git = a/sys/arm64/arm64/trap.c b/sys/arm64/arm64/trap.c<br>index = 4e54a06548cc..1b33d7aa60c4 100644<br>--- a/sys/arm64/arm64/trap.c<br>+++ = b/sys/arm64/arm64/trap.c<br>@@ -76,7 +76,7 @@ = __FBSDID("$FreeBSD$");<br><br>/* Called from exception.S */<br>void = do_el1h_sync(struct thread *, struct trapframe *);<br></blockquote><br = style=3D"caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: = 12px; font-style: normal; font-variant-caps: normal; font-weight: 400; = letter-spacing: normal; text-align: start; text-indent: 0px; = text-transform: none; white-space: normal; word-spacing: 0px; = -webkit-text-stroke-width: 0px; text-decoration: none;"><span = style=3D"caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: = 12px; font-style: normal; font-variant-caps: normal; font-weight: 400; = letter-spacing: normal; text-align: start; text-indent: 0px; = text-transform: none; white-space: normal; word-spacing: 0px; = -webkit-text-stroke-width: 0px; text-decoration: none; float: none; = display: inline !important;">This did not address my feedback regarding = EL1 debug exceptions also</span><br style=3D"caret-color: rgb(0, 0, 0); = font-family: Helvetica; font-size: 12px; font-style: normal; = font-variant-caps: normal; font-weight: 400; letter-spacing: normal; = text-align: start; text-indent: 0px; text-transform: none; white-space: = normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; = text-decoration: none;"><span style=3D"caret-color: rgb(0, 0, 0); = font-family: Helvetica; font-size: 12px; font-style: normal; = font-variant-caps: normal; font-weight: 400; letter-spacing: normal; = text-align: start; text-indent: 0px; text-transform: none; white-space: = normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; = text-decoration: none; float: none; display: inline = !important;">clobbering FAR.</span><br style=3D"caret-color: rgb(0, 0, = 0); font-family: Helvetica; font-size: 12px; font-style: normal; = font-variant-caps: normal; font-weight: 400; letter-spacing: normal; = text-align: start; text-indent: 0px; text-transform: none; white-space: = normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; = text-decoration: none;"></div></blockquote></div><br style=3D"font-family:= Helvetica; font-size: 12px; font-style: normal; font-variant-caps: = normal; font-weight: 400; letter-spacing: normal; orphans: auto; = text-align: start; text-indent: 0px; text-transform: none; white-space: = normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; = -webkit-text-stroke-width: 0px; text-decoration: none; caret-color: = rgb(0, 0, 0); color: rgb(0, 0, 0);"><div style=3D"font-family: = Helvetica; font-size: 12px; font-style: normal; font-variant-caps: = normal; font-weight: 400; letter-spacing: normal; orphans: auto; = text-align: start; text-indent: 0px; text-transform: none; white-space: = normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; = -webkit-text-stroke-width: 0px; text-decoration: none; caret-color: = rgb(0, 0, 0); color: rgb(0, 0, 0);">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.</div><div style=3D"font-family: = Helvetica; font-size: 12px; font-style: normal; font-variant-caps: = normal; font-weight: 400; letter-spacing: normal; orphans: auto; = text-align: start; text-indent: 0px; text-transform: none; white-space: = normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; = -webkit-text-stroke-width: 0px; text-decoration: none; caret-color: = rgb(0, 0, 0); color: rgb(0, 0, 0);"><br></div><div style=3D"font-family: = Helvetica; font-size: 12px; font-style: normal; font-variant-caps: = normal; font-weight: 400; letter-spacing: normal; orphans: auto; = text-align: start; text-indent: 0px; text-transform: none; white-space: = normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; = -webkit-text-stroke-width: 0px; text-decoration: none; caret-color: = rgb(0, 0, 0); color: rgb(0, 0, 0);">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.</div><div style=3D"font-family: = Helvetica; font-size: 12px; font-style: normal; font-variant-caps: = normal; font-weight: 400; letter-spacing: normal; orphans: auto; = text-align: start; text-indent: 0px; text-transform: none; white-space: = normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; = -webkit-text-stroke-width: 0px; text-decoration: none; caret-color: = rgb(0, 0, 0); color: rgb(0, 0, 0);"><br></div><div style=3D"font-family: = Helvetica; font-size: 12px; font-style: normal; font-variant-caps: = normal; font-weight: 400; letter-spacing: normal; orphans: auto; = text-align: start; text-indent: 0px; text-transform: none; white-space: = normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; = -webkit-text-stroke-width: 0px; text-decoration: none; caret-color: = rgb(0, 0, 0); color: rgb(0, 0, 0);">Andrew</div></body></html>= --Apple-Mail=_2099DA77-5483-4ED9-AF5B-4D930D5143AF--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4754F213-95F0-425D-AB48-E0C694B0C3CB>