diff mbox series

[1/2] signal: Introduce COMPAT_SIGMINSTKSZ for use in compat_sys_sigaltstack

Message ID 1532526312-26993-2-git-send-email-will.deacon@arm.com
State Superseded
Headers show
Series Don't use SIGMINSTKSZ when enforcing alternative signal stack size for compat tasks | expand

Commit Message

Will Deacon July 25, 2018, 1:45 p.m. UTC 
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(-)

-- 
2.1.4

Comments

Dave Martin July 25, 2018, 3:54 p.m. UTC | #1 
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
Will Deacon July 25, 2018, 4:37 p.m. UTC | #2 
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
Dave Martin July 26, 2018, 10:44 a.m. UTC | #3 
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 mbox series

Patch

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