| Message ID | 1532526312-26993-2-git-send-email-will.deacon@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Don't use SIGMINSTKSZ when enforcing alternative signal stack size for compat tasks | expand |
On Wed, Jul 25, 2018 at 02:45:11PM +0100, Will Deacon wrote: > The sigaltstack(2) system call fails with -ENOMEM if the new alternative > signal stack is found to be smaller than SIGMINSTKSZ. On architectures > such as arm64, where the native value for SIGMINSTKSZ is larger than > the compat value, this can result in an unexpected error being reported > to a compat task. See, for example: > > https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=904385 > > This patch fixes the problem by extending do_sigaltstack to take the > minimum signal stack size as an additional parameter, allowing the > native and compat system call entry code to pass in their respective > values. COMPAT_SIGMINSTKSZ is just defined as SIGMINSTKSZ if it has not > been defined by the architecture. > > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Dominik Brodowski <linux@dominikbrodowski.net> > Cc: "Eric W. Biederman" <ebiederm@xmission.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Al Viro <viro@zeniv.linux.org.uk> > Cc: Oleg Nesterov <oleg@redhat.com> > Reported-by: Steve McIntyre <steve.mcintyre@arm.com> > Signed-off-by: Will Deacon <will.deacon@arm.com> > --- > include/linux/compat.h | 3 +++ > kernel/signal.c | 14 +++++++++----- > 2 files changed, 12 insertions(+), 5 deletions(-) > > diff --git a/include/linux/compat.h b/include/linux/compat.h > index c68acc47da57..47041c7fed28 100644 > --- a/include/linux/compat.h > +++ b/include/linux/compat.h > @@ -103,6 +103,9 @@ typedef struct compat_sigaltstack { > compat_size_t ss_size; > } compat_stack_t; > #endif > +#ifndef COMPAT_MINSIGSTKSZ > +#define COMPAT_MINSIGSTKSZ MINSIGSTKSZ > +#endif > > #define compat_jiffies_to_clock_t(x) \ > (((unsigned long)(x) * COMPAT_USER_HZ) / HZ) > diff --git a/kernel/signal.c b/kernel/signal.c > index 8d8a940422a8..41a5dd2df27d 100644 > --- a/kernel/signal.c > +++ b/kernel/signal.c > @@ -3417,7 +3417,8 @@ int do_sigaction(int sig, struct k_sigaction *act, struct k_sigaction *oact) > } > > static int > -do_sigaltstack (const stack_t *ss, stack_t *oss, unsigned long sp) > +do_sigaltstack (const stack_t *ss, stack_t *oss, unsigned long sp, > + size_t min_ss_size) > { > struct task_struct *t = current; > > @@ -3447,7 +3448,7 @@ do_sigaltstack (const stack_t *ss, stack_t *oss, unsigned long sp) > ss_size = 0; > ss_sp = NULL; > } else { > - if (unlikely(ss_size < MINSIGSTKSZ)) > + if (unlikely(ss_size < min_ss_size)) > return -ENOMEM; > } > > @@ -3465,7 +3466,8 @@ SYSCALL_DEFINE2(sigaltstack,const stack_t __user *,uss, stack_t __user *,uoss) > if (uss && copy_from_user(&new, uss, sizeof(stack_t))) > return -EFAULT; > err = do_sigaltstack(uss ? &new : NULL, uoss ? &old : NULL, > - current_user_stack_pointer()); > + current_user_stack_pointer(), > + MINSIGSTKSZ); > if (!err && uoss && copy_to_user(uoss, &old, sizeof(stack_t))) > err = -EFAULT; > return err; > @@ -3476,7 +3478,8 @@ int restore_altstack(const stack_t __user *uss) > stack_t new; > if (copy_from_user(&new, uss, sizeof(stack_t))) > return -EFAULT; > - (void)do_sigaltstack(&new, NULL, current_user_stack_pointer()); > + (void)do_sigaltstack(&new, NULL, current_user_stack_pointer(), > + MINSIGSTKSZ); Why can't this fail? If this fails here we silently go wrong, but... > /* squash all but EFAULT for now */ > return 0; > } > @@ -3510,7 +3513,8 @@ static int do_compat_sigaltstack(const compat_stack_t __user *uss_ptr, > uss.ss_size = uss32.ss_size; > } > ret = do_sigaltstack(uss_ptr ? &uss : NULL, &uoss, > - compat_user_stack_pointer()); > + compat_user_stack_pointer(), > + COMPAT_MINSIGSTKSZ); If this fails on arm64, we seem to SEGV (see compat_sys_rt_sigreturn()). This patch doesn't introduce this inconsistency, this might be a good opportunity to clean it up. Cheers ---Dave
On Wed, Jul 25, 2018 at 04:54:27PM +0100, Dave Martin wrote: > On Wed, Jul 25, 2018 at 02:45:11PM +0100, Will Deacon wrote: > > @@ -3476,7 +3478,8 @@ int restore_altstack(const stack_t __user *uss) > > stack_t new; > > if (copy_from_user(&new, uss, sizeof(stack_t))) > > return -EFAULT; > > - (void)do_sigaltstack(&new, NULL, current_user_stack_pointer()); > > + (void)do_sigaltstack(&new, NULL, current_user_stack_pointer(), > > + MINSIGSTKSZ); > > Why can't this fail? > > If this fails here we silently go wrong, but... > > > /* squash all but EFAULT for now */ > > return 0; > > } > > @@ -3510,7 +3513,8 @@ static int do_compat_sigaltstack(const compat_stack_t __user *uss_ptr, > > uss.ss_size = uss32.ss_size; > > } > > ret = do_sigaltstack(uss_ptr ? &uss : NULL, &uoss, > > - compat_user_stack_pointer()); > > + compat_user_stack_pointer(), > > + COMPAT_MINSIGSTKSZ); > > If this fails on arm64, we seem to SEGV (see compat_sys_rt_sigreturn()). > > This patch doesn't introduce this inconsistency, this might be a good > opportunity to clean it up. I don't think there's an inconsistency here -- both restore_altstack and compat_restore_altstack suppress all non--EFAULT errors (remember that uoss is NULL in both cases, so the copy_from_user() immediately before the do_sigaltstack() call for the native path is all we care about). I think the behaviour is: on a sigreturn, if you set the altstack to be an unmapped address then you get a SEGV, otherwise if you make it invalid in some other way (e.g. too small) then it's ignored and the old altstack remains intact. Will
On Wed, Jul 25, 2018 at 05:37:26PM +0100, Will Deacon wrote: > On Wed, Jul 25, 2018 at 04:54:27PM +0100, Dave Martin wrote: > > On Wed, Jul 25, 2018 at 02:45:11PM +0100, Will Deacon wrote: > > > @@ -3476,7 +3478,8 @@ int restore_altstack(const stack_t __user *uss) > > > stack_t new; > > > if (copy_from_user(&new, uss, sizeof(stack_t))) > > > return -EFAULT; > > > - (void)do_sigaltstack(&new, NULL, current_user_stack_pointer()); > > > + (void)do_sigaltstack(&new, NULL, current_user_stack_pointer(), > > > + MINSIGSTKSZ); > > > > Why can't this fail? > > > > If this fails here we silently go wrong, but... > > > > > /* squash all but EFAULT for now */ > > > return 0; > > > } > > > @@ -3510,7 +3513,8 @@ static int do_compat_sigaltstack(const compat_stack_t __user *uss_ptr, > > > uss.ss_size = uss32.ss_size; > > > } > > > ret = do_sigaltstack(uss_ptr ? &uss : NULL, &uoss, > > > - compat_user_stack_pointer()); > > > + compat_user_stack_pointer(), > > > + COMPAT_MINSIGSTKSZ); > > > > If this fails on arm64, we seem to SEGV (see compat_sys_rt_sigreturn()). > > > > This patch doesn't introduce this inconsistency, this might be a good > > opportunity to clean it up. > > I don't think there's an inconsistency here -- both restore_altstack and > compat_restore_altstack suppress all non--EFAULT errors (remember that uoss > is NULL in both cases, so the copy_from_user() immediately before the > do_sigaltstack() call for the native path is all we care about). I think the > behaviour is: on a sigreturn, if you set the altstack to be an unmapped > address then you get a SEGV, otherwise if you make it invalid in some other > way (e.g. too small) then it's ignored and the old altstack remains intact. OK, I think I've satisfied myself that they do the same thing for now. The code for the paths is a bit different, so it's not trivial to see that they have the same effect... Cheers ---Dave
diff --git a/include/linux/compat.h b/include/linux/compat.h index c68acc47da57..47041c7fed28 100644 --- a/include/linux/compat.h +++ b/include/linux/compat.h @@ -103,6 +103,9 @@ typedef struct compat_sigaltstack { compat_size_t ss_size; } compat_stack_t; #endif +#ifndef COMPAT_MINSIGSTKSZ +#define COMPAT_MINSIGSTKSZ MINSIGSTKSZ +#endif #define compat_jiffies_to_clock_t(x) \ (((unsigned long)(x) * COMPAT_USER_HZ) / HZ) diff --git a/kernel/signal.c b/kernel/signal.c index 8d8a940422a8..41a5dd2df27d 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -3417,7 +3417,8 @@ int do_sigaction(int sig, struct k_sigaction *act, struct k_sigaction *oact) } static int -do_sigaltstack (const stack_t *ss, stack_t *oss, unsigned long sp) +do_sigaltstack (const stack_t *ss, stack_t *oss, unsigned long sp, + size_t min_ss_size) { struct task_struct *t = current; @@ -3447,7 +3448,7 @@ do_sigaltstack (const stack_t *ss, stack_t *oss, unsigned long sp) ss_size = 0; ss_sp = NULL; } else { - if (unlikely(ss_size < MINSIGSTKSZ)) + if (unlikely(ss_size < min_ss_size)) return -ENOMEM; } @@ -3465,7 +3466,8 @@ SYSCALL_DEFINE2(sigaltstack,const stack_t __user *,uss, stack_t __user *,uoss) if (uss && copy_from_user(&new, uss, sizeof(stack_t))) return -EFAULT; err = do_sigaltstack(uss ? &new : NULL, uoss ? &old : NULL, - current_user_stack_pointer()); + current_user_stack_pointer(), + MINSIGSTKSZ); if (!err && uoss && copy_to_user(uoss, &old, sizeof(stack_t))) err = -EFAULT; return err; @@ -3476,7 +3478,8 @@ int restore_altstack(const stack_t __user *uss) stack_t new; if (copy_from_user(&new, uss, sizeof(stack_t))) return -EFAULT; - (void)do_sigaltstack(&new, NULL, current_user_stack_pointer()); + (void)do_sigaltstack(&new, NULL, current_user_stack_pointer(), + MINSIGSTKSZ); /* squash all but EFAULT for now */ return 0; } @@ -3510,7 +3513,8 @@ static int do_compat_sigaltstack(const compat_stack_t __user *uss_ptr, uss.ss_size = uss32.ss_size; } ret = do_sigaltstack(uss_ptr ? &uss : NULL, &uoss, - compat_user_stack_pointer()); + compat_user_stack_pointer(), + COMPAT_MINSIGSTKSZ); if (ret >= 0 && uoss_ptr) { compat_stack_t old; memset(&old, 0, sizeof(old));
The sigaltstack(2) system call fails with -ENOMEM if the new alternative signal stack is found to be smaller than SIGMINSTKSZ. On architectures such as arm64, where the native value for SIGMINSTKSZ is larger than the compat value, this can result in an unexpected error being reported to a compat task. See, for example: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=904385 This patch fixes the problem by extending do_sigaltstack to take the minimum signal stack size as an additional parameter, allowing the native and compat system call entry code to pass in their respective values. COMPAT_SIGMINSTKSZ is just defined as SIGMINSTKSZ if it has not been defined by the architecture. Cc: Arnd Bergmann <arnd@arndb.de> Cc: Dominik Brodowski <linux@dominikbrodowski.net> Cc: "Eric W. Biederman" <ebiederm@xmission.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: Oleg Nesterov <oleg@redhat.com> Reported-by: Steve McIntyre <steve.mcintyre@arm.com> Signed-off-by: Will Deacon <will.deacon@arm.com> --- include/linux/compat.h | 3 +++ kernel/signal.c | 14 +++++++++----- 2 files changed, 12 insertions(+), 5 deletions(-)