diff mbox series

[3/3] x86/sigreturn: Reject system segements

Message ID 20231213163443.70490-4-brgerst@gmail.com
State New
Headers
Series Reject setting system segments from userspace |

Commit Message

Brian Gerst Dec. 13, 2023, 4:34 p.m. UTC 
  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 <brgerst@gmail.com>
Reported-By: Michal Luczaj <mhal@rbox.co>
Link: https://lore.kernel.org/lkml/20231206004654.2986026-1-mhal@rbox.co/
---
 arch/x86/kernel/signal_32.c | 4 ++++
 arch/x86/kernel/signal_64.c | 4 ++++
 2 files changed, 8 insertions(+)

Comments

Linus Torvalds Dec. 13, 2023, 6:54 p.m. UTC | #1 
On Wed, 13 Dec 2023 at 08:34, Brian Gerst <brgerst@gmail.com> 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
H. Peter Anvin Dec. 17, 2023, 9:07 p.m. UTC | #2 
On December 13, 2023 10:54:00 AM PST, Linus Torvalds <torvalds@linuxfoundation.org> wrote:
>On Wed, 13 Dec 2023 at 08:34, Brian Gerst <brgerst@gmail.com> 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.
Linus Torvalds Dec. 17, 2023, 9:40 p.m. UTC | #3 
On Sun, 17 Dec 2023 at 13:08, H. Peter Anvin <hpa@zytor.com> wrote:
>
> On December 13, 2023 10:54:00 AM PST, Linus Torvalds <torvalds@linuxfoundation.org> 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
H. Peter Anvin Dec. 17, 2023, 9:45 p.m. UTC | #4 
On December 17, 2023 1:40:53 PM PST, Linus Torvalds <torvalds@linuxfoundation.org> wrote:
>On Sun, 17 Dec 2023 at 13:08, H. Peter Anvin <hpa@zytor.com> wrote:
>>
>> On December 13, 2023 10:54:00 AM PST, Linus Torvalds <torvalds@linuxfoundation.org> 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.
Li, Xin3 Dec. 18, 2023, 8:31 a.m. UTC | #5 
> -----Original Message-----
> From: H. Peter Anvin <hpa@zytor.com>
> Sent: Sunday, December 17, 2023 1:46 PM
> To: Linus Torvalds <torvalds@linuxfoundation.org>
> Cc: Brian Gerst <brgerst@gmail.com>; linux-kernel@vger.kernel.org;
> x86@kernel.org; Ingo Molnar <mingo@kernel.org>; Thomas Gleixner
> <tglx@linutronix.de>; Borislav Petkov <bp@alien8.de>; Peter Zijlstra
> <peterz@infradead.org>; Michal Luczaj <mhal@rbox.co>
> Subject: Re: [PATCH 3/3] x86/sigreturn: Reject system segements
> 
> On December 17, 2023 1:40:53 PM PST, Linus Torvalds
> <torvalds@linuxfoundation.org> wrote:
> >On Sun, 17 Dec 2023 at 13:08, H. Peter Anvin <hpa@zytor.com> wrote:
> >>
> >> On December 13, 2023 10:54:00 AM PST, Linus Torvalds
> <torvalds@linuxfoundation.org> 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/20230706052231.2183-1-xin3.li@intel.com/

Thanks!
    Xin
diff mbox series

Patch

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 */