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

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

[Will Deacon]

Hi all,

This is a resend of:

  http://lists.infradead.org/pipermail/linux-arm-kernel/2018-July/593559.html

now based on 4.19-rc2.

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

[RESEND 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 1a3c4f37e908..de0c13bdcd2c 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 5843c541fda9..e4aad0e90882 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3460,7 +3460,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;
 
@@ -3490,7 +3491,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;
 		}
 
@@ -3508,7 +3509,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;
@@ -3519,7 +3521,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;
 }
@@ -3553,7 +3556,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

[RESEND 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>
Reviewed-by: Dave Martin <Dave.Martin@arm.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

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

[Will Deacon]

Hi all,

[+ more people from get_maintainers.pl]

On Wed, Sep 05, 2018 at 03:34:41PM +0100, Will Deacon wrote:
> This is a resend of:
> 
>   http://lists.infradead.org/pipermail/linux-arm-kernel/2018-July/593559.html
> 
> now based on 4.19-rc2.
> 
> 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,

Any comments on this, please? I'd really like to get this fixed, since
Debian have an outstanding issue without these patches merged. I'm happy
to take them via arm64, but would really like an Ack on the changes to
signal.c first...

Thanks,

Will

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

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

[Steve McIntyre]

On Wed, Sep 05, 2018 at 03:34:41PM +0100, Will Deacon wrote:
>Hi all,
>
>This is a resend of:
>
>  http://lists.infradead.org/pipermail/linux-arm-kernel/2018-July/593559.html
>
>now based on 4.19-rc2.
>
>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,

Apologies for the delayed response here - conference travel etc . got
in the way... I've just tested and I can confirm that this patchset
fixes our reported bug (as in https://bugs.debian.org/904385). Thanks
Will!

Tested-by: Steve McIntyre <93sam@debian.org>

--
Steve McIntyre
steve.mcintyre@arm.com

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

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

[Steve McIntyre]

[ Re-sending without the corporate footer garbage... ]

On Wed, Sep 05, 2018 at 03:34:41PM +0100, Will Deacon wrote:
>Hi all,
>
>This is a resend of:
>
>  http://lists.infradead.org/pipermail/linux-arm-kernel/2018-July/593559.html
>
>now based on 4.19-rc2.
>
>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,

Apologies for the delayed response here - conference travel etc . got
in the way... I've just tested and I can confirm that this patchset
fixes our reported bug (as in https://bugs.debian.org/904385). Thanks
Will!

Tested-by: Steve McIntyre <93sam@debian.org>

-- 
Steve McIntyre  <93sam@debian.org>

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

[Catalin Marinas]

On Fri, Sep 28, 2018 at 02:18:11PM +0100, Steve McIntyre wrote:
> On Wed, Sep 05, 2018 at 03:34:41PM +0100, Will Deacon wrote:
> >Hi all,
> >
> >This is a resend of:
> >
> >  http://lists.infradead.org/pipermail/linux-arm-kernel/2018-July/593559.html
> >
> >now based on 4.19-rc2.
> >
> >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,
> 
> Apologies for the delayed response here - conference travel etc . got
> in the way... I've just tested and I can confirm that this patchset
> fixes our reported bug (as in https://bugs.debian.org/904385). Thanks
> Will!
> 
> Tested-by: Steve McIntyre <93sam@debian.org>

Thanks for confirming Steve.

As there is no functional change for other architectures, I'll queue
both patches through the arm64 tree. If anyone objects, please let me
know before the merging window.

-- 
Catalin