From: Dave Martin <Dave.Martin@arm.com> To: Will Deacon <will.deacon@arm.com> Cc: linux-kernel@vger.kernel.org, arnd@arndb.de, oleg@redhat.com, linux@dominikbrodowski.net, viro@zeniv.linux.org.uk, akpm@linux-foundation.org, steve.mcintyre@arm.com, linux-arm-kernel@lists.infradead.org, ebiederm@xmission.com Subject: Re: [PATCH 1/2] signal: Introduce COMPAT_SIGMINSTKSZ for use in compat_sys_sigaltstack Date: Wed, 25 Jul 2018 16:54:27 +0100 [thread overview] Message-ID: <20180725155427.GR4240@e103592.cambridge.arm.com> (raw) In-Reply-To: <1532526312-26993-2-git-send-email-will.deacon@arm.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Dave.Martin@arm.com (Dave Martin) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 1/2] signal: Introduce COMPAT_SIGMINSTKSZ for use in compat_sys_sigaltstack Date: Wed, 25 Jul 2018 16:54:27 +0100 [thread overview] Message-ID: <20180725155427.GR4240@e103592.cambridge.arm.com> (raw) In-Reply-To: <1532526312-26993-2-git-send-email-will.deacon@arm.com> 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
next prev parent reply other threads:[~2018-07-25 15:54 UTC|newest] Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-07-25 13:45 [PATCH 0/2] Don't use SIGMINSTKSZ when enforcing alternative signal stack size for compat tasks Will Deacon 2018-07-25 13:45 ` Will Deacon 2018-07-25 13:45 ` [PATCH 1/2] signal: Introduce COMPAT_SIGMINSTKSZ for use in compat_sys_sigaltstack Will Deacon 2018-07-25 13:45 ` Will Deacon 2018-07-25 15:54 ` Dave Martin [this message] 2018-07-25 15:54 ` Dave Martin 2018-07-25 16:37 ` Will Deacon 2018-07-25 16:37 ` Will Deacon 2018-07-26 10:44 ` Dave Martin 2018-07-26 10:44 ` Dave Martin 2018-07-25 13:45 ` [PATCH 2/2] arm64: compat: Provide definition for COMPAT_SIGMINSTKSZ Will Deacon 2018-07-25 13:45 ` Will Deacon 2018-07-25 15:55 ` Dave Martin 2018-07-25 15:55 ` Dave Martin 2019-07-29 20:23 ` [PATCH 0/2] Don't use SIGMINSTKSZ when enforcing alternative signal stack size for compat tasks Aurelien Jarno 2019-07-29 20:23 ` Aurelien Jarno 2019-07-30 9:27 ` Will Deacon 2019-07-30 9:27 ` Will Deacon
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20180725155427.GR4240@e103592.cambridge.arm.com \ --to=dave.martin@arm.com \ --cc=akpm@linux-foundation.org \ --cc=arnd@arndb.de \ --cc=ebiederm@xmission.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@dominikbrodowski.net \ --cc=oleg@redhat.com \ --cc=steve.mcintyre@arm.com \ --cc=viro@zeniv.linux.org.uk \ --cc=will.deacon@arm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.