* [PATCH] x86/traps: 'Fix' safety of read_registers() in #DF path
@ 2020-10-12 13:49 Andrew Cooper
2020-10-13 15:51 ` Jan Beulich
0 siblings, 1 reply; 10+ messages in thread
From: Andrew Cooper @ 2020-10-12 13:49 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné, Wei Liu, Julien Grall
All interrupts and exceptions pass a struct cpu_user_regs up into C. This
contains the legacy vm86 fields from 32bit days, which are beyond the
hardware-pushed frame.
Accessing these fields is generally illegal, as they are logically out of
bounds for anything other than an interrupt/exception hitting ring1/3 code.
Unfortunately, the #DF handler uses these fields as part of preparing the
state dump, and being IST, accesses the adjacent stack frame.
This has been broken forever, but c/s 6001660473 "x86/shstk: Rework the stack
layout to support shadow stacks" repositioned the #DF stack to be adjacent to
the guard page, which turns this OoB write into a fatal pagefault:
(XEN) *** DOUBLE FAULT ***
(XEN) ----[ Xen-4.15-unstable x86_64 debug=y Tainted: C ]----
(XEN) ----[ Xen-4.15-unstable x86_64 debug=y Tainted: C ]----
(XEN) CPU: 4
(XEN) RIP: e008:[<ffff82d04031fd4f>] traps.c#read_registers+0x29/0xc1
(XEN) RFLAGS: 0000000000050086 CONTEXT: hypervisor (d1v0)
...
(XEN) Xen call trace:
(XEN) [<ffff82d04031fd4f>] R traps.c#read_registers+0x29/0xc1
(XEN) [<ffff82d0403207b3>] F do_double_fault+0x3d/0x7e
(XEN) [<ffff82d04039acd7>] F double_fault+0x107/0x110
(XEN)
(XEN) Pagetable walk from ffff830236f6d008:
(XEN) L4[0x106] = 80000000bfa9b063 ffffffffffffffff
(XEN) L3[0x008] = 0000000236ffd063 ffffffffffffffff
(XEN) L2[0x1b7] = 0000000236ffc063 ffffffffffffffff
(XEN) L1[0x16d] = 8000000236f6d161 ffffffffffffffff
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 4:
(XEN) FATAL PAGE FAULT
(XEN) [error_code=0003]
(XEN) Faulting linear address: ffff830236f6d008
(XEN) ****************************************
(XEN)
and rendering the main #DF analysis broken.
The proper fix is to delete cpu_user_regs.es and later, so no
interrupt/exception path can access OoB, but this needs disentangling from the
PV ABI first.
Not-really-fixes: 6001660473 ("x86/shstk: Rework the stack layout to support shadow stacks")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Julien Grall <julien@xen.org>
---
xen/arch/x86/cpu/common.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index da74172776..a684519a20 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -770,7 +770,13 @@ void load_system_tables(void)
tss->ist[IST_MCE - 1] = stack_top + (1 + IST_MCE) * PAGE_SIZE;
tss->ist[IST_NMI - 1] = stack_top + (1 + IST_NMI) * PAGE_SIZE;
tss->ist[IST_DB - 1] = stack_top + (1 + IST_DB) * PAGE_SIZE;
- tss->ist[IST_DF - 1] = stack_top + (1 + IST_DF) * PAGE_SIZE;
+ /*
+ * Gross bodge. The #DF handler uses the vm86 fields of cpu_user_regs
+ * beyond the hardware frame. Adjust the stack entrypoint so this
+ * doesn't manifest as an OoB write which hits the guard page.
+ */
+ tss->ist[IST_DF - 1] = stack_top + (1 + IST_DF) * PAGE_SIZE -
+ (sizeof(struct cpu_user_regs) - offsetof(struct cpu_user_regs, es));
tss->bitmap = IOBMP_INVALID_OFFSET;
/* All other stack pointers poisioned. */
--
2.11.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] x86/traps: 'Fix' safety of read_registers() in #DF path 2020-10-12 13:49 [PATCH] x86/traps: 'Fix' safety of read_registers() in #DF path Andrew Cooper @ 2020-10-13 15:51 ` Jan Beulich 2020-10-14 18:00 ` Andrew Cooper 0 siblings, 1 reply; 10+ messages in thread From: Jan Beulich @ 2020-10-13 15:51 UTC (permalink / raw) To: Andrew Cooper; +Cc: Xen-devel, Roger Pau Monné, Wei Liu, Julien Grall On 12.10.2020 15:49, Andrew Cooper wrote: > All interrupts and exceptions pass a struct cpu_user_regs up into C. This > contains the legacy vm86 fields from 32bit days, which are beyond the > hardware-pushed frame. > > Accessing these fields is generally illegal, as they are logically out of > bounds for anything other than an interrupt/exception hitting ring1/3 code. > > Unfortunately, the #DF handler uses these fields as part of preparing the > state dump, and being IST, accesses the adjacent stack frame. > > This has been broken forever, but c/s 6001660473 "x86/shstk: Rework the stack > layout to support shadow stacks" repositioned the #DF stack to be adjacent to > the guard page, which turns this OoB write into a fatal pagefault: > > (XEN) *** DOUBLE FAULT *** > (XEN) ----[ Xen-4.15-unstable x86_64 debug=y Tainted: C ]---- > (XEN) ----[ Xen-4.15-unstable x86_64 debug=y Tainted: C ]---- > (XEN) CPU: 4 > (XEN) RIP: e008:[<ffff82d04031fd4f>] traps.c#read_registers+0x29/0xc1 > (XEN) RFLAGS: 0000000000050086 CONTEXT: hypervisor (d1v0) > ... > (XEN) Xen call trace: > (XEN) [<ffff82d04031fd4f>] R traps.c#read_registers+0x29/0xc1 > (XEN) [<ffff82d0403207b3>] F do_double_fault+0x3d/0x7e > (XEN) [<ffff82d04039acd7>] F double_fault+0x107/0x110 > (XEN) > (XEN) Pagetable walk from ffff830236f6d008: > (XEN) L4[0x106] = 80000000bfa9b063 ffffffffffffffff > (XEN) L3[0x008] = 0000000236ffd063 ffffffffffffffff > (XEN) L2[0x1b7] = 0000000236ffc063 ffffffffffffffff > (XEN) L1[0x16d] = 8000000236f6d161 ffffffffffffffff > (XEN) > (XEN) **************************************** > (XEN) Panic on CPU 4: > (XEN) FATAL PAGE FAULT > (XEN) [error_code=0003] > (XEN) Faulting linear address: ffff830236f6d008 > (XEN) **************************************** > (XEN) > > and rendering the main #DF analysis broken. > > The proper fix is to delete cpu_user_regs.es and later, so no > interrupt/exception path can access OoB, but this needs disentangling from the > PV ABI first. > > Not-really-fixes: 6001660473 ("x86/shstk: Rework the stack layout to support shadow stacks") > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> Is it perhaps worth also saying explicitly that the other IST stacks don't suffer the same problem because show_registers() makes an local copy of the passed in struct? (Doing so also for #DF would apparently be an alternative solution.) Jan ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86/traps: 'Fix' safety of read_registers() in #DF path 2020-10-13 15:51 ` Jan Beulich @ 2020-10-14 18:00 ` Andrew Cooper 2020-10-15 7:27 ` Jan Beulich 0 siblings, 1 reply; 10+ messages in thread From: Andrew Cooper @ 2020-10-14 18:00 UTC (permalink / raw) To: Jan Beulich; +Cc: Xen-devel, Roger Pau Monné, Wei Liu, Julien Grall On 13/10/2020 16:51, Jan Beulich wrote: > On 12.10.2020 15:49, Andrew Cooper wrote: >> All interrupts and exceptions pass a struct cpu_user_regs up into C. This >> contains the legacy vm86 fields from 32bit days, which are beyond the >> hardware-pushed frame. >> >> Accessing these fields is generally illegal, as they are logically out of >> bounds for anything other than an interrupt/exception hitting ring1/3 code. >> >> Unfortunately, the #DF handler uses these fields as part of preparing the >> state dump, and being IST, accesses the adjacent stack frame. >> >> This has been broken forever, but c/s 6001660473 "x86/shstk: Rework the stack >> layout to support shadow stacks" repositioned the #DF stack to be adjacent to >> the guard page, which turns this OoB write into a fatal pagefault: >> >> (XEN) *** DOUBLE FAULT *** >> (XEN) ----[ Xen-4.15-unstable x86_64 debug=y Tainted: C ]---- >> (XEN) ----[ Xen-4.15-unstable x86_64 debug=y Tainted: C ]---- >> (XEN) CPU: 4 >> (XEN) RIP: e008:[<ffff82d04031fd4f>] traps.c#read_registers+0x29/0xc1 >> (XEN) RFLAGS: 0000000000050086 CONTEXT: hypervisor (d1v0) >> ... >> (XEN) Xen call trace: >> (XEN) [<ffff82d04031fd4f>] R traps.c#read_registers+0x29/0xc1 >> (XEN) [<ffff82d0403207b3>] F do_double_fault+0x3d/0x7e >> (XEN) [<ffff82d04039acd7>] F double_fault+0x107/0x110 >> (XEN) >> (XEN) Pagetable walk from ffff830236f6d008: >> (XEN) L4[0x106] = 80000000bfa9b063 ffffffffffffffff >> (XEN) L3[0x008] = 0000000236ffd063 ffffffffffffffff >> (XEN) L2[0x1b7] = 0000000236ffc063 ffffffffffffffff >> (XEN) L1[0x16d] = 8000000236f6d161 ffffffffffffffff >> (XEN) >> (XEN) **************************************** >> (XEN) Panic on CPU 4: >> (XEN) FATAL PAGE FAULT >> (XEN) [error_code=0003] >> (XEN) Faulting linear address: ffff830236f6d008 >> (XEN) **************************************** >> (XEN) >> >> and rendering the main #DF analysis broken. >> >> The proper fix is to delete cpu_user_regs.es and later, so no >> interrupt/exception path can access OoB, but this needs disentangling from the >> PV ABI first. >> >> Not-really-fixes: 6001660473 ("x86/shstk: Rework the stack layout to support shadow stacks") >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> > > Is it perhaps worth also saying explicitly that the other IST > stacks don't suffer the same problem because show_registers() > makes an local copy of the passed in struct? (Doing so also > for #DF would apparently be an alternative solution.) They're not safe. They merely don't explode. https://lore.kernel.org/xen-devel/1532546157-5974-1-git-send-email-andrew.cooper3@citrix.com/ was "fixed" by CET-SS turning the guard page from not present to read-only, but the same CET-SS series swapped #DB for #DF when it comes to the single OoB write problem case. ~Andrew ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86/traps: 'Fix' safety of read_registers() in #DF path 2020-10-14 18:00 ` Andrew Cooper @ 2020-10-15 7:27 ` Jan Beulich 2020-10-16 10:58 ` Andrew Cooper 0 siblings, 1 reply; 10+ messages in thread From: Jan Beulich @ 2020-10-15 7:27 UTC (permalink / raw) To: Andrew Cooper; +Cc: Xen-devel, Roger Pau Monné, Wei Liu, Julien Grall On 14.10.2020 20:00, Andrew Cooper wrote: > On 13/10/2020 16:51, Jan Beulich wrote: >> On 12.10.2020 15:49, Andrew Cooper wrote: >>> All interrupts and exceptions pass a struct cpu_user_regs up into C. This >>> contains the legacy vm86 fields from 32bit days, which are beyond the >>> hardware-pushed frame. >>> >>> Accessing these fields is generally illegal, as they are logically out of >>> bounds for anything other than an interrupt/exception hitting ring1/3 code. >>> >>> Unfortunately, the #DF handler uses these fields as part of preparing the >>> state dump, and being IST, accesses the adjacent stack frame. >>> >>> This has been broken forever, but c/s 6001660473 "x86/shstk: Rework the stack >>> layout to support shadow stacks" repositioned the #DF stack to be adjacent to >>> the guard page, which turns this OoB write into a fatal pagefault: >>> >>> (XEN) *** DOUBLE FAULT *** >>> (XEN) ----[ Xen-4.15-unstable x86_64 debug=y Tainted: C ]---- >>> (XEN) ----[ Xen-4.15-unstable x86_64 debug=y Tainted: C ]---- >>> (XEN) CPU: 4 >>> (XEN) RIP: e008:[<ffff82d04031fd4f>] traps.c#read_registers+0x29/0xc1 >>> (XEN) RFLAGS: 0000000000050086 CONTEXT: hypervisor (d1v0) >>> ... >>> (XEN) Xen call trace: >>> (XEN) [<ffff82d04031fd4f>] R traps.c#read_registers+0x29/0xc1 >>> (XEN) [<ffff82d0403207b3>] F do_double_fault+0x3d/0x7e >>> (XEN) [<ffff82d04039acd7>] F double_fault+0x107/0x110 >>> (XEN) >>> (XEN) Pagetable walk from ffff830236f6d008: >>> (XEN) L4[0x106] = 80000000bfa9b063 ffffffffffffffff >>> (XEN) L3[0x008] = 0000000236ffd063 ffffffffffffffff >>> (XEN) L2[0x1b7] = 0000000236ffc063 ffffffffffffffff >>> (XEN) L1[0x16d] = 8000000236f6d161 ffffffffffffffff >>> (XEN) >>> (XEN) **************************************** >>> (XEN) Panic on CPU 4: >>> (XEN) FATAL PAGE FAULT >>> (XEN) [error_code=0003] >>> (XEN) Faulting linear address: ffff830236f6d008 >>> (XEN) **************************************** >>> (XEN) >>> >>> and rendering the main #DF analysis broken. >>> >>> The proper fix is to delete cpu_user_regs.es and later, so no >>> interrupt/exception path can access OoB, but this needs disentangling from the >>> PV ABI first. >>> >>> Not-really-fixes: 6001660473 ("x86/shstk: Rework the stack layout to support shadow stacks") >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> Reviewed-by: Jan Beulich <jbeulich@suse.com> >> >> Is it perhaps worth also saying explicitly that the other IST >> stacks don't suffer the same problem because show_registers() >> makes an local copy of the passed in struct? (Doing so also >> for #DF would apparently be an alternative solution.) > > They're not safe. They merely don't explode. > > https://lore.kernel.org/xen-devel/1532546157-5974-1-git-send-email-andrew.cooper3@citrix.com/ > was "fixed" by CET-SS turning the guard page from not present to > read-only, but the same CET-SS series swapped #DB for #DF when it comes > to the single OoB write problem case. I see. While indeed I didn't pay attention to the OoB read aspect, me saying "the other IST stacks don't suffer the same problem" was still correct, wasn't it? Anyway - not a big deal. Jan ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86/traps: 'Fix' safety of read_registers() in #DF path 2020-10-15 7:27 ` Jan Beulich @ 2020-10-16 10:58 ` Andrew Cooper 2020-10-16 11:03 ` Jan Beulich 0 siblings, 1 reply; 10+ messages in thread From: Andrew Cooper @ 2020-10-16 10:58 UTC (permalink / raw) To: Jan Beulich; +Cc: Xen-devel, Roger Pau Monné, Wei Liu, Julien Grall On 15/10/2020 08:27, Jan Beulich wrote: > On 14.10.2020 20:00, Andrew Cooper wrote: >> On 13/10/2020 16:51, Jan Beulich wrote: >>> On 12.10.2020 15:49, Andrew Cooper wrote: >>>> All interrupts and exceptions pass a struct cpu_user_regs up into C. This >>>> contains the legacy vm86 fields from 32bit days, which are beyond the >>>> hardware-pushed frame. >>>> >>>> Accessing these fields is generally illegal, as they are logically out of >>>> bounds for anything other than an interrupt/exception hitting ring1/3 code. >>>> >>>> Unfortunately, the #DF handler uses these fields as part of preparing the >>>> state dump, and being IST, accesses the adjacent stack frame. >>>> >>>> This has been broken forever, but c/s 6001660473 "x86/shstk: Rework the stack >>>> layout to support shadow stacks" repositioned the #DF stack to be adjacent to >>>> the guard page, which turns this OoB write into a fatal pagefault: >>>> >>>> (XEN) *** DOUBLE FAULT *** >>>> (XEN) ----[ Xen-4.15-unstable x86_64 debug=y Tainted: C ]---- >>>> (XEN) ----[ Xen-4.15-unstable x86_64 debug=y Tainted: C ]---- >>>> (XEN) CPU: 4 >>>> (XEN) RIP: e008:[<ffff82d04031fd4f>] traps.c#read_registers+0x29/0xc1 >>>> (XEN) RFLAGS: 0000000000050086 CONTEXT: hypervisor (d1v0) >>>> ... >>>> (XEN) Xen call trace: >>>> (XEN) [<ffff82d04031fd4f>] R traps.c#read_registers+0x29/0xc1 >>>> (XEN) [<ffff82d0403207b3>] F do_double_fault+0x3d/0x7e >>>> (XEN) [<ffff82d04039acd7>] F double_fault+0x107/0x110 >>>> (XEN) >>>> (XEN) Pagetable walk from ffff830236f6d008: >>>> (XEN) L4[0x106] = 80000000bfa9b063 ffffffffffffffff >>>> (XEN) L3[0x008] = 0000000236ffd063 ffffffffffffffff >>>> (XEN) L2[0x1b7] = 0000000236ffc063 ffffffffffffffff >>>> (XEN) L1[0x16d] = 8000000236f6d161 ffffffffffffffff >>>> (XEN) >>>> (XEN) **************************************** >>>> (XEN) Panic on CPU 4: >>>> (XEN) FATAL PAGE FAULT >>>> (XEN) [error_code=0003] >>>> (XEN) Faulting linear address: ffff830236f6d008 >>>> (XEN) **************************************** >>>> (XEN) >>>> >>>> and rendering the main #DF analysis broken. >>>> >>>> The proper fix is to delete cpu_user_regs.es and later, so no >>>> interrupt/exception path can access OoB, but this needs disentangling from the >>>> PV ABI first. >>>> >>>> Not-really-fixes: 6001660473 ("x86/shstk: Rework the stack layout to support shadow stacks") >>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >>> >>> Is it perhaps worth also saying explicitly that the other IST >>> stacks don't suffer the same problem because show_registers() >>> makes an local copy of the passed in struct? (Doing so also >>> for #DF would apparently be an alternative solution.) >> They're not safe. They merely don't explode. >> >> https://lore.kernel.org/xen-devel/1532546157-5974-1-git-send-email-andrew.cooper3@citrix.com/ >> was "fixed" by CET-SS turning the guard page from not present to >> read-only, but the same CET-SS series swapped #DB for #DF when it comes >> to the single OoB write problem case. > I see. While indeed I didn't pay attention to the OoB read aspect, > me saying "the other IST stacks don't suffer the same problem" was > still correct, wasn't it? Anyway - not a big deal. I've tweaked the commit message to make this more clear. --8<--- Accessing these fields is generally illegal, as they are logically out of bounds for anything other than an interrupt/exception hitting ring1/3 code. show_registers() unconditionally reads these fields, but the content is discarded before use. This is benign right now, as all parts of the stack are readable, including the guard pages. However, read_registers() in the #DF handler writes to these fields as part of preparing the state dump, and being IST, hits the adjacent stack frame. --8<-- ~Andrew ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86/traps: 'Fix' safety of read_registers() in #DF path 2020-10-16 10:58 ` Andrew Cooper @ 2020-10-16 11:03 ` Jan Beulich 2020-10-16 11:24 ` Andrew Cooper 0 siblings, 1 reply; 10+ messages in thread From: Jan Beulich @ 2020-10-16 11:03 UTC (permalink / raw) To: Andrew Cooper; +Cc: Xen-devel, Roger Pau Monné, Wei Liu, Julien Grall On 16.10.2020 12:58, Andrew Cooper wrote: > On 15/10/2020 08:27, Jan Beulich wrote: >> On 14.10.2020 20:00, Andrew Cooper wrote: >>> On 13/10/2020 16:51, Jan Beulich wrote: >>>> On 12.10.2020 15:49, Andrew Cooper wrote: >>>>> All interrupts and exceptions pass a struct cpu_user_regs up into C. This >>>>> contains the legacy vm86 fields from 32bit days, which are beyond the >>>>> hardware-pushed frame. >>>>> >>>>> Accessing these fields is generally illegal, as they are logically out of >>>>> bounds for anything other than an interrupt/exception hitting ring1/3 code. >>>>> >>>>> Unfortunately, the #DF handler uses these fields as part of preparing the >>>>> state dump, and being IST, accesses the adjacent stack frame. >>>>> >>>>> This has been broken forever, but c/s 6001660473 "x86/shstk: Rework the stack >>>>> layout to support shadow stacks" repositioned the #DF stack to be adjacent to >>>>> the guard page, which turns this OoB write into a fatal pagefault: >>>>> >>>>> (XEN) *** DOUBLE FAULT *** >>>>> (XEN) ----[ Xen-4.15-unstable x86_64 debug=y Tainted: C ]---- >>>>> (XEN) ----[ Xen-4.15-unstable x86_64 debug=y Tainted: C ]---- >>>>> (XEN) CPU: 4 >>>>> (XEN) RIP: e008:[<ffff82d04031fd4f>] traps.c#read_registers+0x29/0xc1 >>>>> (XEN) RFLAGS: 0000000000050086 CONTEXT: hypervisor (d1v0) >>>>> ... >>>>> (XEN) Xen call trace: >>>>> (XEN) [<ffff82d04031fd4f>] R traps.c#read_registers+0x29/0xc1 >>>>> (XEN) [<ffff82d0403207b3>] F do_double_fault+0x3d/0x7e >>>>> (XEN) [<ffff82d04039acd7>] F double_fault+0x107/0x110 >>>>> (XEN) >>>>> (XEN) Pagetable walk from ffff830236f6d008: >>>>> (XEN) L4[0x106] = 80000000bfa9b063 ffffffffffffffff >>>>> (XEN) L3[0x008] = 0000000236ffd063 ffffffffffffffff >>>>> (XEN) L2[0x1b7] = 0000000236ffc063 ffffffffffffffff >>>>> (XEN) L1[0x16d] = 8000000236f6d161 ffffffffffffffff >>>>> (XEN) >>>>> (XEN) **************************************** >>>>> (XEN) Panic on CPU 4: >>>>> (XEN) FATAL PAGE FAULT >>>>> (XEN) [error_code=0003] >>>>> (XEN) Faulting linear address: ffff830236f6d008 >>>>> (XEN) **************************************** >>>>> (XEN) >>>>> >>>>> and rendering the main #DF analysis broken. >>>>> >>>>> The proper fix is to delete cpu_user_regs.es and later, so no >>>>> interrupt/exception path can access OoB, but this needs disentangling from the >>>>> PV ABI first. >>>>> >>>>> Not-really-fixes: 6001660473 ("x86/shstk: Rework the stack layout to support shadow stacks") >>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >>>> >>>> Is it perhaps worth also saying explicitly that the other IST >>>> stacks don't suffer the same problem because show_registers() >>>> makes an local copy of the passed in struct? (Doing so also >>>> for #DF would apparently be an alternative solution.) >>> They're not safe. They merely don't explode. >>> >>> https://lore.kernel.org/xen-devel/1532546157-5974-1-git-send-email-andrew.cooper3@citrix.com/ >>> was "fixed" by CET-SS turning the guard page from not present to >>> read-only, but the same CET-SS series swapped #DB for #DF when it comes >>> to the single OoB write problem case. >> I see. While indeed I didn't pay attention to the OoB read aspect, >> me saying "the other IST stacks don't suffer the same problem" was >> still correct, wasn't it? Anyway - not a big deal. > > I've tweaked the commit message to make this more clear. > > --8<--- > Accessing these fields is generally illegal, as they are logically out of > bounds for anything other than an interrupt/exception hitting ring1/3 code. > > show_registers() unconditionally reads these fields, but the content is > discarded before use. This is benign right now, as all parts of the > stack are > readable, including the guard pages. > > However, read_registers() in the #DF handler writes to these fields as > part of > preparing the state dump, and being IST, hits the adjacent stack frame. > --8<-- Thanks, lgtm. Jan ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86/traps: 'Fix' safety of read_registers() in #DF path 2020-10-16 11:03 ` Jan Beulich @ 2020-10-16 11:24 ` Andrew Cooper 2020-10-16 11:55 ` Jan Beulich 0 siblings, 1 reply; 10+ messages in thread From: Andrew Cooper @ 2020-10-16 11:24 UTC (permalink / raw) To: Jan Beulich; +Cc: Xen-devel, Roger Pau Monné, Wei Liu, Julien Grall On 16/10/2020 12:03, Jan Beulich wrote: > On 16.10.2020 12:58, Andrew Cooper wrote: >> On 15/10/2020 08:27, Jan Beulich wrote: >>> On 14.10.2020 20:00, Andrew Cooper wrote: >>>> On 13/10/2020 16:51, Jan Beulich wrote: >>>>> On 12.10.2020 15:49, Andrew Cooper wrote: >>>>>> All interrupts and exceptions pass a struct cpu_user_regs up into C. This >>>>>> contains the legacy vm86 fields from 32bit days, which are beyond the >>>>>> hardware-pushed frame. >>>>>> >>>>>> Accessing these fields is generally illegal, as they are logically out of >>>>>> bounds for anything other than an interrupt/exception hitting ring1/3 code. >>>>>> >>>>>> Unfortunately, the #DF handler uses these fields as part of preparing the >>>>>> state dump, and being IST, accesses the adjacent stack frame. >>>>>> >>>>>> This has been broken forever, but c/s 6001660473 "x86/shstk: Rework the stack >>>>>> layout to support shadow stacks" repositioned the #DF stack to be adjacent to >>>>>> the guard page, which turns this OoB write into a fatal pagefault: >>>>>> >>>>>> (XEN) *** DOUBLE FAULT *** >>>>>> (XEN) ----[ Xen-4.15-unstable x86_64 debug=y Tainted: C ]---- >>>>>> (XEN) ----[ Xen-4.15-unstable x86_64 debug=y Tainted: C ]---- >>>>>> (XEN) CPU: 4 >>>>>> (XEN) RIP: e008:[<ffff82d04031fd4f>] traps.c#read_registers+0x29/0xc1 >>>>>> (XEN) RFLAGS: 0000000000050086 CONTEXT: hypervisor (d1v0) >>>>>> ... >>>>>> (XEN) Xen call trace: >>>>>> (XEN) [<ffff82d04031fd4f>] R traps.c#read_registers+0x29/0xc1 >>>>>> (XEN) [<ffff82d0403207b3>] F do_double_fault+0x3d/0x7e >>>>>> (XEN) [<ffff82d04039acd7>] F double_fault+0x107/0x110 >>>>>> (XEN) >>>>>> (XEN) Pagetable walk from ffff830236f6d008: >>>>>> (XEN) L4[0x106] = 80000000bfa9b063 ffffffffffffffff >>>>>> (XEN) L3[0x008] = 0000000236ffd063 ffffffffffffffff >>>>>> (XEN) L2[0x1b7] = 0000000236ffc063 ffffffffffffffff >>>>>> (XEN) L1[0x16d] = 8000000236f6d161 ffffffffffffffff >>>>>> (XEN) >>>>>> (XEN) **************************************** >>>>>> (XEN) Panic on CPU 4: >>>>>> (XEN) FATAL PAGE FAULT >>>>>> (XEN) [error_code=0003] >>>>>> (XEN) Faulting linear address: ffff830236f6d008 >>>>>> (XEN) **************************************** >>>>>> (XEN) >>>>>> >>>>>> and rendering the main #DF analysis broken. >>>>>> >>>>>> The proper fix is to delete cpu_user_regs.es and later, so no >>>>>> interrupt/exception path can access OoB, but this needs disentangling from the >>>>>> PV ABI first. >>>>>> >>>>>> Not-really-fixes: 6001660473 ("x86/shstk: Rework the stack layout to support shadow stacks") >>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >>>>> >>>>> Is it perhaps worth also saying explicitly that the other IST >>>>> stacks don't suffer the same problem because show_registers() >>>>> makes an local copy of the passed in struct? (Doing so also >>>>> for #DF would apparently be an alternative solution.) >>>> They're not safe. They merely don't explode. >>>> >>>> https://lore.kernel.org/xen-devel/1532546157-5974-1-git-send-email-andrew.cooper3@citrix.com/ >>>> was "fixed" by CET-SS turning the guard page from not present to >>>> read-only, but the same CET-SS series swapped #DB for #DF when it comes >>>> to the single OoB write problem case. >>> I see. While indeed I didn't pay attention to the OoB read aspect, >>> me saying "the other IST stacks don't suffer the same problem" was >>> still correct, wasn't it? Anyway - not a big deal. >> I've tweaked the commit message to make this more clear. >> >> --8<--- >> Accessing these fields is generally illegal, as they are logically out of >> bounds for anything other than an interrupt/exception hitting ring1/3 code. >> >> show_registers() unconditionally reads these fields, but the content is >> discarded before use. This is benign right now, as all parts of the >> stack are >> readable, including the guard pages. >> >> However, read_registers() in the #DF handler writes to these fields as >> part of >> preparing the state dump, and being IST, hits the adjacent stack frame. >> --8<-- > Thanks, lgtm. On a tangent, what are your views WRT backport beyond 4.14? Back then, it was #DB which was adjacent to the guard frame (which was not present), but it doesn't use show_registers() by default, so I think the problem is mostly hidden. ~Andrew ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86/traps: 'Fix' safety of read_registers() in #DF path 2020-10-16 11:24 ` Andrew Cooper @ 2020-10-16 11:55 ` Jan Beulich 2020-10-16 12:07 ` Andrew Cooper 0 siblings, 1 reply; 10+ messages in thread From: Jan Beulich @ 2020-10-16 11:55 UTC (permalink / raw) To: Andrew Cooper; +Cc: Xen-devel, Roger Pau Monné, Wei Liu, Julien Grall On 16.10.2020 13:24, Andrew Cooper wrote: > On a tangent, what are your views WRT backport beyond 4.14? > > Back then, it was #DB which was adjacent to the guard frame (which was > not present), but it doesn't use show_registers() by default, so I think > the problem is mostly hidden. I wasn't fully decided yet, but as long as it applies reasonably cleanly I think I'm leaning towards also putting it on 4.13. 4.12 closes anyway once 4.12.4 is out, and I don't think I want to pick up not-really-urgent changes for putting there beyond the few ones that I already have (and that I mean to put in alongside the XSA fixes on Tuesday); I could be talked into it, though. Jan ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86/traps: 'Fix' safety of read_registers() in #DF path 2020-10-16 11:55 ` Jan Beulich @ 2020-10-16 12:07 ` Andrew Cooper 2020-10-16 12:14 ` Jan Beulich 0 siblings, 1 reply; 10+ messages in thread From: Andrew Cooper @ 2020-10-16 12:07 UTC (permalink / raw) To: Jan Beulich; +Cc: Xen-devel, Roger Pau Monné, Wei Liu, Julien Grall On 16/10/2020 12:55, Jan Beulich wrote: > On 16.10.2020 13:24, Andrew Cooper wrote: >> On a tangent, what are your views WRT backport beyond 4.14? >> >> Back then, it was #DB which was adjacent to the guard frame (which was >> not present), but it doesn't use show_registers() by default, so I think >> the problem is mostly hidden. > I wasn't fully decided yet, but as long as it applies reasonably > cleanly I think I'm leaning towards also putting it on 4.13. > 4.12 closes anyway once 4.12.4 is out, and I don't think I want > to pick up not-really-urgent changes for putting there beyond > the few ones that I already have (and that I mean to put in > alongside the XSA fixes on Tuesday); I could be talked into it, > though. The question I was asking was really "should I try and make an equivalent fix for 4.13 and older". While the base premise of the fix would be the same, the logic in load_system_tables() is different, and the commit message is completely wrong. I only encountered this problem with added instrumentation in the #DB handler, which is why I'm questioning the utility of going to this effort. ~Andrew ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86/traps: 'Fix' safety of read_registers() in #DF path 2020-10-16 12:07 ` Andrew Cooper @ 2020-10-16 12:14 ` Jan Beulich 0 siblings, 0 replies; 10+ messages in thread From: Jan Beulich @ 2020-10-16 12:14 UTC (permalink / raw) To: Andrew Cooper; +Cc: Xen-devel, Roger Pau Monné, Wei Liu, Julien Grall On 16.10.2020 14:07, Andrew Cooper wrote: > On 16/10/2020 12:55, Jan Beulich wrote: >> On 16.10.2020 13:24, Andrew Cooper wrote: >>> On a tangent, what are your views WRT backport beyond 4.14? >>> >>> Back then, it was #DB which was adjacent to the guard frame (which was >>> not present), but it doesn't use show_registers() by default, so I think >>> the problem is mostly hidden. >> I wasn't fully decided yet, but as long as it applies reasonably >> cleanly I think I'm leaning towards also putting it on 4.13. >> 4.12 closes anyway once 4.12.4 is out, and I don't think I want >> to pick up not-really-urgent changes for putting there beyond >> the few ones that I already have (and that I mean to put in >> alongside the XSA fixes on Tuesday); I could be talked into it, >> though. > > The question I was asking was really "should I try and make an > equivalent fix for 4.13 and older". Oh, I see. > While the base premise of the fix would be the same, the logic in > load_system_tables() is different, and the commit message is completely > wrong. > > I only encountered this problem with added instrumentation in the #DB > handler, which is why I'm questioning the utility of going to this effort. Yeah, then probably not worth it. Jan ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-10-16 12:14 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-12 13:49 [PATCH] x86/traps: 'Fix' safety of read_registers() in #DF path Andrew Cooper 2020-10-13 15:51 ` Jan Beulich 2020-10-14 18:00 ` Andrew Cooper 2020-10-15 7:27 ` Jan Beulich 2020-10-16 10:58 ` Andrew Cooper 2020-10-16 11:03 ` Jan Beulich 2020-10-16 11:24 ` Andrew Cooper 2020-10-16 11:55 ` Jan Beulich 2020-10-16 12:07 ` Andrew Cooper 2020-10-16 12:14 ` Jan Beulich
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).