[PATCH 0/2] Don't use SIGMINSTKSZ when enforcing alternative signal stack size for compat tasks

[PATCH 0/2] Don't use SIGMINSTKSZ when enforcing alternative signal stack size for compat tasks

[Will Deacon]

Hi all,

The Debian folks have observed a failure in the 32-bit arm glibc testsuite
when running under a 64-bit kernel. They tracked this down to sigaltstack(2)
enforcing the alternative signal stack to be at least SIGMINSTKSZ bytes,
which is higher for native arm64 tasks than compat 32-bit tasks.

These patches resolve the issue by allowing an architecture to define
COMPAT_SIGMINSTKSZ for compat tasks, which is then used by the sigaltstack
checking code.

Feedback welcome,

Will

--->8

Will Deacon (2):
  signal: Introduce COMPAT_SIGMINSTKSZ for use in compat_sys_sigaltstack
  arm64: compat: Provide definition for COMPAT_SIGMINSTKSZ

 arch/arm64/include/asm/compat.h |  1 +
 include/linux/compat.h          |  3 +++
 kernel/signal.c                 | 14 +++++++++-----
 3 files changed, 13 insertions(+), 5 deletions(-)

-- 
2.1.4

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

[Will Deacon]

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);
 	/* 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));
-- 
2.1.4

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

[Dave Martin]

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

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

[Will Deacon]

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

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

[Dave Martin]

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

[PATCH 2/2] arm64: compat: Provide definition for COMPAT_SIGMINSTKSZ

[Will Deacon]

arch/arm/ defines a SIGMINSTKSZ of 2k, so we should use the same value
for compat tasks.

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>
---
 arch/arm64/include/asm/compat.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/include/asm/compat.h b/arch/arm64/include/asm/compat.h
index 1a037b94eba1..cee28a05ee98 100644
--- a/arch/arm64/include/asm/compat.h
+++ b/arch/arm64/include/asm/compat.h
@@ -159,6 +159,7 @@ static inline compat_uptr_t ptr_to_compat(void __user *uptr)
 }
 
 #define compat_user_stack_pointer() (user_stack_pointer(task_pt_regs(current)))
+#define COMPAT_MINSIGSTKSZ	2048
 
 static inline void __user *arch_compat_alloc_user_space(long len)
 {
-- 
2.1.4

[PATCH 2/2] arm64: compat: Provide definition for COMPAT_SIGMINSTKSZ

[Dave Martin]

On Wed, Jul 25, 2018 at 02:45:12PM +0100, Will Deacon wrote:
> arch/arm/ defines a SIGMINSTKSZ of 2k, so we should use the same value
> for compat tasks.
> 
> 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>

Reviewed-by: Dave Martin <Dave.Martin@arm.com>

> ---
>  arch/arm64/include/asm/compat.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/include/asm/compat.h b/arch/arm64/include/asm/compat.h
> index 1a037b94eba1..cee28a05ee98 100644
> --- a/arch/arm64/include/asm/compat.h
> +++ b/arch/arm64/include/asm/compat.h
> @@ -159,6 +159,7 @@ static inline compat_uptr_t ptr_to_compat(void __user *uptr)
>  }
>  
>  #define compat_user_stack_pointer() (user_stack_pointer(task_pt_regs(current)))
> +#define COMPAT_MINSIGSTKSZ	2048
>  
>  static inline void __user *arch_compat_alloc_user_space(long len)
>  {
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] Don't use SIGMINSTKSZ when enforcing alternative signal stack size for compat tasks

[Aurelien Jarno]

On 2018-07-25 14:45, Will Deacon wrote:
> Hi all,
> 
> The Debian folks have observed a failure in the 32-bit arm glibc testsuite
> when running under a 64-bit kernel. They tracked this down to sigaltstack(2)
> enforcing the alternative signal stack to be at least SIGMINSTKSZ bytes,
> which is higher for native arm64 tasks than compat 32-bit tasks.
> 
> These patches resolve the issue by allowing an architecture to define
> COMPAT_SIGMINSTKSZ for compat tasks, which is then used by the sigaltstack
> checking code.
> 
> Feedback welcome,
> 
> Will
> 
> --->8
> 
> Will Deacon (2):
>   signal: Introduce COMPAT_SIGMINSTKSZ for use in compat_sys_sigaltstack
>   arm64: compat: Provide definition for COMPAT_SIGMINSTKSZ

Only the first patch went to the stable kernels. The second one is
missing, so the bug is still not fixed in those kernels. Would it be
possible to also get it included?

Thanks,
Aurelien

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] Don't use SIGMINSTKSZ when enforcing alternative signal stack size for compat tasks

[Will Deacon]

On Mon, Jul 29, 2019 at 10:23:02PM +0200, Aurelien Jarno wrote:
> On 2018-07-25 14:45, Will Deacon wrote:
> > Hi all,
> > 
> > The Debian folks have observed a failure in the 32-bit arm glibc testsuite
> > when running under a 64-bit kernel. They tracked this down to sigaltstack(2)
> > enforcing the alternative signal stack to be at least SIGMINSTKSZ bytes,
> > which is higher for native arm64 tasks than compat 32-bit tasks.
> > 
> > These patches resolve the issue by allowing an architecture to define
> > COMPAT_SIGMINSTKSZ for compat tasks, which is then used by the sigaltstack
> > checking code.
> > 
> > Feedback welcome,
> > 
> > Will
> > 
> > --->8
> > 
> > Will Deacon (2):
> >   signal: Introduce COMPAT_SIGMINSTKSZ for use in compat_sys_sigaltstack
> >   arm64: compat: Provide definition for COMPAT_SIGMINSTKSZ
> 
> Only the first patch went to the stable kernels. The second one is
> missing, so the bug is still not fixed in those kernels. Would it be
> possible to also get it included?

Damn, you're right. I think the autosel bot picked the first commit but not
the second. In hindsight, we should've tagged them both, but oh well. I've
posted the patch here for -stable, with you on cc:

https://lore.kernel.org/lkml/20190730092547.1284-1-will@kernel.org/T/#u

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel