* [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).