From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Xen-devel <xen-devel@lists.xenproject.org>,
"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
"Julien Grall" <julien@xen.org>
Subject: Re: [PATCH] x86/traps: 'Fix' safety of read_registers() in #DF path
Date: Fri, 16 Oct 2020 13:03:51 +0200 [thread overview]
Message-ID: <00ba5885-5ee6-c772-a72e-15431cd3b1f4@suse.com> (raw)
In-Reply-To: <dbd19cd0-316a-c62f-de7b-627ada4df350@citrix.com>
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
next prev parent reply other threads:[~2020-10-16 11:04 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=00ba5885-5ee6-c772-a72e-15431cd3b1f4@suse.com \
--to=jbeulich@suse.com \
--cc=andrew.cooper3@citrix.com \
--cc=julien@xen.org \
--cc=roger.pau@citrix.com \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.