From: Michal Luczaj <mhal@rbox.co>
To: Borislav Petkov <bp@alien8.de>
Cc: x86@kernel.org, tglx@linutronix.de, mingo@redhat.com,
dave.hansen@linux.intel.com, shuah@kernel.org, luto@kernel.org,
torvalds@linuxfoundation.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 0/2] x86: UMIP emulation leaking kernel addresses
Date: Sun, 10 Dec 2023 18:08:43 +0100 [thread overview]
Message-ID: <c2da6e0b-aad1-4151-a8d3-07afb149b1c5@rbox.co> (raw)
In-Reply-To: <20231209155308.GAZXSNZB9PPlcHBI4W@fat_crate.local>
On 12/9/23 16:53, Borislav Petkov wrote:
> On Wed, Dec 06, 2023 at 01:43:43AM +0100, Michal Luczaj wrote:
>> Introducing a DPL check in insn_get_seg_base(), or even in get_desc(),
>> seems enough to prevent the decoder from disclosing data.
>>
>> diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
>> index 558a605929db..4c1eea736519 100644
>> --- a/arch/x86/lib/insn-eval.c
>> +++ b/arch/x86/lib/insn-eval.c
>> @@ -725,6 +725,18 @@ unsigned long insn_get_seg_base(struct pt_regs *regs, int seg_reg_idx)
>> if (!get_desc(&desc, sel))
>> return -1L;
>>
>> + /*
>> + * Some segment selectors coming from @regs do not necessarily reflect
>> + * the state of CPU; see get_segment_selector(). Their values might
>> + * have been altered by ptrace. Thus, the instruction decoder can be
>> + * tricked into "dereferencing" a segment descriptor that would
>> + * otherwise cause a CPU exception -- for example due to mismatched
>> + * privilege levels. This opens up the possibility to expose kernel
>> + * space base address of DPL=0 segments.
>> + */
>> + if (desc.dpl < (regs->cs & SEGMENT_RPL_MASK))
>> + return -1L;
>> +
>> return get_desc_base(&desc);
>> }
>>
>> That said, I guess instead of trying to harden the decoder,
>
> Well, here's what my CPU manual says:
>
> "4.10.1 Accessing Data Segments
>
> ...
>
> The processor compares the effective privilege level with the DPL in the
> descriptor-table entry referenced by the segment selector. If the
> effective privilege level is greater than or equal to (numerically
> lower-than or equal-to) the DPL, then the processor loads the segment
> register with the data-segment selector.
>
> If the effective privilege level is lower than (numerically
> greater-than) the DPL, a general-protection exception (#GP) occurs and
> the segment register is not loaded.
>
> ...
>
> 4.10.2 Accessing Stack Segments
>
> The processor compares the CPL with the DPL in the descriptor-table
> entry referenced by the segment selector. The two values must be equal.
> If they are not equal, a #GP occurs and the SS register is not loaded."
>
> So *actually* doing those checks in the insn decoder is the proper thing
> to do, IMNSVHO.
Are you suggesting checking only CPL vs. DPL or making sure the insn
decoder faithfully emulates all the other stuff CPU does? Like the desc.s
issue described below.
>> Now, I'm far from being competent, but here's an idea I've tried: tell
>> the #GP handler that UMIP-related exceptions come only as #GP(0):
>>
>> if (static_cpu_has(X86_FEATURE_UMIP)) {
>> - if (user_mode(regs) && fixup_umip_exception(regs))
>> + if (user_mode(regs) && !error_code && fixup_umip_exception(regs))
>> goto exit;
>> }
>
> And yap, as you've realized, that alone doesn't fix the leaking.
With this fix applied, I can't see any way to sufficiently confuse the
UMIP emulation with a non-ESPFIX bad IRET. It appears that #GP(selector)
takes precedence over #GP(0), so tripping IRET with any malformed selector
always ends up with #GP handler's error_code != 0, even if conditions were
met for #GP(0) just as well. Is there something I'm missing?
That said, there's still the case of #DF handler feeding #GP handler after
a fault in ESPFIX. Consider
cs = (GDT_ENTRY_TSS << 3) | USER_RPL
ss = (SOME_LDT_ENTRY << 3) | SEGMENT_LDT | USER_RPL
ip = "sgdt %cs:(%reg)"
Attempting IRET with such illegal CS raises #GP(selector), but (because of
ESPFIX) this #GP is promoted to #DF where it becomes #GP(0). And UMIP
emulation is triggered.
UMIP emulator starts by fetching code from user. insn decoder does
`copy_from_user(buf, (void __user *)ip, MAX_INSN_SIZE)` where `ip` is
CS.base+IP and CS.base here is actually a (part of) GDT_ENTRY_TSS.base, a
kernel address. With IP under user's control, no fault while copying.
Next, insn_get_code_seg_params() concludes that, given TSS as a code
segment, address and operand size are both 16-bit. Prefix SGDT with size
overrides, and we're back to 32-bit. Then insn_get_addr_ref() and
copy_to_user() does the leaking.
I don't know if/how to deal with ESPFIX losing #GP's error code. As for
telling insn decoder that system segments aren't code:
--- a/arch/x86/lib/insn-eval.c
+++ b/arch/x86/lib/insn-eval.c
@@ -809,6 +809,10 @@ int insn_get_code_seg_params(struct pt_regs *regs)
if (!get_desc(&desc, sel))
return -EINVAL;
+ /* System segments are not code. */
+ if (!desc.s)
+ return -EINVAL;
+
/*
* The most significant byte of the Type field of the segment descriptor
* determines whether a segment contains data or code. If this is a data
Is this something in the right direction?
(Note, get_segment_selector() is broken for selectors with the high bit
set. I'll send patch later.)
thanks,
Michal
next prev parent reply other threads:[~2023-12-10 17:17 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-06 0:43 [PATCH 0/2] x86: UMIP emulation leaking kernel addresses Michal Luczaj
2023-12-06 0:43 ` [PATCH 1/2] x86/traps: Attempt UMIP fixup only on #GP(0) Michal Luczaj
2023-12-06 0:43 ` [PATCH 2/2] selftests/x86: UMIP DPL=0 segment base address info leak test Michal Luczaj
2023-12-09 15:53 ` [PATCH 0/2] x86: UMIP emulation leaking kernel addresses Borislav Petkov
2023-12-10 17:08 ` Michal Luczaj [this message]
2023-12-09 17:16 ` Brian Gerst
2023-12-09 20:08 ` Linus Torvalds
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=c2da6e0b-aad1-4151-a8d3-07afb149b1c5@rbox.co \
--to=mhal@rbox.co \
--cc=bp@alien8.de \
--cc=dave.hansen@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@kernel.org \
--cc=mingo@redhat.com \
--cc=shuah@kernel.org \
--cc=tglx@linutronix.de \
--cc=torvalds@linuxfoundation.org \
--cc=x86@kernel.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 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).