[PATCH 1/2] x86: signal_32: introduce retcode and rt_retcode

[PATCH 1/2] x86: signal_32: introduce retcode and rt_retcode

[Hiroshi Shimamoto]

From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>

Impact: cleanup

Introduce retcode and rt_retcode to replace setting up frame->retcode.

Signed-off-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
---
 arch/x86/kernel/signal_32.c |   30 ++++++++++++++++++++++++------
 1 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/signal_32.c b/arch/x86/kernel/signal_32.c
index 27a5c81..514171a 100644
--- a/arch/x86/kernel/signal_32.c
+++ b/arch/x86/kernel/signal_32.c
@@ -45,6 +45,28 @@
 # define FIX_EFLAGS	__FIX_EFLAGS
 #endif
 
+static const struct {
+	u16 poplmovl;
+	u32 val;
+	u16 int80;
+} __attribute__((packed)) retcode = {
+	0xb858,		/* popl %eax; movl $..., %eax */
+	__NR_sigreturn,
+	0x80cd,		/* int $0x80 */
+};
+
+static const struct {
+	u8  movl;
+	u32 val;
+	u16 int80;
+	u8  pad;
+} __attribute__((packed)) rt_retcode = {
+	0xb8,		/* movl $..., %eax */
+	__NR_rt_sigreturn,
+	0x80cd,		/* int $0x80 */
+	0
+};
+
 /*
  * Atomically swap in the new signal mask, and wait for a signal.
  */
@@ -427,9 +449,7 @@ __setup_frame(int sig, struct k_sigaction *ka, sigset_t *set,
 	 * reasons and because gdb uses it as a signature to notice
 	 * signal handler stack frames.
 	 */
-	err |= __put_user(0xb858, (short __user *)(frame->retcode+0));
-	err |= __put_user(__NR_sigreturn, (int __user *)(frame->retcode+2));
-	err |= __put_user(0x80cd, (short __user *)(frame->retcode+6));
+	err |= __put_user(*((u64 *)&retcode), (u64 *)frame->retcode);
 
 	if (err)
 		return -EFAULT;
@@ -498,9 +518,7 @@ static int __setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
 	 * reasons and because gdb uses it as a signature to notice
 	 * signal handler stack frames.
 	 */
-	err |= __put_user(0xb8, (char __user *)(frame->retcode+0));
-	err |= __put_user(__NR_rt_sigreturn, (int __user *)(frame->retcode+1));
-	err |= __put_user(0x80cd, (short __user *)(frame->retcode+5));
+	err |= __put_user(*((u64 *)&rt_retcode), (u64 *)frame->retcode);
 
 	if (err)
 		return -EFAULT;
-- 
1.5.6

[PATCH 2/2] x86: ia32_signal: remove unnecessary padding

[Hiroshi Shimamoto]

From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>

Impact: cleanup

Remove unnecessary paddings, this saves 4 bytes.

Signed-off-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
---
 arch/x86/ia32/ia32_signal.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
index bafc0b4..e886907 100644
--- a/arch/x86/ia32/ia32_signal.c
+++ b/arch/x86/ia32/ia32_signal.c
@@ -427,12 +427,10 @@ int ia32_setup_frame(int sig, struct k_sigaction *ka,
 		u16 poplmovl;
 		u32 val;
 		u16 int80;
-		u16 pad;
 	} __attribute__((packed)) code = {
 		0xb858,		 /* popl %eax ; movl $...,%eax */
 		__NR_ia32_sigreturn,
 		0x80cd,		/* int $0x80 */
-		0,
 	};
 
 	frame = get_sigframe(ka, regs, sizeof(*frame), &fpstate);
@@ -508,8 +506,7 @@ int ia32_setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
 		u8 movl;
 		u32 val;
 		u16 int80;
-		u16 pad;
-		u8  pad2;
+		u8  pad;
 	} __attribute__((packed)) code = {
 		0xb8,
 		__NR_ia32_rt_sigreturn,
-- 
1.5.6

Re: [PATCH 2/2] x86: ia32_signal: remove unnecessary padding

[Ingo Molnar]

* Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com> wrote:

> From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
> 
> Impact: cleanup
> 
> Remove unnecessary paddings, this saves 4 bytes.
> 
> Signed-off-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
> ---
>  arch/x86/ia32/ia32_signal.c |    5 +----
>  1 files changed, 1 insertions(+), 4 deletions(-)

applied your patches to tip/x86/signal:

 9cc3c49: x86: ia32_signal: remove unnecessary padding
 4a61204: x86: signal_32: introduce retcode and rt_retcode

thanks!

A question - this change:

> @@ -427,12 +427,10 @@ int ia32_setup_frame(int sig, struct k_sigaction *ka,
>  		u16 poplmovl;
>  		u32 val;
>  		u16 int80;
> -		u16 pad;
>  	} __attribute__((packed)) code = {
>  		0xb858,		 /* popl %eax ; movl $...,%eax */
>  		__NR_ia32_sigreturn,
>  		0x80cd,		/* int $0x80 */
> -		0,
>  	};
>  
>  	frame = get_sigframe(ka, regs, sizeof(*frame), &fpstate);
> @@ -508,8 +506,7 @@ int ia32_setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
>  		u8 movl;
>  		u32 val;
>  		u16 int80;
> -		u16 pad;
> -		u8  pad2;
> +		u8  pad;
>  	} __attribute__((packed)) code = {
>  		0xb8,
>  		__NR_ia32_rt_sigreturn,

does not impact any ABI, because it's only about the signal trampoline 
the kernel pushes to the user-space stack - not about any 
userspace-visible signal frame detail, right?

	Ingo

Re: [PATCH 2/2] x86: ia32_signal: remove unnecessary padding

[Mikael Pettersson]

Ingo Molnar writes:
 > 
 > * Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com> wrote:
 > 
 > > From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
 > > 
 > > Impact: cleanup
 > > 
 > > Remove unnecessary paddings, this saves 4 bytes.
 > > 
 > > Signed-off-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
 > > ---
 > >  arch/x86/ia32/ia32_signal.c |    5 +----
 > >  1 files changed, 1 insertions(+), 4 deletions(-)
 > 
 > applied your patches to tip/x86/signal:
 > 
 >  9cc3c49: x86: ia32_signal: remove unnecessary padding
 >  4a61204: x86: signal_32: introduce retcode and rt_retcode
 > 
 > thanks!
 > 
 > A question - this change:
 > 
 > > @@ -427,12 +427,10 @@ int ia32_setup_frame(int sig, struct k_sigaction *ka,
 > >  		u16 poplmovl;
 > >  		u32 val;
 > >  		u16 int80;
 > > -		u16 pad;
 > >  	} __attribute__((packed)) code = {
 > >  		0xb858,		 /* popl %eax ; movl $...,%eax */
 > >  		__NR_ia32_sigreturn,
 > >  		0x80cd,		/* int $0x80 */
 > > -		0,
 > >  	};
 > >  
 > >  	frame = get_sigframe(ka, regs, sizeof(*frame), &fpstate);
 > > @@ -508,8 +506,7 @@ int ia32_setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
 > >  		u8 movl;
 > >  		u32 val;
 > >  		u16 int80;
 > > -		u16 pad;
 > > -		u8  pad2;
 > > +		u8  pad;
 > >  	} __attribute__((packed)) code = {
 > >  		0xb8,
 > >  		__NR_ia32_rt_sigreturn,
 > 
 > does not impact any ABI, because it's only about the signal trampoline 
 > the kernel pushes to the user-space stack - not about any 
 > userspace-visible signal frame detail, right?

As long as the 'char retcode[8]' field isn't reduced in size
(you can _never_ do that!), and the initial instruction
sequence up to the 'int $0x80' isn't changed (you can't change
that either), I believe this is safe.

It does cause each signal delivery to leak 2 uninitialised
kernel bytes to the end of retcode[], which seems unnecessary.

Re: [PATCH 2/2] x86: ia32_signal: remove unnecessary padding

[Andi Kleen]

Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com> writes:

> From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
>
> Impact: cleanup

Actually it's not a cleanup.

> Remove unnecessary paddings, this saves 4 bytes.

This might actually break code. The code is not actually used,
but only kept around because some old gdb versions used this
code as a marker for detecting signals.

I don't know if they require the padding or not, but it seems
somewhat risky to change a legacy marker like this.

-Andi

-- 
ak@linux.intel.com

Re: [PATCH 2/2] x86: ia32_signal: remove unnecessary padding

[H. Peter Anvin]

Mikael Pettersson wrote:
> It does cause each signal delivery to leak 2 uninitialised
> kernel bytes to the end of retcode[], which seems unnecessary.

Not just unnecessary, it is a huge no-no for security.

NAK on this.

	-hpa

Re: [PATCH 2/2] x86: ia32_signal: remove unnecessary padding

[Hiroshi Shimamoto]

H. Peter Anvin wrote:
> Mikael Pettersson wrote:
>> It does cause each signal delivery to leak 2 uninitialised
>> kernel bytes to the end of retcode[], which seems unnecessary.
> 
> Not just unnecessary, it is a huge no-no for security.

Am I missing important thing?
The frame->retcode is 8 bytes and packed structure with padding
is 10 bytes each, and the code is copied to user stack 8 bytes only.

err |= __copy_to_user(frame->retcode, &code, 8);

I don't think the behavior is changed.

thanks,
Hiroshi Shimamoto

> 
> NAK on this.
> 
> 	-hpa
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH 2/2] x86: ia32_signal: remove unnecessary padding

[H. Peter Anvin]

Hiroshi Shimamoto wrote:
> H. Peter Anvin wrote:
>> Mikael Pettersson wrote:
>>> It does cause each signal delivery to leak 2 uninitialised
>>> kernel bytes to the end of retcode[], which seems unnecessary.
>> Not just unnecessary, it is a huge no-no for security.
> 
> Am I missing important thing?
> The frame->retcode is 8 bytes and packed structure with padding
> is 10 bytes each, and the code is copied to user stack 8 bytes only.
> 
> err |= __copy_to_user(frame->retcode, &code, 8);
> 
> I don't think the behavior is changed.
> 

Ah, nevermind, then.  Then it fine, obviously.

	-hpa

Re: [PATCH 2/2] x86: ia32_signal: remove unnecessary padding

[Hiroshi Shimamoto]

Andi Kleen wrote:
> Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com> writes:
> 
>> From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
>>
>> Impact: cleanup
> 
> Actually it's not a cleanup.
> 
>> Remove unnecessary paddings, this saves 4 bytes.
> 
> This might actually break code. The code is not actually used,
> but only kept around because some old gdb versions used this
> code as a marker for detecting signals.

I think, this cleanup doesn't change the code copied to user stack.
It removes additional 2 bytes from each struct, and these had not
been copied to user stack, because the structures were 10 bytes and
frame->retcode is 8 bytes.

Thanks,
Hiroshi Shimamoto

Re: [PATCH 2/2] x86: ia32_signal: remove unnecessary padding

[Mikael Pettersson]

H. Peter Anvin writes:
 > Hiroshi Shimamoto wrote:
 > > H. Peter Anvin wrote:
 > >> Mikael Pettersson wrote:
 > >>> It does cause each signal delivery to leak 2 uninitialised
 > >>> kernel bytes to the end of retcode[], which seems unnecessary.
 > >> Not just unnecessary, it is a huge no-no for security.
 > > 
 > > Am I missing important thing?
 > > The frame->retcode is 8 bytes and packed structure with padding
 > > is 10 bytes each, and the code is copied to user stack 8 bytes only.
 > > 
 > > err |= __copy_to_user(frame->retcode, &code, 8);
 > > 
 > > I don't think the behavior is changed.
 > > 
 > 
 > Ah, nevermind, then.  Then it fine, obviously.

Agreed. I wonder how on earth those templates ended
up as 10 bytes large when retcode[] always has been
8 bytes.

Re: [PATCH 2/2] x86: ia32_signal: remove unnecessary padding

[H. Peter Anvin]

Mikael Pettersson wrote:
>  > 
>  > Ah, nevermind, then.  Then it fine, obviously.
> 
> Agreed. I wonder how on earth those templates ended
> up as 10 bytes large when retcode[] always has been
> 8 bytes.

Probably someone added something and didn't adjust the padding.

	-hpa