LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 0/2] x86_64,signal: Fix the set of saved segment registers
@ 2014-07-11 16:29 Andy Lutomirski
  2014-07-11 16:29 ` [PATCH 1/2] x86_64,signal: Save and restore SS in signal frames Andy Lutomirski
  2014-07-11 16:29 ` [PATCH 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext Andy Lutomirski
  0 siblings, 2 replies; 16+ messages in thread
From: Andy Lutomirski @ 2014-07-11 16:29 UTC (permalink / raw)
  To: H. Peter Anvin, Andi Kleen; +Cc: Al Viro, x86, linux-kernel, Andy Lutomirski

The x86_64 signal code claims to save and restore CS, FS, and GS,
and it further claims that this is the minimal set that's needed.

Neither of these claims is true.  The code does not, and AFAICT
never has, saved or restored FS and GS, nor does it need to.  On the
other hand, all 64-bit syscalls (and rt_sigreturn in particular)
clobber SS, making it impossible for signal handlers to correctly
restore SS without using a trampoline or ptracing themselves.

This patchset saves and restores SS in the __pad0 slot and renames
the FS and GS slots __pad1 and __pad2 to more accurately document
their purpose.

I discovered this while writing a test for espfix64.

Andy Lutomirski (2):
  x86_64,signal: Save and restore SS in signal frames
  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               | 12 +++---------
 3 files changed, 9 insertions(+), 15 deletions(-)

-- 
1.9.3


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/2] x86_64,signal: Save and restore SS in signal frames
  2014-07-11 16:29 [PATCH 0/2] x86_64,signal: Fix the set of saved segment registers Andy Lutomirski
@ 2014-07-11 16:29 ` Andy Lutomirski
  2015-03-09 20:15   ` Andy Lutomirski
  2014-07-11 16:29 ` [PATCH 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext Andy Lutomirski
  1 sibling, 1 reply; 16+ messages in thread
From: Andy Lutomirski @ 2014-07-11 16:29 UTC (permalink / raw)
  To: H. Peter Anvin, Andi Kleen; +Cc: Al Viro, x86, linux-kernel, 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.

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               | 8 +-------
 3 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/arch/x86/include/asm/sigcontext.h b/arch/x86/include/asm/sigcontext.h
index 9dfce4e..f910cdc 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 d8b9f90..076b11f 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 2851d63..08d7a9e 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);
-- 
1.9.3


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext
  2014-07-11 16:29 [PATCH 0/2] x86_64,signal: Fix the set of saved segment registers Andy Lutomirski
  2014-07-11 16:29 ` [PATCH 1/2] x86_64,signal: Save and restore SS in signal frames Andy Lutomirski
@ 2014-07-11 16:29 ` Andy Lutomirski
  2014-07-11 18:12   ` Andi Kleen
                     ` (2 more replies)
  1 sibling, 3 replies; 16+ messages in thread
From: Andy Lutomirski @ 2014-07-11 16:29 UTC (permalink / raw)
  To: H. Peter Anvin, Andi Kleen; +Cc: Al Viro, x86, linux-kernel, 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 f910cdc..5f0ef11 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 076b11f..df9908b 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 08d7a9e..a1867ce 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 */
 
-- 
1.9.3


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext
  2014-07-11 16:29 ` [PATCH 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext Andy Lutomirski
@ 2014-07-11 18:12   ` Andi Kleen
  2014-07-11 18:39     ` Andy Lutomirski
  2014-07-12  2:04   ` H. Peter Anvin
  2014-07-12  2:21   ` Linus Torvalds
  2 siblings, 1 reply; 16+ messages in thread
From: Andi Kleen @ 2014-07-11 18:12 UTC (permalink / raw)
  To: Andy Lutomirski; +Cc: H. Peter Anvin, Andi Kleen, Al Viro, x86, linux-kernel

> diff --git a/arch/x86/include/uapi/asm/sigcontext.h b/arch/x86/include/uapi/asm/sigcontext.h
> index 076b11f..df9908b 100644
> --- a/arch/x86/include/uapi/asm/sigcontext.h
> +++ b/arch/x86/include/uapi/asm/sigcontext.h

I don't think renaming fields in uapi/asm is acceptable. These
are likely used by user programs and you'll break compiles.

-Andi

> @@ -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;

-- 
ak@linux.intel.com -- Speaking for myself only.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext
  2014-07-11 18:12   ` Andi Kleen
@ 2014-07-11 18:39     ` Andy Lutomirski
  2014-07-12  2:09       ` H. Peter Anvin
  0 siblings, 1 reply; 16+ messages in thread
From: Andy Lutomirski @ 2014-07-11 18:39 UTC (permalink / raw)
  To: Andi Kleen; +Cc: H. Peter Anvin, Al Viro, X86 ML, linux-kernel

On Fri, Jul 11, 2014 at 11:12 AM, Andi Kleen <andi@firstfloor.org> wrote:
>> diff --git a/arch/x86/include/uapi/asm/sigcontext.h b/arch/x86/include/uapi/asm/sigcontext.h
>> index 076b11f..df9908b 100644
>> --- a/arch/x86/include/uapi/asm/sigcontext.h
>> +++ b/arch/x86/include/uapi/asm/sigcontext.h
>
> I don't think renaming fields in uapi/asm is acceptable. These
> are likely used by user programs and you'll break compiles.

Hmm.  That's a fair point.  On the other hand, any user code that uses
these fields explicitly may already be broken, since the current names
of these fields rather strongly imply that they do something.

Is there any clear policy on minor API breaks in the UAPI headers that
don't affect ABI?

--Andy

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext
  2014-07-11 16:29 ` [PATCH 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext Andy Lutomirski
  2014-07-11 18:12   ` Andi Kleen
@ 2014-07-12  2:04   ` H. Peter Anvin
  2014-07-12  2:21   ` Linus Torvalds
  2 siblings, 0 replies; 16+ messages in thread
From: H. Peter Anvin @ 2014-07-12  2:04 UTC (permalink / raw)
  To: Andy Lutomirski, Andi Kleen; +Cc: Al Viro, x86, linux-kernel

On 07/11/2014 09:29 AM, Andy Lutomirski wrote:
> diff --git a/arch/x86/include/uapi/asm/sigcontext.h b/arch/x86/include/uapi/asm/sigcontext.h
> index 076b11f..df9908b 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;

I'm just wondering if this is likely to cause compile error in existing
code.  I guess worst case we can just revert this patch...

	-hpa


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext
  2014-07-11 18:39     ` Andy Lutomirski
@ 2014-07-12  2:09       ` H. Peter Anvin
  0 siblings, 0 replies; 16+ messages in thread
From: H. Peter Anvin @ 2014-07-12  2:09 UTC (permalink / raw)
  To: Andy Lutomirski, Andi Kleen; +Cc: Al Viro, X86 ML, linux-kernel, Linus Torvalds

On 07/11/2014 11:39 AM, Andy Lutomirski wrote:
> On Fri, Jul 11, 2014 at 11:12 AM, Andi Kleen <andi@firstfloor.org> wrote:
>>> diff --git a/arch/x86/include/uapi/asm/sigcontext.h b/arch/x86/include/uapi/asm/sigcontext.h
>>> index 076b11f..df9908b 100644
>>> --- a/arch/x86/include/uapi/asm/sigcontext.h
>>> +++ b/arch/x86/include/uapi/asm/sigcontext.h
>>
>> I don't think renaming fields in uapi/asm is acceptable. These
>> are likely used by user programs and you'll break compiles.
> 
> Hmm.  That's a fair point.  On the other hand, any user code that uses
> these fields explicitly may already be broken, since the current names
> of these fields rather strongly imply that they do something.
> 
> Is there any clear policy on minor API breaks in the UAPI headers that
> don't affect ABI?
> 

There really isn't, and this *definitely* a boundary case: as you state,
it is very likely that anyone currently using them are doing so
incorrectly, but it does induce potential source-level breakage.

Linus, do you have any guidance here?

	-hpa


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext
  2014-07-11 16:29 ` [PATCH 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext Andy Lutomirski
  2014-07-11 18:12   ` Andi Kleen
  2014-07-12  2:04   ` H. Peter Anvin
@ 2014-07-12  2:21   ` Linus Torvalds
  2014-07-12  2:26     ` H. Peter Anvin
  2014-07-12  8:39     ` Andy Lutomirski
  2 siblings, 2 replies; 16+ messages in thread
From: Linus Torvalds @ 2014-07-12  2:21 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: H. Peter Anvin, Andi Kleen, Al Viro, the arch/x86 maintainers,
	Linux Kernel Mailing List

On Fri, Jul 11, 2014 at 9:29 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.

Shouldn't we actually try to save/restore gs/fs properly? Admittedly,
changing them in the signal handler would be insane, but still.. See
our context switching code  with the whole segment selector vs base
save/restore code. Hmm?

            Linus

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext
  2014-07-12  2:21   ` Linus Torvalds
@ 2014-07-12  2:26     ` H. Peter Anvin
  2014-07-12  8:39     ` Andy Lutomirski
  1 sibling, 0 replies; 16+ messages in thread
From: H. Peter Anvin @ 2014-07-12  2:26 UTC (permalink / raw)
  To: Linus Torvalds, Andy Lutomirski
  Cc: Andi Kleen, Al Viro, the arch/x86 maintainers, Linux Kernel Mailing List

Our current handling of fs/vs is really weird.  The best may very well be to explicitly save and restore both fs and he as part of Andi's patchset to support wrxsbase.

On July 11, 2014 7:21:54 PM PDT, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>On Fri, Jul 11, 2014 at 9:29 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.
>
>Shouldn't we actually try to save/restore gs/fs properly? Admittedly,
>changing them in the signal handler would be insane, but still.. See
>our context switching code  with the whole segment selector vs base
>save/restore code. Hmm?
>
>            Linus

-- 
Sent from my mobile phone.  Please pardon brevity and lack of formatting.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext
  2014-07-12  2:21   ` Linus Torvalds
  2014-07-12  2:26     ` H. Peter Anvin
@ 2014-07-12  8:39     ` Andy Lutomirski
  2014-07-12 18:37       ` Andi Kleen
  1 sibling, 1 reply; 16+ messages in thread
From: Andy Lutomirski @ 2014-07-12  8:39 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: linux-kernel, Andi Kleen, H. Peter Anvin, Al Viro,
	the arch/x86 maintainers

On Jul 11, 2014 7:21 PM, "Linus Torvalds" <torvalds@linux-foundation.org> wrote:
>
> On Fri, Jul 11, 2014 at 9:29 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.
>
> Shouldn't we actually try to save/restore gs/fs properly? Admittedly,
> changing them in the signal handler would be insane, but still.. See
> our context switching code  with the whole segment selector vs base
> save/restore code. Hmm?

This seems like it's asking for trouble.  I think wxe'd have to
separately save the selectors and the base registers to avoid breaking
something, especially once wrgsbase, etc are enabled.

Why would this be needed anyway?

Does anyone implement makecontext, etc using raise/sigreturn?  If so,
they might be in for a surprise when their gs starts getting saved,
too.

Linus, for context, the other patch in this series saves and restores
SS.  Without that, 64-bit sigreturn to a nondefault stack segment is
basically impossible.  But I don't see why any other segments (besides
CS) are needed for 64-bit sigreturn.

--Andy

>
>             Linus

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext
  2014-07-12  8:39     ` Andy Lutomirski
@ 2014-07-12 18:37       ` Andi Kleen
  2014-07-12 18:40         ` H. Peter Anvin
  0 siblings, 1 reply; 16+ messages in thread
From: Andi Kleen @ 2014-07-12 18:37 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Linus Torvalds, linux-kernel, Andi Kleen, H. Peter Anvin,
	Al Viro, the arch/x86 maintainers

> This seems like it's asking for trouble.  I think wxe'd have to
> separately save the selectors and the base registers to avoid breaking
> something, especially once wrgsbase, etc are enabled.

I agree, it would likely break existing code.

> Linus, for context, the other patch in this series saves and restores
> SS. 

> Without that, 64-bit sigreturn to a nondefault stack segment is
> basically impossible.  

Why would you ever want to do that?

-Andi

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext
  2014-07-12 18:37       ` Andi Kleen
@ 2014-07-12 18:40         ` H. Peter Anvin
  2014-07-12 18:52           ` Andi Kleen
  0 siblings, 1 reply; 16+ messages in thread
From: H. Peter Anvin @ 2014-07-12 18:40 UTC (permalink / raw)
  To: Andi Kleen, Andy Lutomirski
  Cc: Linus Torvalds, linux-kernel, Al Viro, the arch/x86 maintainers

Because you are doing something weird (like Pin, for example) and take an asynchronous fault?

On July 12, 2014 11:37:48 AM PDT, Andi Kleen <andi@firstfloor.org> wrote:
>> This seems like it's asking for trouble.  I think wxe'd have to
>> separately save the selectors and the base registers to avoid
>breaking
>> something, especially once wrgsbase, etc are enabled.
>
>I agree, it would likely break existing code.
>
>> Linus, for context, the other patch in this series saves and restores
>> SS. 
>
>> Without that, 64-bit sigreturn to a nondefault stack segment is
>> basically impossible.  
>
>Why would you ever want to do that?
>
>-Andi

-- 
Sent from my mobile phone.  Please pardon brevity and lack of formatting.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext
  2014-07-12 18:40         ` H. Peter Anvin
@ 2014-07-12 18:52           ` Andi Kleen
  2014-07-12 21:17             ` Andy Lutomirski
  0 siblings, 1 reply; 16+ messages in thread
From: Andi Kleen @ 2014-07-12 18:52 UTC (permalink / raw)
  To: H. Peter Anvin
  Cc: Andi Kleen, Andy Lutomirski, Linus Torvalds, linux-kernel,
	Al Viro, the arch/x86 maintainers

On Sat, Jul 12, 2014 at 11:40:03AM -0700, H. Peter Anvin wrote:
> Because you are doing something weird (like Pin, for example) and take an asynchronous fault?

But even for pin that would need executing 16 bit code, or really weird
32bit code. AFAIK for 32bit the only good use case was NX emulation
(and old virtualization) which are both completely obsolete.

I don't think it's worth messing with the signal handlers for 16bit
code. If there's any problem with saving/restoring state that emulator 
can always handle it by itself.

-Andi

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext
  2014-07-12 18:52           ` Andi Kleen
@ 2014-07-12 21:17             ` Andy Lutomirski
  2014-07-18  1:13               ` Andy Lutomirski
  0 siblings, 1 reply; 16+ messages in thread
From: Andy Lutomirski @ 2014-07-12 21:17 UTC (permalink / raw)
  To: Andi Kleen
  Cc: H. Peter Anvin, Linus Torvalds, linux-kernel, Al Viro,
	the arch/x86 maintainers

On Sat, Jul 12, 2014 at 11:52 AM, Andi Kleen <andi@firstfloor.org> wrote:
> On Sat, Jul 12, 2014 at 11:40:03AM -0700, H. Peter Anvin wrote:
>> Because you are doing something weird (like Pin, for example) and take an asynchronous fault?
>
> But even for pin that would need executing 16 bit code, or really weird
> 32bit code. AFAIK for 32bit the only good use case was NX emulation
> (and old virtualization) which are both completely obsolete.

Nothing particularly weird is needed.  Set a non-default stack segment
(e.g. any 16-bit ss) and take *any* fault.  This could be something
asynchronous or even a normal synchronous fault.  Return from the
signal handler: boom.

We know that people use 16-bit stack segments: it's what prompted the
whole espfix64 thing.

>
> I don't think it's worth messing with the signal handlers for 16bit
> code. If there's any problem with saving/restoring state that emulator
> can always handle it by itself.
>

How?

I can think of at least two vaguely feasible ways.  The program could
ptrace itself, trap sigreturn, and fix ss.  Or it could restore
registers itself and return using far ret or iret.  Both suck.

--Andy

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext
  2014-07-12 21:17             ` Andy Lutomirski
@ 2014-07-18  1:13               ` Andy Lutomirski
  0 siblings, 0 replies; 16+ messages in thread
From: Andy Lutomirski @ 2014-07-18  1:13 UTC (permalink / raw)
  To: Andi Kleen
  Cc: H. Peter Anvin, Linus Torvalds, linux-kernel, Al Viro,
	the arch/x86 maintainers

On Sat, Jul 12, 2014 at 2:17 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Sat, Jul 12, 2014 at 11:52 AM, Andi Kleen <andi@firstfloor.org> wrote:
>> On Sat, Jul 12, 2014 at 11:40:03AM -0700, H. Peter Anvin wrote:
>>> Because you are doing something weird (like Pin, for example) and take an asynchronous fault?
>>
>> But even for pin that would need executing 16 bit code, or really weird
>> 32bit code. AFAIK for 32bit the only good use case was NX emulation
>> (and old virtualization) which are both completely obsolete.
>
> Nothing particularly weird is needed.  Set a non-default stack segment
> (e.g. any 16-bit ss) and take *any* fault.  This could be something
> asynchronous or even a normal synchronous fault.  Return from the
> signal handler: boom.
>
> We know that people use 16-bit stack segments: it's what prompted the
> whole espfix64 thing.
>
>>
>> I don't think it's worth messing with the signal handlers for 16bit
>> code. If there's any problem with saving/restoring state that emulator
>> can always handle it by itself.
>>
>
> How?
>
> I can think of at least two vaguely feasible ways.  The program could
> ptrace itself, trap sigreturn, and fix ss.  Or it could restore
> registers itself and return using far ret or iret.  Both suck.

You can't even hack around this with ptrace -- ptrace silently fails
to write ss for non-TIF_IA32 tasks.

--Andy

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/2] x86_64,signal: Save and restore SS in signal frames
  2014-07-11 16:29 ` [PATCH 1/2] x86_64,signal: Save and restore SS in signal frames Andy Lutomirski
@ 2015-03-09 20:15   ` Andy Lutomirski
  0 siblings, 0 replies; 16+ messages in thread
From: Andy Lutomirski @ 2015-03-09 20:15 UTC (permalink / raw)
  To: H. Peter Anvin, Andi Kleen, Ingo Molnar
  Cc: Al Viro, X86 ML, linux-kernel, Andy Lutomirski

Hi Ingo,

Here's a blast from the past.  I asked Andi today, and he seems
conceptually okay with this.  Could you consider applying it?  Aside
from being a bug fix IMO, it will allow my sigreturn test to work
reasonably well as a native 64-bit program.

I checked: it still applies cleanly.

--Andy

On Fri, Jul 11, 2014 at 9:29 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.
>
> 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               | 8 +-------
>  3 files changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/include/asm/sigcontext.h b/arch/x86/include/asm/sigcontext.h
> index 9dfce4e..f910cdc 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 d8b9f90..076b11f 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 2851d63..08d7a9e 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);
> --
> 1.9.3
>



-- 
Andy Lutomirski
AMA Capital Management, LLC

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2015-03-09 20:15 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-07-11 16:29 [PATCH 0/2] x86_64,signal: Fix the set of saved segment registers Andy Lutomirski
2014-07-11 16:29 ` [PATCH 1/2] x86_64,signal: Save and restore SS in signal frames Andy Lutomirski
2015-03-09 20:15   ` Andy Lutomirski
2014-07-11 16:29 ` [PATCH 2/2] x86_64,signal: Remove 'fs' and 'gs' from sigcontext Andy Lutomirski
2014-07-11 18:12   ` Andi Kleen
2014-07-11 18:39     ` Andy Lutomirski
2014-07-12  2:09       ` H. Peter Anvin
2014-07-12  2:04   ` H. Peter Anvin
2014-07-12  2:21   ` Linus Torvalds
2014-07-12  2:26     ` H. Peter Anvin
2014-07-12  8:39     ` Andy Lutomirski
2014-07-12 18:37       ` Andi Kleen
2014-07-12 18:40         ` H. Peter Anvin
2014-07-12 18:52           ` Andi Kleen
2014-07-12 21:17             ` Andy Lutomirski
2014-07-18  1:13               ` Andy Lutomirski

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).