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

[Dave Martin <Dave.Martin@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