From: Hiroshi Shimamoto <[email protected]>
Impact: cleanup
Introduce retcode and rt_retcode to replace setting up frame->retcode.
Signed-off-by: Hiroshi Shimamoto <[email protected]>
---
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
From: Hiroshi Shimamoto <[email protected]>
Impact: cleanup
Remove unnecessary paddings, this saves 4 bytes.
Signed-off-by: Hiroshi Shimamoto <[email protected]>
---
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
* Hiroshi Shimamoto <[email protected]> wrote:
> From: Hiroshi Shimamoto <[email protected]>
>
> Impact: cleanup
>
> Remove unnecessary paddings, this saves 4 bytes.
>
> Signed-off-by: Hiroshi Shimamoto <[email protected]>
> ---
> 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
Ingo Molnar writes:
>
> * Hiroshi Shimamoto <[email protected]> wrote:
>
> > From: Hiroshi Shimamoto <[email protected]>
> >
> > Impact: cleanup
> >
> > Remove unnecessary paddings, this saves 4 bytes.
> >
> > Signed-off-by: Hiroshi Shimamoto <[email protected]>
> > ---
> > 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.
Hiroshi Shimamoto <[email protected]> writes:
> From: Hiroshi Shimamoto <[email protected]>
>
> 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
--
[email protected]
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
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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
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
Andi Kleen wrote:
> Hiroshi Shimamoto <[email protected]> writes:
>
>> From: Hiroshi Shimamoto <[email protected]>
>>
>> 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
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.
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