Michal noted[1] that on systems that support UMIP, the instruction
decoder can be tricked into leaking the address of the TSS or LDT by
using ptrace to set the SS segment to a system segment index. Prevent
this from happening by rejecting attempts to use a system segment in the
ptrace and sigreturn syscalls.
[1] https://lore.kernel.org/lkml/[email protected]/
Brian Gerst (3):
x86: Move TSS and LDT to end of the GDT
x86/ptrace: Reject system segements
x86/sigreturn: Reject system segements
arch/x86/include/asm/segment.h | 44 ++++++++++++++++++++++++----------
arch/x86/kernel/ptrace.c | 12 ++--------
arch/x86/kernel/signal_32.c | 4 ++++
arch/x86/kernel/signal_64.c | 4 ++++
4 files changed, 42 insertions(+), 22 deletions(-)
base-commit: 3d626e0a7be7ddb635791fee18cb40631bc1d0b3
--
2.43.0
Do not allow system segments (TSS and LDT) from being loaded into segment
registers via sigreturn. Loading these segments into a segment register
normally results in a general protection fault. In the case of sigreturn,
setting CS or SS to a system segment will cause IRET to fault. This
then results in the instruction decoder attempting to use the invalid
segment. This can be avoided by rejecting system segments in the
sigreturn() syscall.
Signed-off-by: Brian Gerst <[email protected]>
Reported-By: Michal Luczaj <[email protected]>
Link: https://lore.kernel.org/lkml/[email protected]/
---
arch/x86/kernel/signal_32.c | 4 ++++
arch/x86/kernel/signal_64.c | 4 ++++
2 files changed, 8 insertions(+)
diff --git a/arch/x86/kernel/signal_32.c b/arch/x86/kernel/signal_32.c
index c12624bc82a3..0e1926b676b0 100644
--- a/arch/x86/kernel/signal_32.c
+++ b/arch/x86/kernel/signal_32.c
@@ -98,7 +98,11 @@ static bool ia32_restore_sigcontext(struct pt_regs *regs,
/* Get CS/SS and force CPL3 */
regs->cs = sc.cs | 0x03;
+ if (!valid_user_selector(regs->cs))
+ return false;
regs->ss = sc.ss | 0x03;
+ if (!valid_user_selector(regs->ss))
+ return false;
regs->flags = (regs->flags & ~FIX_EFLAGS) | (sc.flags & FIX_EFLAGS);
/* disable syscall checks */
diff --git a/arch/x86/kernel/signal_64.c b/arch/x86/kernel/signal_64.c
index 23d8aaf8d9fd..666b147bf43a 100644
--- a/arch/x86/kernel/signal_64.c
+++ b/arch/x86/kernel/signal_64.c
@@ -79,7 +79,11 @@ static bool restore_sigcontext(struct pt_regs *regs,
/* Get CS/SS and force CPL3 */
regs->cs = sc.cs | 0x03;
+ if (!valid_user_selector(regs->cs))
+ return false;
regs->ss = sc.ss | 0x03;
+ if (!valid_user_selector(regs->ss))
+ return false;
regs->flags = (regs->flags & ~FIX_EFLAGS) | (sc.flags & FIX_EFLAGS);
/* disable syscall checks */
--
2.43.0
This will make testing for system segments easier.
Signed-off-by: Brian Gerst <[email protected]>
---
arch/x86/include/asm/segment.h | 33 +++++++++++++++++++++------------
1 file changed, 21 insertions(+), 12 deletions(-)
diff --git a/arch/x86/include/asm/segment.h b/arch/x86/include/asm/segment.h
index 9d6411c65920..a155843d0c37 100644
--- a/arch/x86/include/asm/segment.h
+++ b/arch/x86/include/asm/segment.h
@@ -83,8 +83,8 @@
* 13 - kernel data segment
* 14 - default user CS
* 15 - default user DS
- * 16 - TSS <=== cacheline #5
- * 17 - LDT
+ * 16 - unused <=== cacheline #5
+ * 17 - unused
* 18 - PNPBIOS support (16->32 gate)
* 19 - PNPBIOS support
* 20 - PNPBIOS support <=== cacheline #6
@@ -97,8 +97,11 @@
* 26 - ESPFIX small SS
* 27 - per-cpu [ offset to per-cpu data area ]
* 28 - VDSO getcpu
- * 29 - unused
- * 30 - unused
+ *
+ * ------- start of system segments:
+ *
+ * 29 - TSS
+ * 30 - LDT
* 31 - TSS for double fault handler
*/
#define GDT_ENTRY_TLS_MIN 6
@@ -108,8 +111,6 @@
#define GDT_ENTRY_KERNEL_DS 13
#define GDT_ENTRY_DEFAULT_USER_CS 14
#define GDT_ENTRY_DEFAULT_USER_DS 15
-#define GDT_ENTRY_TSS 16
-#define GDT_ENTRY_LDT 17
#define GDT_ENTRY_PNPBIOS_CS32 18
#define GDT_ENTRY_PNPBIOS_CS16 19
#define GDT_ENTRY_PNPBIOS_DS 20
@@ -121,6 +122,10 @@
#define GDT_ENTRY_PERCPU 27
#define GDT_ENTRY_CPUNODE 28
+/* Start of system segments */
+
+#define GDT_ENTRY_TSS 29
+#define GDT_ENTRY_LDT 30
#define GDT_ENTRY_DOUBLEFAULT_TSS 31
/*
@@ -188,20 +193,22 @@
#define GDT_ENTRY_DEFAULT_USER_DS 5
#define GDT_ENTRY_DEFAULT_USER_CS 6
-/* Needs two entries */
-#define GDT_ENTRY_TSS 8
-/* Needs two entries */
-#define GDT_ENTRY_LDT 10
-
#define GDT_ENTRY_TLS_MIN 12
#define GDT_ENTRY_TLS_MAX 14
#define GDT_ENTRY_CPUNODE 15
+/* Start of system segments */
+
+/* Needs two entries */
+#define GDT_ENTRY_TSS 16
+/* Needs two entries */
+#define GDT_ENTRY_LDT 18
+
/*
* Number of entries in the GDT table:
*/
-#define GDT_ENTRIES 16
+#define GDT_ENTRIES 20
/*
* Segment selector values corresponding to the above entries:
@@ -219,6 +226,8 @@
#endif
+#define GDT_SYSTEM_START GDT_ENTRY_TSS
+
#define IDT_ENTRIES 256
#define NUM_EXCEPTION_VECTORS 32
--
2.43.0
Do not allow system segments (TSS and LDT) from being poked into segment
registers via ptrace. Loading these segments into a segment register
normally results in a general protection fault. But in the case of
ptrace, setting CS or SS to a system segment will cause IRET to fault.
This then results in the instruction decoder attempting to use the
invalid segment. This can be avoided by rejecting system segments in
PTRACE_SETREGS.
Signed-off-by: Brian Gerst <[email protected]>
Reported-By: Michal Luczaj <[email protected]>
Link: https://lore.kernel.org/lkml/[email protected]/
---
arch/x86/include/asm/segment.h | 11 +++++++++++
arch/x86/kernel/ptrace.c | 12 ++----------
2 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/arch/x86/include/asm/segment.h b/arch/x86/include/asm/segment.h
index a155843d0c37..ede1fa5aa4cc 100644
--- a/arch/x86/include/asm/segment.h
+++ b/arch/x86/include/asm/segment.h
@@ -359,6 +359,17 @@ static inline void __loadsegment_fs(unsigned short value)
#define savesegment(seg, value) \
asm("mov %%" #seg ",%0":"=r" (value) : : "memory")
+/*
+ * Determines whether a value may be installed in a segment register.
+ */
+static inline bool valid_user_selector(u16 value)
+{
+ if (unlikely(!(value & SEGMENT_TI_MASK) && value >= (GDT_SYSTEM_START * 8)))
+ return false;
+
+ return likely(value == 0 || (value & SEGMENT_RPL_MASK) == USER_RPL);
+}
+
#endif /* !__ASSEMBLY__ */
#endif /* __KERNEL__ */
diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 095f04bdabdc..4c3a2278691e 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -162,14 +162,6 @@ const char *regs_query_register_name(unsigned int offset)
X86_EFLAGS_DF | X86_EFLAGS_OF | \
X86_EFLAGS_RF | X86_EFLAGS_AC))
-/*
- * Determines whether a value may be installed in a segment register.
- */
-static inline bool invalid_selector(u16 value)
-{
- return unlikely(value != 0 && (value & SEGMENT_RPL_MASK) != USER_RPL);
-}
-
#ifdef CONFIG_X86_32
#define FLAG_MASK FLAG_MASK_32
@@ -206,7 +198,7 @@ static int set_segment_reg(struct task_struct *task,
/*
* The value argument was already truncated to 16 bits.
*/
- if (invalid_selector(value))
+ if (!valid__user_selector(value))
return -EIO;
/*
@@ -296,7 +288,7 @@ static int set_segment_reg(struct task_struct *task,
/*
* The value argument was already truncated to 16 bits.
*/
- if (invalid_selector(value))
+ if (!valid_user_selector(value))
return -EIO;
/*
--
2.43.0
On Wed, 13 Dec 2023 at 08:34, Brian Gerst <[email protected]> wrote:
>
> This will make testing for system segments easier.
It seems to make more sense organizationally too, with the special
non-data/code segments clearly separate at the end.
So I think this is fine conceptually.
HOWEVER, I think that you might want to expand on this a bit more,
because there are other special segments selectors that might not be
thing you want to expose to user space.
We have GDT_ENTRY_PERCPU for example, which is a kernel-only segment.
It also happens to be 32-bit only, it doesn't matter for the thing
you're trying to fix, but that valid_user_selector() thing is then
used on x86-32 too.
So the ESPFIX and per-cpu segments are kernel-only, but then the VDSO
getcpu one is a user segment.
And the PnP and APM BIOS segments are similarly kernel-only.
But then the VDSO getcpu segment is user-visible, in the middle, and
again, it's 32-bit only but that whole GDT_SYSTEM_START thing is
supposed to work there too.
End result: this seems incomplete and not really fully baked.
I wonder if instead of GDT_SYSTEM_START, you'd be better off just
making a trivial constant bitmap of "these are user visible segments
in the GDT". No need to re-order things, just have something like
#define USER_SEGMENTS_MASK \
((1ul << GDT_ENTRY_DEFAULT_USER_CS) |
,,,,
and use that for the test (remember to check for GDT_ENTRIES as the max).
Hmm?
Linus
On Wed, 13 Dec 2023 at 08:34, Brian Gerst <[email protected]> wrote:
>
> @@ -98,7 +98,11 @@ static bool ia32_restore_sigcontext(struct pt_regs *regs,
>
> /* Get CS/SS and force CPL3 */
> regs->cs = sc.cs | 0x03;
> + if (!valid_user_selector(regs->cs))
> + return false;
> regs->ss = sc.ss | 0x03;
> + if (!valid_user_selector(regs->ss))
> + return false;
Side note: the SS/CS checks could be stricter than the usual selector tests.
In particular, normal segments can be Null segments. But CS/SS must not be.
Also, since you're now checking the validity, maybe we shouldn't do
the "force cpl3" any more, and just make it an error to try to load a
non-cpl3 segment at sigreturn..
That forcing was literally just because we weren't checking it for sanity...
Linus
On Wed, 13 Dec 2023 at 10:51, Linus Torvalds
<[email protected]> wrote:
>
> We have GDT_ENTRY_PERCPU for example, which is a kernel-only segment.
> It also happens to be 32-bit only, it doesn't matter for the thing
> you're trying to fix, but that valid_user_selector() thing is then
> used on x86-32 too.
>
> So the ESPFIX and per-cpu segments are kernel-only, but then the VDSO
> getcpu one is a user segment.
>
> And the PnP and APM BIOS segments are similarly kernel-only.
Final (?) note: when looking at this, I have to say that our
GDT_ENTRY_INIT() and GDT_ENTRY() macros are horrendous.
I know exactly *why* they are horrendous, with all the history of
passing in raw flags values, etc, and you can most certainly see that
whole thing in the GDT_ENTRY() macro. It's used in assembly code in a
couple of cases too.
But then you look at GDT_ENTRY_INIT(), and it turns that illegible
"flags" value into (slightly more) legible S/DPL/etc values. So it
literally makes people use those odd "this is how this is encoded"
values even when the code actually wants to use a structure definition
that has the flags split out.
I guess it's much too much work to really fix things, but maybe we
could at least add #defines and comments for the special values.
So instead of
GDT_ENTRY_INIT(0xc093, 0, 0xfffff)
we could maybe have
#define GDT_ENTRY_FLAGS(type,s,dpl,p,avl,l,d,g) \
((type) |
(s)<<4) | \
(dpl) << 5) | ....
and have #defines for those 0xc093 values (with comments), so that we'd have
GDT_ENTRY_INIT(KERNEL_DATA_FLAGS, 0, 0xffff)
instead of a magic 0xc093 number.
This would require some nit-picky "read all those values and know the
crazy descriptor table layout" thing. Maybe somebody has a serious
case of insomnia and boredom?
Linus
On 13/12/2023 20:08, Linus Torvalds wrote:
> I guess it's much too much work to really fix things, but maybe we
> could at least add #defines and comments for the special values.
>
> So instead of
>
> GDT_ENTRY_INIT(0xc093, 0, 0xfffff)
>
> we could maybe have
>
> #define GDT_ENTRY_FLAGS(type,s,dpl,p,avl,l,d,g) \
> ((type) |
> (s)<<4) | \
> (dpl) << 5) | ....
>
> and have #defines for those 0xc093 values (with comments), so that we'd have
>
> GDT_ENTRY_INIT(KERNEL_DATA_FLAGS, 0, 0xffff)
>
> instead of a magic 0xc093 number.
>
> This would require some nit-picky "read all those values and know the
> crazy descriptor table layout" thing. Maybe somebody has a serious
> case of insomnia and boredom?
I took a stab at this, see attached RFC patch. Maybe this does too much,
though.
I did basic build and boot tests on both 64- and 32-bit, but I would
also try a binary diff before/after just to verify nothing changed by
accident, as well as making sure all the code is actually compiled (some
of the BIOS stuff only gets used on 32-bit with ISA/PNP enabled, for
example).
While preparing the patch I also came across some things that are
unclear to me:
- why do we want some segments with the A (accessed) bit set and some
with it cleared -- is there an actual reason for the difference, or
could we just set it for all of them?
- why does setup_percpu_segment() want the DB (size) flag clear? This
seems to indicate that it's a 16-bit segment -- is this correct?
Vegard
On Sat, 16 Dec 2023 at 10:25, Vegard Nossum <[email protected]> wrote:
>
> While preparing the patch I also came across some things that are
> unclear to me:
>
> - why do we want some segments with the A (accessed) bit set and some
> with it cleared -- is there an actual reason for the difference, or
> could we just set it for all of them?
I think it's random, and an effect of just having hardcoded numbers
and not having any structure to it.
But I do think you're right that we should just start with all
kernel-created segment descriptors marked as accessed. I do not
believe that we have any actual *use* for the descriptor access bit.
> - why does setup_percpu_segment() want the DB (size) flag clear? This
> seems to indicate that it's a 16-bit segment -- is this correct?
I think it's nonsensical and doesn't matter, and is another mistake
from us just having random numbers.
I don't think the DB bit matters except for when it's used for the
code or stack segment (or, apparently, if it's a grow-down segment).
So I think your patch looks good, and I would keep it in that form if
it makes it easier to just verify that it generates an identical
kernel image.
And then as a separate patch, I would remove that DB bit clear thing.
Anyway, I do like your patch, and I think the fact that you found
those oddities is a good argument *for* the patch, but at the same
time I think I'll just bow to the x86 maintainers who may think that
this is churn in an area that they'd rather not touch any more.
So consider that an "ack" from me, but with that caveat of yes, I
think a binary diff would be a good thing because this is *so* odd and
low-level and maybe people just think it's not worth it.
Thanks,
Linus
On December 13, 2023 10:54:00 AM PST, Linus Torvalds <[email protected]> wrote:
>On Wed, 13 Dec 2023 at 08:34, Brian Gerst <[email protected]> wrote:
>>
>> @@ -98,7 +98,11 @@ static bool ia32_restore_sigcontext(struct pt_regs *regs,
>>
>> /* Get CS/SS and force CPL3 */
>> regs->cs = sc.cs | 0x03;
>> + if (!valid_user_selector(regs->cs))
>> + return false;
>> regs->ss = sc.ss | 0x03;
>> + if (!valid_user_selector(regs->ss))
>> + return false;
>
>Side note: the SS/CS checks could be stricter than the usual selector tests.
>
>In particular, normal segments can be Null segments. But CS/SS must not be.
>
>Also, since you're now checking the validity, maybe we shouldn't do
>the "force cpl3" any more, and just make it an error to try to load a
>non-cpl3 segment at sigreturn..
>
>That forcing was literally just because we weren't checking it for sanity...
>
> Linus
Not to mention that changing a null descriptor to 3 is wrong.
On December 13, 2023 10:51:11 AM PST, Linus Torvalds <[email protected]> wrote:
>On Wed, 13 Dec 2023 at 08:34, Brian Gerst <[email protected]> wrote:
>>
>> This will make testing for system segments easier.
>
>It seems to make more sense organizationally too, with the special
>non-data/code segments clearly separate at the end.
>
>So I think this is fine conceptually.
>
>HOWEVER, I think that you might want to expand on this a bit more,
>because there are other special segments selectors that might not be
>thing you want to expose to user space.
>
>We have GDT_ENTRY_PERCPU for example, which is a kernel-only segment.
>It also happens to be 32-bit only, it doesn't matter for the thing
>you're trying to fix, but that valid_user_selector() thing is then
>used on x86-32 too.
>
>So the ESPFIX and per-cpu segments are kernel-only, but then the VDSO
>getcpu one is a user segment.
>
>And the PnP and APM BIOS segments are similarly kernel-only.
>
>But then the VDSO getcpu segment is user-visible, in the middle, and
>again, it's 32-bit only but that whole GDT_SYSTEM_START thing is
>supposed to work there too.
>
>End result: this seems incomplete and not really fully baked.
>
>I wonder if instead of GDT_SYSTEM_START, you'd be better off just
>making a trivial constant bitmap of "these are user visible segments
>in the GDT". No need to re-order things, just have something like
>
> #define USER_SEGMENTS_MASK \
> ((1ul << GDT_ENTRY_DEFAULT_USER_CS) |
> ,,,,
>
>and use that for the test (remember to check for GDT_ENTRIES as the max).
>
>Hmm?
>
> Linus
Somewhat unrelated: X86_BUG_ESPFIX should just be deleted, as we aren't actually ever cleaning it. All current x86 processors have that problem (until FRED).
On Sun, 17 Dec 2023 at 13:08, H. Peter Anvin <[email protected]> wrote:
>
> On December 13, 2023 10:54:00 AM PST, Linus Torvalds <[email protected]> wrote:
]> >Side note: the SS/CS checks could be stricter than the usual selector tests.
> >
> >In particular, normal segments can be Null segments. But CS/SS must not be.
> >
> >Also, since you're now checking the validity, maybe we shouldn't do
> >the "force cpl3" any more, and just make it an error to try to load a
> >non-cpl3 segment at sigreturn..
> >
> >That forcing was literally just because we weren't checking it for sanity...
> >
> > Linus
>
> Not to mention that changing a null descriptor to 3 is wrong.
I don't think it is. All of 0-3 are "Null selectors". The RPL of the
selector simply doesn't matter when the index is zero, afaik.
But we obviously only do this for CS/SS, which can't be (any kind of)
Null selector and iret will GP on them regardless of the RPL in the
selector.
Linus
On December 17, 2023 1:40:53 PM PST, Linus Torvalds <[email protected]> wrote:
>On Sun, 17 Dec 2023 at 13:08, H. Peter Anvin <[email protected]> wrote:
>>
>> On December 13, 2023 10:54:00 AM PST, Linus Torvalds <[email protected]> wrote:
>]> >Side note: the SS/CS checks could be stricter than the usual selector tests.
>> >
>> >In particular, normal segments can be Null segments. But CS/SS must not be.
>> >
>> >Also, since you're now checking the validity, maybe we shouldn't do
>> >the "force cpl3" any more, and just make it an error to try to load a
>> >non-cpl3 segment at sigreturn..
>> >
>> >That forcing was literally just because we weren't checking it for sanity...
>> >
>> > Linus
>>
>> Not to mention that changing a null descriptor to 3 is wrong.
>
>I don't think it is. All of 0-3 are "Null selectors". The RPL of the
>selector simply doesn't matter when the index is zero, afaik.
>
>But we obviously only do this for CS/SS, which can't be (any kind of)
>Null selector and iret will GP on them regardless of the RPL in the
>selector.
>
> Linus
Of course not for CS/SS, but I would agree that if the selector is 0 before the signal it shouldn't mysteriously and asynchronously become 3.
> -----Original Message-----
> From: H. Peter Anvin <[email protected]>
> Sent: Sunday, December 17, 2023 1:46 PM
> To: Linus Torvalds <[email protected]>
> Cc: Brian Gerst <[email protected]>; [email protected];
> [email protected]; Ingo Molnar <[email protected]>; Thomas Gleixner
> <[email protected]>; Borislav Petkov <[email protected]>; Peter Zijlstra
> <[email protected]>; Michal Luczaj <[email protected]>
> Subject: Re: [PATCH 3/3] x86/sigreturn: Reject system segements
>
> On December 17, 2023 1:40:53 PM PST, Linus Torvalds
> <[email protected]> wrote:
> >On Sun, 17 Dec 2023 at 13:08, H. Peter Anvin <[email protected]> wrote:
> >>
> >> On December 13, 2023 10:54:00 AM PST, Linus Torvalds
> <[email protected]> wrote:
> >]> >Side note: the SS/CS checks could be stricter than the usual selector tests.
> >> >
> >> >In particular, normal segments can be Null segments. But CS/SS must not be.
> >> >
> >> >Also, since you're now checking the validity, maybe we shouldn't do
> >> >the "force cpl3" any more, and just make it an error to try to load
> >> >a
> >> >non-cpl3 segment at sigreturn..
> >> >
> >> >That forcing was literally just because we weren't checking it for sanity...
> >> >
> >> > Linus
> >>
> >> Not to mention that changing a null descriptor to 3 is wrong.
> >
> >I don't think it is. All of 0-3 are "Null selectors". The RPL of the
> >selector simply doesn't matter when the index is zero, afaik.
> >
> >But we obviously only do this for CS/SS, which can't be (any kind of)
> >Null selector and iret will GP on them regardless of the RPL in the
> >selector.
> >
> > Linus
>
> Of course not for CS/SS, but I would agree that if the selector is 0 before the
> signal it shouldn't mysteriously and asynchronously become 3.
Unfortunately reload_segments() _always_ sets DPL bits of GS/FS/DS/ES
to 3, even for 0. And IRET clears DPL bits when loading a NULL selector
into GS/FS/DS/ES:
https://lore.kernel.org/lkml/[email protected]/
Thanks!
Xin