[PATCH v2 0/2] x86_64: Sigcontext improvements

[PATCH v2 0/2] x86_64: Sigcontext improvements

[Andy Lutomirski]

Patch 1 is IMO a bug fix.

Patch 2 is a cleanup and avoids some confusion.  It's also sort of an
API break (no ABI change) because it removes a struct field.  But that
struct field has never done anything at all as far as I can tell.

AFAICT this series got bogged down in discussion about patch 2 last
time.  I'm only including patch 2 for completeness here -- patch 1
is IMO far more useful, and I'm fine with patch 1 being merged and
tabling patch 2 again.

Changes from v1:
 - Add the __USER_DS fix, caught by my sigreturn test.
 - Re-tested locally.

Andy Lutomirski (2):
  x86_64,signal: Fix SS handling for signals delivered to 64-bit
    programs
  x86_64,signal: Remove 'fs' and 'gs' from sigcontext

 arch/x86/include/asm/sigcontext.h      |  6 +++---
 arch/x86/include/uapi/asm/sigcontext.h |  6 +++---
 arch/x86/kernel/signal.c               | 24 +++++++++++++-----------
 3 files changed, 19 insertions(+), 17 deletions(-)

-- 
2.3.0

[PATCH v2 1/2] x86_64,signal: Fix SS handling for signals delivered to 64-bit programs

[Andy Lutomirski]

The comment in the signal code says that apps can save/restore other
segments on their own.  It's true that apps can *save* SS on their
own, but there's no way for apps to restore it: SYSCALL effectively
resets SS to __USER_DS, so any value that user code tries to load
into SS gets lost on entry to sigreturn.

This recycles two padding bytes in the segment selector area for SS.

While we're at it, we need a second change to make this useful.  If
the signal we're delivering is caused by a bad SS value, saving that
value isn't enough.  We need to remove that bad value from the regs
before we try to deliver the signal.  Oddly, x32 already got this
right.

I suspect that 64-bit programs that try to run 16-bit code and uses
signals will have a lot of trouble without this.

Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 arch/x86/include/asm/sigcontext.h      |  2 +-
 arch/x86/include/uapi/asm/sigcontext.h |  2 +-
 arch/x86/kernel/signal.c               | 20 +++++++++++---------
 3 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/arch/x86/include/asm/sigcontext.h b/arch/x86/include/asm/sigcontext.h
index 9dfce4e0417d..f910cdcb71fd 100644
--- a/arch/x86/include/asm/sigcontext.h
+++ b/arch/x86/include/asm/sigcontext.h
@@ -59,7 +59,7 @@ struct sigcontext {
 	unsigned short cs;
 	unsigned short gs;
 	unsigned short fs;
-	unsigned short __pad0;
+	unsigned short ss;
 	unsigned long err;
 	unsigned long trapno;
 	unsigned long oldmask;
diff --git a/arch/x86/include/uapi/asm/sigcontext.h b/arch/x86/include/uapi/asm/sigcontext.h
index d8b9f9081e86..076b11fd6fa1 100644
--- a/arch/x86/include/uapi/asm/sigcontext.h
+++ b/arch/x86/include/uapi/asm/sigcontext.h
@@ -179,7 +179,7 @@ struct sigcontext {
 	__u16 cs;
 	__u16 gs;
 	__u16 fs;
-	__u16 __pad0;
+	__u16 ss;
 	__u64 err;
 	__u64 trapno;
 	__u64 oldmask;
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index ed37a768d0fc..40f34574fb36 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -94,15 +94,8 @@ int restore_sigcontext(struct pt_regs *regs, struct sigcontext __user *sc,
 		COPY(r15);
 #endif /* CONFIG_X86_64 */
 
-#ifdef CONFIG_X86_32
 		COPY_SEG_CPL3(cs);
 		COPY_SEG_CPL3(ss);
-#else /* !CONFIG_X86_32 */
-		/* Kernel saves and restores only the CS segment register on signals,
-		 * which is the bare minimum needed to allow mixed 32/64-bit code.
-		 * App's signal handler can save/restore other segments if needed. */
-		COPY_SEG_CPL3(cs);
-#endif /* CONFIG_X86_32 */
 
 		get_user_ex(tmpflags, &sc->flags);
 		regs->flags = (regs->flags & ~FIX_EFLAGS) | (tmpflags & FIX_EFLAGS);
@@ -164,6 +157,7 @@ int setup_sigcontext(struct sigcontext __user *sc, void __user *fpstate,
 		put_user_ex(regs->cs, &sc->cs);
 		put_user_ex(0, &sc->gs);
 		put_user_ex(0, &sc->fs);
+		put_user_ex(regs->ss, &sc->ss);
 #endif /* CONFIG_X86_32 */
 
 		put_user_ex(fpstate, &sc->fpstate);
@@ -457,9 +451,17 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig,
 
 	regs->sp = (unsigned long)frame;
 
-	/* Set up the CS register to run signal handlers in 64-bit mode,
-	   even if the handler happens to be interrupting 32-bit code. */
+	/* Set up the CS and SS registers to run signal handlers in
+	   64-bit mode, even if the handler happens to be interrupting
+	   32-bit or 16-bit code.
+
+	   SS is subtle.  In 64-bit mode, we don't need any particular
+	   SS descriptor, but we do need SS to be valid.  It's possible
+	   that the old SS is entirely bogus -- this can happen if the
+	   signal we're trying to deliver is #GP or #SS caused by a bad
+	   SS value. */
 	regs->cs = __USER_CS;
+	regs->ss = __USER_DS;
 
 	return 0;
 }
-- 
2.3.0

[PATCH v2 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext

[Andy Lutomirski]

As far as I can tell, these fields have been set to zero on save and
ignored on restore since Linux was imported into git.  Rename them
'__pad1' and '__pad2' to avoid confusion and to allow them to be
recycled some day.

I'm intentionally avoiding calling either of them __pad0: the field
formerly known as __pad0 is now ss.

Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 arch/x86/include/asm/sigcontext.h      | 4 ++--
 arch/x86/include/uapi/asm/sigcontext.h | 4 ++--
 arch/x86/kernel/signal.c               | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/sigcontext.h b/arch/x86/include/asm/sigcontext.h
index f910cdcb71fd..5f0ef11719e1 100644
--- a/arch/x86/include/asm/sigcontext.h
+++ b/arch/x86/include/asm/sigcontext.h
@@ -57,8 +57,8 @@ struct sigcontext {
 	unsigned long ip;
 	unsigned long flags;
 	unsigned short cs;
-	unsigned short gs;
-	unsigned short fs;
+	unsigned short __pad2;	/* Was called gs, but was always zero. */
+	unsigned short __pad1;	/* Was called gs, but was always zero. */
 	unsigned short ss;
 	unsigned long err;
 	unsigned long trapno;
diff --git a/arch/x86/include/uapi/asm/sigcontext.h b/arch/x86/include/uapi/asm/sigcontext.h
index 076b11fd6fa1..df9908b1aa95 100644
--- a/arch/x86/include/uapi/asm/sigcontext.h
+++ b/arch/x86/include/uapi/asm/sigcontext.h
@@ -177,8 +177,8 @@ struct sigcontext {
 	__u64 rip;
 	__u64 eflags;		/* RFLAGS */
 	__u16 cs;
-	__u16 gs;
-	__u16 fs;
+	__u16 __pad2;		/* Was called gs, but was always zero. */
+	__u16 __pad1;		/* Was called fs, but was always zero. */
 	__u16 ss;
 	__u64 err;
 	__u64 trapno;
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 40f34574fb36..01a53767823c 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -155,8 +155,8 @@ int setup_sigcontext(struct sigcontext __user *sc, void __user *fpstate,
 #else /* !CONFIG_X86_32 */
 		put_user_ex(regs->flags, &sc->flags);
 		put_user_ex(regs->cs, &sc->cs);
-		put_user_ex(0, &sc->gs);
-		put_user_ex(0, &sc->fs);
+		put_user_ex(0, &sc->__pad2);
+		put_user_ex(0, &sc->__pad1);
 		put_user_ex(regs->ss, &sc->ss);
 #endif /* CONFIG_X86_32 */
 
-- 
2.3.0

Re: [PATCH v2 0/2] x86_64: Sigcontext improvements

[Andy Lutomirski]

Sorry, forgot to CC Oleg and Borislav.  Let me know if you want me to
forward the rest of the thread to you.

On Tue, Mar 10, 2015 at 7:03 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> Patch 1 is IMO a bug fix.
>
> Patch 2 is a cleanup and avoids some confusion.  It's also sort of an
> API break (no ABI change) because it removes a struct field.  But that
> struct field has never done anything at all as far as I can tell.
>
> AFAICT this series got bogged down in discussion about patch 2 last
> time.  I'm only including patch 2 for completeness here -- patch 1
> is IMO far more useful, and I'm fine with patch 1 being merged and
> tabling patch 2 again.
>
> Changes from v1:
>  - Add the __USER_DS fix, caught by my sigreturn test.
>  - Re-tested locally.
>
> Andy Lutomirski (2):
>   x86_64,signal: Fix SS handling for signals delivered to 64-bit
>     programs
>   x86_64,signal: Remove 'fs' and 'gs' from sigcontext
>
>  arch/x86/include/asm/sigcontext.h      |  6 +++---
>  arch/x86/include/uapi/asm/sigcontext.h |  6 +++---
>  arch/x86/kernel/signal.c               | 24 +++++++++++++-----------
>  3 files changed, 19 insertions(+), 17 deletions(-)
>
> --
> 2.3.0
>



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH v2 1/2] x86_64,signal: Fix SS handling for signals delivered to 64-bit programs

[Andy Lutomirski]

[cc: Oleg, Borislav]

On Tue, Mar 10, 2015 at 7:03 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> The comment in the signal code says that apps can save/restore other
> segments on their own.  It's true that apps can *save* SS on their
> own, but there's no way for apps to restore it: SYSCALL effectively
> resets SS to __USER_DS, so any value that user code tries to load
> into SS gets lost on entry to sigreturn.
>
> This recycles two padding bytes in the segment selector area for SS.
>
> While we're at it, we need a second change to make this useful.  If
> the signal we're delivering is caused by a bad SS value, saving that
> value isn't enough.  We need to remove that bad value from the regs
> before we try to deliver the signal.  Oddly, x32 already got this
> right.
>
> I suspect that 64-bit programs that try to run 16-bit code and uses
> signals will have a lot of trouble without this.
>
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> ---
>  arch/x86/include/asm/sigcontext.h      |  2 +-
>  arch/x86/include/uapi/asm/sigcontext.h |  2 +-
>  arch/x86/kernel/signal.c               | 20 +++++++++++---------
>  3 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/arch/x86/include/asm/sigcontext.h b/arch/x86/include/asm/sigcontext.h
> index 9dfce4e0417d..f910cdcb71fd 100644
> --- a/arch/x86/include/asm/sigcontext.h
> +++ b/arch/x86/include/asm/sigcontext.h
> @@ -59,7 +59,7 @@ struct sigcontext {
>         unsigned short cs;
>         unsigned short gs;
>         unsigned short fs;
> -       unsigned short __pad0;
> +       unsigned short ss;
>         unsigned long err;
>         unsigned long trapno;
>         unsigned long oldmask;
> diff --git a/arch/x86/include/uapi/asm/sigcontext.h b/arch/x86/include/uapi/asm/sigcontext.h
> index d8b9f9081e86..076b11fd6fa1 100644
> --- a/arch/x86/include/uapi/asm/sigcontext.h
> +++ b/arch/x86/include/uapi/asm/sigcontext.h
> @@ -179,7 +179,7 @@ struct sigcontext {
>         __u16 cs;
>         __u16 gs;
>         __u16 fs;
> -       __u16 __pad0;
> +       __u16 ss;
>         __u64 err;
>         __u64 trapno;
>         __u64 oldmask;
> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> index ed37a768d0fc..40f34574fb36 100644
> --- a/arch/x86/kernel/signal.c
> +++ b/arch/x86/kernel/signal.c
> @@ -94,15 +94,8 @@ int restore_sigcontext(struct pt_regs *regs, struct sigcontext __user *sc,
>                 COPY(r15);
>  #endif /* CONFIG_X86_64 */
>
> -#ifdef CONFIG_X86_32
>                 COPY_SEG_CPL3(cs);
>                 COPY_SEG_CPL3(ss);
> -#else /* !CONFIG_X86_32 */
> -               /* Kernel saves and restores only the CS segment register on signals,
> -                * which is the bare minimum needed to allow mixed 32/64-bit code.
> -                * App's signal handler can save/restore other segments if needed. */
> -               COPY_SEG_CPL3(cs);
> -#endif /* CONFIG_X86_32 */
>
>                 get_user_ex(tmpflags, &sc->flags);
>                 regs->flags = (regs->flags & ~FIX_EFLAGS) | (tmpflags & FIX_EFLAGS);
> @@ -164,6 +157,7 @@ int setup_sigcontext(struct sigcontext __user *sc, void __user *fpstate,
>                 put_user_ex(regs->cs, &sc->cs);
>                 put_user_ex(0, &sc->gs);
>                 put_user_ex(0, &sc->fs);
> +               put_user_ex(regs->ss, &sc->ss);
>  #endif /* CONFIG_X86_32 */
>
>                 put_user_ex(fpstate, &sc->fpstate);
> @@ -457,9 +451,17 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig,
>
>         regs->sp = (unsigned long)frame;
>
> -       /* Set up the CS register to run signal handlers in 64-bit mode,
> -          even if the handler happens to be interrupting 32-bit code. */
> +       /* Set up the CS and SS registers to run signal handlers in
> +          64-bit mode, even if the handler happens to be interrupting
> +          32-bit or 16-bit code.
> +
> +          SS is subtle.  In 64-bit mode, we don't need any particular
> +          SS descriptor, but we do need SS to be valid.  It's possible
> +          that the old SS is entirely bogus -- this can happen if the
> +          signal we're trying to deliver is #GP or #SS caused by a bad
> +          SS value. */
>         regs->cs = __USER_CS;
> +       regs->ss = __USER_DS;
>
>         return 0;
>  }
> --
> 2.3.0
>



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH v2 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext

[Andy Lutomirski]

[cc: Oleg, Borislav]

On Tue, Mar 10, 2015 at 7:03 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> As far as I can tell, these fields have been set to zero on save and
> ignored on restore since Linux was imported into git.  Rename them
> '__pad1' and '__pad2' to avoid confusion and to allow them to be
> recycled some day.
>
> I'm intentionally avoiding calling either of them __pad0: the field
> formerly known as __pad0 is now ss.
>
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> ---
>  arch/x86/include/asm/sigcontext.h      | 4 ++--
>  arch/x86/include/uapi/asm/sigcontext.h | 4 ++--
>  arch/x86/kernel/signal.c               | 4 ++--
>  3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/include/asm/sigcontext.h b/arch/x86/include/asm/sigcontext.h
> index f910cdcb71fd..5f0ef11719e1 100644
> --- a/arch/x86/include/asm/sigcontext.h
> +++ b/arch/x86/include/asm/sigcontext.h
> @@ -57,8 +57,8 @@ struct sigcontext {
>         unsigned long ip;
>         unsigned long flags;
>         unsigned short cs;
> -       unsigned short gs;
> -       unsigned short fs;
> +       unsigned short __pad2;  /* Was called gs, but was always zero. */
> +       unsigned short __pad1;  /* Was called gs, but was always zero. */
>         unsigned short ss;
>         unsigned long err;
>         unsigned long trapno;
> diff --git a/arch/x86/include/uapi/asm/sigcontext.h b/arch/x86/include/uapi/asm/sigcontext.h
> index 076b11fd6fa1..df9908b1aa95 100644
> --- a/arch/x86/include/uapi/asm/sigcontext.h
> +++ b/arch/x86/include/uapi/asm/sigcontext.h
> @@ -177,8 +177,8 @@ struct sigcontext {
>         __u64 rip;
>         __u64 eflags;           /* RFLAGS */
>         __u16 cs;
> -       __u16 gs;
> -       __u16 fs;
> +       __u16 __pad2;           /* Was called gs, but was always zero. */
> +       __u16 __pad1;           /* Was called fs, but was always zero. */
>         __u16 ss;
>         __u64 err;
>         __u64 trapno;
> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> index 40f34574fb36..01a53767823c 100644
> --- a/arch/x86/kernel/signal.c
> +++ b/arch/x86/kernel/signal.c
> @@ -155,8 +155,8 @@ int setup_sigcontext(struct sigcontext __user *sc, void __user *fpstate,
>  #else /* !CONFIG_X86_32 */
>                 put_user_ex(regs->flags, &sc->flags);
>                 put_user_ex(regs->cs, &sc->cs);
> -               put_user_ex(0, &sc->gs);
> -               put_user_ex(0, &sc->fs);
> +               put_user_ex(0, &sc->__pad2);
> +               put_user_ex(0, &sc->__pad1);
>                 put_user_ex(regs->ss, &sc->ss);
>  #endif /* CONFIG_X86_32 */
>
> --
> 2.3.0
>



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH v2 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext

[John Stoffel]

>>>>> "Andy" == Andy Lutomirski <luto@amacapital.net> writes:

Andy> As far as I can tell, these fields have been set to zero on save and
Andy> ignored on restore since Linux was imported into git.  Rename them
Andy> '__pad1' and '__pad2' to avoid confusion and to allow them to be
Andy> recycled some day.

Andy> I'm intentionally avoiding calling either of them __pad0: the field
Andy> formerly known as __pad0 is now ss.

Andy> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
Andy> ---
Andy>  arch/x86/include/asm/sigcontext.h      | 4 ++--
Andy>  arch/x86/include/uapi/asm/sigcontext.h | 4 ++--
Andy>  arch/x86/kernel/signal.c               | 4 ++--
Andy>  3 files changed, 6 insertions(+), 6 deletions(-)

Andy> diff --git a/arch/x86/include/asm/sigcontext.h b/arch/x86/include/asm/sigcontext.h
Andy> index f910cdcb71fd..5f0ef11719e1 100644
Andy> --- a/arch/x86/include/asm/sigcontext.h
Andy> +++ b/arch/x86/include/asm/sigcontext.h
Andy> @@ -57,8 +57,8 @@ struct sigcontext {
Andy>  	unsigned long ip;
Andy>  	unsigned long flags;
Andy>  	unsigned short cs;
Andy> -	unsigned short gs;
Andy> -	unsigned short fs;
Andy> +	unsigned short __pad2;	/* Was called gs, but was always zero. */
Andy> +	unsigned short __pad1;	/* Was called gs, but was always zero. */

Shouldn't this comment read:

  /* Was called fs, but was always zero. */

for the second one?

Re: [PATCH v2 1/2] x86_64,signal: Fix SS handling for signals delivered to 64-bit programs

[Oleg Nesterov]

Well, the patch looks "obviously fine" to me, but this is all I can say.

I mean, I simply can't understand this __pad0/ifdef(CONFIG_X86_32), it
looks as if ->ss was specially excluded for unknown reason from the very
beginning.

On 03/10, Andy Lutomirski wrote:
>
> > --- a/arch/x86/include/uapi/asm/sigcontext.h
> > +++ b/arch/x86/include/uapi/asm/sigcontext.h
> > @@ -179,7 +179,7 @@ struct sigcontext {
> >         __u16 cs;
> >         __u16 gs;
> >         __u16 fs;
> > -       __u16 __pad0;
> > +       __u16 ss;

I do not know the rules for include/uapi/ ...

OK, nobody should ever use __pad0, so probably it is safe to rename it.
OTOH, an application can (say) try to print all members for debugging
purposes, it won't compile after this change.

Oleg.

Re: [PATCH v2 1/2] x86_64,signal: Fix SS handling for signals delivered to 64-bit programs

[Andy Lutomirski]

On Tue, Mar 10, 2015 at 9:42 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> Well, the patch looks "obviously fine" to me, but this is all I can say.
>
> I mean, I simply can't understand this __pad0/ifdef(CONFIG_X86_32), it
> looks as if ->ss was specially excluded for unknown reason from the very
> beginning.

I assume it was confusion in the very early days of x86_64.  Andi?

>
> On 03/10, Andy Lutomirski wrote:
>>
>> > --- a/arch/x86/include/uapi/asm/sigcontext.h
>> > +++ b/arch/x86/include/uapi/asm/sigcontext.h
>> > @@ -179,7 +179,7 @@ struct sigcontext {
>> >         __u16 cs;
>> >         __u16 gs;
>> >         __u16 fs;
>> > -       __u16 __pad0;
>> > +       __u16 ss;
>
> I do not know the rules for include/uapi/ ...
>
> OK, nobody should ever use __pad0, so probably it is safe to rename it.
> OTOH, an application can (say) try to print all members for debugging
> purposes, it won't compile after this change.

Yeah, I don't really know what the rules are.

--Andy

Re: [PATCH v2 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext

[Andy Lutomirski]

On Tue, Mar 10, 2015 at 9:16 AM, John Stoffel <john@stoffel.org> wrote:
>>>>>> "Andy" == Andy Lutomirski <luto@amacapital.net> writes:
>
> Andy> As far as I can tell, these fields have been set to zero on save and
> Andy> ignored on restore since Linux was imported into git.  Rename them
> Andy> '__pad1' and '__pad2' to avoid confusion and to allow them to be
> Andy> recycled some day.
>
> Andy> I'm intentionally avoiding calling either of them __pad0: the field
> Andy> formerly known as __pad0 is now ss.
>
> Andy> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> Andy> ---
> Andy>  arch/x86/include/asm/sigcontext.h      | 4 ++--
> Andy>  arch/x86/include/uapi/asm/sigcontext.h | 4 ++--
> Andy>  arch/x86/kernel/signal.c               | 4 ++--
> Andy>  3 files changed, 6 insertions(+), 6 deletions(-)
>
> Andy> diff --git a/arch/x86/include/asm/sigcontext.h b/arch/x86/include/asm/sigcontext.h
> Andy> index f910cdcb71fd..5f0ef11719e1 100644
> Andy> --- a/arch/x86/include/asm/sigcontext.h
> Andy> +++ b/arch/x86/include/asm/sigcontext.h
> Andy> @@ -57,8 +57,8 @@ struct sigcontext {
> Andy>   unsigned long ip;
> Andy>   unsigned long flags;
> Andy>   unsigned short cs;
> Andy> - unsigned short gs;
> Andy> - unsigned short fs;
> Andy> + unsigned short __pad2;  /* Was called gs, but was always zero. */
> Andy> + unsigned short __pad1;  /* Was called gs, but was always zero. */
>
> Shouldn't this comment read:
>
>   /* Was called fs, but was always zero. */
>
> for the second one?

Will fix for v2.  Thanks.

-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH v2 1/2] x86_64,signal: Fix SS handling for signals delivered to 64-bit programs

[Borislav Petkov]

On Tue, Mar 10, 2015 at 07:03:24AM -0700, Andy Lutomirski wrote:
> The comment in the signal code says that apps can save/restore other
> segments on their own.  It's true that apps can *save* SS on their
> own, but there's no way for apps to restore it: SYSCALL effectively
> resets SS to __USER_DS, so any value that user code tries to load
> into SS gets lost on entry to sigreturn.
> 
> This recycles two padding bytes in the segment selector area for SS.
> 
> While we're at it, we need a second change to make this useful.  If
> the signal we're delivering is caused by a bad SS value, saving that
> value isn't enough.  We need to remove that bad value from the regs
> before we try to deliver the signal.  Oddly, x32 already got this
> right.

Are we at least reporting the bad SS value when delivering the signal so
that userpsace knows why it got the signal?

> I suspect that 64-bit programs that try to run 16-bit code and uses
> signals will have a lot of trouble without this.

Do we even have software doing that? Maybe we should search for similar
bug reports...

> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> ---
>  arch/x86/include/asm/sigcontext.h      |  2 +-
>  arch/x86/include/uapi/asm/sigcontext.h |  2 +-
>  arch/x86/kernel/signal.c               | 20 +++++++++++---------
>  3 files changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/include/asm/sigcontext.h b/arch/x86/include/asm/sigcontext.h
> index 9dfce4e0417d..f910cdcb71fd 100644
> --- a/arch/x86/include/asm/sigcontext.h
> +++ b/arch/x86/include/asm/sigcontext.h
> @@ -59,7 +59,7 @@ struct sigcontext {
>  	unsigned short cs;
>  	unsigned short gs;
>  	unsigned short fs;
> -	unsigned short __pad0;

This __pad0 thing has been there since the beginning, according to my
git history dive.

> +	unsigned short ss;
>  	unsigned long err;
>  	unsigned long trapno;
>  	unsigned long oldmask;
> diff --git a/arch/x86/include/uapi/asm/sigcontext.h b/arch/x86/include/uapi/asm/sigcontext.h
> index d8b9f9081e86..076b11fd6fa1 100644
> --- a/arch/x86/include/uapi/asm/sigcontext.h
> +++ b/arch/x86/include/uapi/asm/sigcontext.h
> @@ -179,7 +179,7 @@ struct sigcontext {
>  	__u16 cs;
>  	__u16 gs;
>  	__u16 fs;
> -	__u16 __pad0;
> +	__u16 ss;
>  	__u64 err;
>  	__u64 trapno;
>  	__u64 oldmask;
> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> index ed37a768d0fc..40f34574fb36 100644
> --- a/arch/x86/kernel/signal.c
> +++ b/arch/x86/kernel/signal.c
> @@ -94,15 +94,8 @@ int restore_sigcontext(struct pt_regs *regs, struct sigcontext __user *sc,
>  		COPY(r15);
>  #endif /* CONFIG_X86_64 */
>  
> -#ifdef CONFIG_X86_32
>  		COPY_SEG_CPL3(cs);
>  		COPY_SEG_CPL3(ss);
> -#else /* !CONFIG_X86_32 */
> -		/* Kernel saves and restores only the CS segment register on signals,
> -		 * which is the bare minimum needed to allow mixed 32/64-bit code.
> -		 * App's signal handler can save/restore other segments if needed. */
> -		COPY_SEG_CPL3(cs);
> -#endif /* CONFIG_X86_32 */
>  
>  		get_user_ex(tmpflags, &sc->flags);
>  		regs->flags = (regs->flags & ~FIX_EFLAGS) | (tmpflags & FIX_EFLAGS);
> @@ -164,6 +157,7 @@ int setup_sigcontext(struct sigcontext __user *sc, void __user *fpstate,
>  		put_user_ex(regs->cs, &sc->cs);
>  		put_user_ex(0, &sc->gs);
>  		put_user_ex(0, &sc->fs);
> +		put_user_ex(regs->ss, &sc->ss);
>  #endif /* CONFIG_X86_32 */
>  
>  		put_user_ex(fpstate, &sc->fpstate);
> @@ -457,9 +451,17 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig,
>  
>  	regs->sp = (unsigned long)frame;
>  
> -	/* Set up the CS register to run signal handlers in 64-bit mode,
> -	   even if the handler happens to be interrupting 32-bit code. */
> +	/* Set up the CS and SS registers to run signal handlers in
> +	   64-bit mode, even if the handler happens to be interrupting
> +	   32-bit or 16-bit code.
> +
> +	   SS is subtle.  In 64-bit mode, we don't need any particular
> +	   SS descriptor, but we do need SS to be valid.  It's possible
> +	   that the old SS is entirely bogus -- this can happen if the
> +	   signal we're trying to deliver is #GP or #SS caused by a bad
> +	   SS value. */

Kernel comment style please:

	/*
	 * Andy likes to go and play 16-bit games on 64-bit linux. We all are
	 * having lotsa fun.
	 */

:-D

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH v2 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext

[Borislav Petkov]

On Tue, Mar 10, 2015 at 07:03:25AM -0700, Andy Lutomirski wrote:
> As far as I can tell, these fields have been set to zero on save and
> ignored on restore since Linux was imported into git.  Rename them

Actually from the very beginning:

commit 47f16da277d10ef9494f3e9da2a9113bb22bcd75
Author: Andi Kleen <ak@muc.de>
Date:   Tue Feb 12 20:17:35 2002 -0800

    [PATCH] x86_64 merge: arch + asm

...

+static int
+setup_sigcontext(struct sigcontext *sc, struct _fpstate *fpstate,
+                struct pt_regs *regs, unsigned long mask)
+{
+       int tmp, err = 0;
+
+       tmp = 0;
+       __asm__("movl %%gs,%0" : "=r"(tmp): "0"(tmp));
+       err |= __put_user(tmp, (unsigned int *)&sc->gs);
+       __asm__("movl %%fs,%0" : "=r"(tmp): "0"(tmp));
+       err |= __put_user(tmp, (unsigned int *)&sc->fs);


> '__pad1' and '__pad2' to avoid confusion and to allow them to be
> recycled some day.
> 
> I'm intentionally avoiding calling either of them __pad0: the field
> formerly known as __pad0 is now ss.
> 
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>

Acked-by: Borislav Petkov <bp@suse.de>

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH v2 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext

[Andy Lutomirski]

On Mar 11, 2015 5:29 AM, "Borislav Petkov" <bp@alien8.de> wrote:
>
> On Tue, Mar 10, 2015 at 07:03:25AM -0700, Andy Lutomirski wrote:
> > As far as I can tell, these fields have been set to zero on save and
> > ignored on restore since Linux was imported into git.  Rename them
>
> Actually from the very beginning:
>
> commit 47f16da277d10ef9494f3e9da2a9113bb22bcd75
> Author: Andi Kleen <ak@muc.de>
> Date:   Tue Feb 12 20:17:35 2002 -0800
>
>     [PATCH] x86_64 merge: arch + asm
>
> ...
>
> +static int
> +setup_sigcontext(struct sigcontext *sc, struct _fpstate *fpstate,
> +                struct pt_regs *regs, unsigned long mask)
> +{
> +       int tmp, err = 0;
> +
> +       tmp = 0;
> +       __asm__("movl %%gs,%0" : "=r"(tmp): "0"(tmp));
> +       err |= __put_user(tmp, (unsigned int *)&sc->gs);
> +       __asm__("movl %%fs,%0" : "=r"(tmp): "0"(tmp));
> +       err |= __put_user(tmp, (unsigned int *)&sc->fs);
>

Wait... It looks like it really was saving them.  Of course, this is
mostly useless unless the code also does something very clever with
fsbase and gsbase.

After wrestling with the historic git a bit:

commit a104ba57b3b3697fd77778918b6a77ed16bb2ff8
Author: Andi Kleen <ak@muc.de>
Date:   Mon Mar 10 01:41:56 2003 -0800

    [PATCH] x86-64 updates for 2.5.64-bk3

...

 static inline int
 setup_sigcontext(struct sigcontext *sc, struct pt_regs *regs,
unsigned long mask, struct task_struct *me)
 {
-       int tmp, err = 0;
+       int err = 0;

-       tmp = 0;
-       __asm__("movl %%gs,%0" : "=r"(tmp): "0"(tmp));
-       err |= __put_user(tmp, (unsigned int *)&sc->gs);
-       __asm__("movl %%fs,%0" : "=r"(tmp): "0"(tmp));
-       err |= __put_user(tmp, (unsigned int *)&sc->fs);
+       err |= __put_user(0, &sc->gs);
+       err |= __put_user(0, &sc->fs);

Prior to that, we actually saved and restored the thing.  In restore_context:

        {
                unsigned int seg;
                err |= __get_user(seg, &sc->gs);
                load_gs_index(seg);
                err |= __get_user(seg, &sc->fs);
                loadsegment(fs,seg);
        }

We did not, however, save and restore fsbase and gsbase.

Interestingly, in the same BK change, we also remove set_thread_area
as a 64-bit syscall entirely (take that, ABI compatibility!), but
arch_prctl existed before that change.

IOW, in 2.5.63 and earlier, we tried to save and restore TLS state
across signals, but we did it wrong and would corrupt it for any
program that used arch_prctl.  At that time, programs could switch
userspace threads using signals and their TLS pointers would switch.
In 2.5.64 and later, we broke set_thread_area users, fixed arch_prctl
users, and made signals effectively *not* switch TLS pointers.

It seems unlikely to me that many pre-2.5.64 programs still run due to
the aforementioned removal of a syscall, but I think that we should at
least document this in the header so that anyone who tries to recycle
those padding bits is appropriately careful.

--Andy

Re: [PATCH v2 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext

[Borislav Petkov]

On Wed, Mar 11, 2015 at 05:22:48AM -0700, Andy Lutomirski wrote:
> Wait... It looks like it really was saving them.  Of course, this is

Bah, of course. Blind me.

> IOW, in 2.5.63 and earlier, we tried to save and restore TLS state
> across signals, but we did it wrong and would corrupt it for any
> program that used arch_prctl.  At that time, programs could switch
> userspace threads using signals and their TLS pointers would switch.
> In 2.5.64 and later, we broke set_thread_area users, fixed arch_prctl

That is probably non-issue as 2.5 was the devel branch anyway in the old
days. I'm not sure what the statement on breaking ABI was though then...

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH v2 1/2] x86_64,signal: Fix SS handling for signals delivered to 64-bit programs

[Andy Lutomirski]

On Mar 11, 2015 4:32 AM, "Borislav Petkov" <bp@alien8.de> wrote:
>
> On Tue, Mar 10, 2015 at 07:03:24AM -0700, Andy Lutomirski wrote:
> > The comment in the signal code says that apps can save/restore other
> > segments on their own.  It's true that apps can *save* SS on their
> > own, but there's no way for apps to restore it: SYSCALL effectively
> > resets SS to __USER_DS, so any value that user code tries to load
> > into SS gets lost on entry to sigreturn.
> >
> > This recycles two padding bytes in the segment selector area for SS.
> >
> > While we're at it, we need a second change to make this useful.  If
> > the signal we're delivering is caused by a bad SS value, saving that
> > value isn't enough.  We need to remove that bad value from the regs
> > before we try to deliver the signal.  Oddly, x32 already got this
> > right.
>
> Are we at least reporting the bad SS value when delivering the signal so
> that userpsace knows why it got the signal?

Yes, more or less.  We'll send sigsegv with an appropriate trapnr.
Without this fix, though, we fail to deliver that signal and kill the
process.

>
> > I suspect that 64-bit programs that try to run 16-bit code and uses
> > signals will have a lot of trouble without this.
>
> Do we even have software doing that? Maybe we should search for similar
> bug reports...
>
> > Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> > ---
> >  arch/x86/include/asm/sigcontext.h      |  2 +-
> >  arch/x86/include/uapi/asm/sigcontext.h |  2 +-
> >  arch/x86/kernel/signal.c               | 20 +++++++++++---------
> >  3 files changed, 13 insertions(+), 11 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/sigcontext.h b/arch/x86/include/asm/sigcontext.h
> > index 9dfce4e0417d..f910cdcb71fd 100644
> > --- a/arch/x86/include/asm/sigcontext.h
> > +++ b/arch/x86/include/asm/sigcontext.h
> > @@ -59,7 +59,7 @@ struct sigcontext {
> >       unsigned short cs;
> >       unsigned short gs;
> >       unsigned short fs;
> > -     unsigned short __pad0;
>
> This __pad0 thing has been there since the beginning, according to my
> git history dive.
>
> > +     unsigned short ss;
> >       unsigned long err;
> >       unsigned long trapno;
> >       unsigned long oldmask;
> > diff --git a/arch/x86/include/uapi/asm/sigcontext.h b/arch/x86/include/uapi/asm/sigcontext.h
> > index d8b9f9081e86..076b11fd6fa1 100644
> > --- a/arch/x86/include/uapi/asm/sigcontext.h
> > +++ b/arch/x86/include/uapi/asm/sigcontext.h
> > @@ -179,7 +179,7 @@ struct sigcontext {
> >       __u16 cs;
> >       __u16 gs;
> >       __u16 fs;
> > -     __u16 __pad0;
> > +     __u16 ss;
> >       __u64 err;
> >       __u64 trapno;
> >       __u64 oldmask;
> > diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> > index ed37a768d0fc..40f34574fb36 100644
> > --- a/arch/x86/kernel/signal.c
> > +++ b/arch/x86/kernel/signal.c
> > @@ -94,15 +94,8 @@ int restore_sigcontext(struct pt_regs *regs, struct sigcontext __user *sc,
> >               COPY(r15);
> >  #endif /* CONFIG_X86_64 */
> >
> > -#ifdef CONFIG_X86_32
> >               COPY_SEG_CPL3(cs);
> >               COPY_SEG_CPL3(ss);
> > -#else /* !CONFIG_X86_32 */
> > -             /* Kernel saves and restores only the CS segment register on signals,
> > -              * which is the bare minimum needed to allow mixed 32/64-bit code.
> > -              * App's signal handler can save/restore other segments if needed. */
> > -             COPY_SEG_CPL3(cs);
> > -#endif /* CONFIG_X86_32 */
> >
> >               get_user_ex(tmpflags, &sc->flags);
> >               regs->flags = (regs->flags & ~FIX_EFLAGS) | (tmpflags & FIX_EFLAGS);
> > @@ -164,6 +157,7 @@ int setup_sigcontext(struct sigcontext __user *sc, void __user *fpstate,
> >               put_user_ex(regs->cs, &sc->cs);
> >               put_user_ex(0, &sc->gs);
> >               put_user_ex(0, &sc->fs);
> > +             put_user_ex(regs->ss, &sc->ss);
> >  #endif /* CONFIG_X86_32 */
> >
> >               put_user_ex(fpstate, &sc->fpstate);
> > @@ -457,9 +451,17 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig,
> >
> >       regs->sp = (unsigned long)frame;
> >
> > -     /* Set up the CS register to run signal handlers in 64-bit mode,
> > -        even if the handler happens to be interrupting 32-bit code. */
> > +     /* Set up the CS and SS registers to run signal handlers in
> > +        64-bit mode, even if the handler happens to be interrupting
> > +        32-bit or 16-bit code.
> > +
> > +        SS is subtle.  In 64-bit mode, we don't need any particular
> > +        SS descriptor, but we do need SS to be valid.  It's possible
> > +        that the old SS is entirely bogus -- this can happen if the
> > +        signal we're trying to deliver is #GP or #SS caused by a bad
> > +        SS value. */
>
> Kernel comment style please:
>
>         /*
>          * Andy likes to go and play 16-bit games on 64-bit linux. We all are
>          * having lotsa fun.
>          */
>
> :-D

I did it this way for consistency with every other comment in that
function.  I'll change it.



>
> --
> Regards/Gruss,
>     Boris.
>
> ECO tip #101: Trim your mails when you reply.
> --