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
arch/arm/ defines a SIGMINSTKSZ of 2k, so we should use the same value
for compat tasks.
Cc: Arnd Bergmann <[email protected]>
Cc: Dominik Brodowski <[email protected]>
Cc: "Eric W. Biederman" <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Reported-by: Steve McIntyre <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
---
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
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 <[email protected]>
Cc: Dominik Brodowski <[email protected]>
Cc: "Eric W. Biederman" <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Reported-by: Steve McIntyre <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
---
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
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 <[email protected]>
> Cc: Dominik Brodowski <[email protected]>
> Cc: "Eric W. Biederman" <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: Oleg Nesterov <[email protected]>
> Reported-by: Steve McIntyre <[email protected]>
> Signed-off-by: Will Deacon <[email protected]>
Reviewed-by: Dave Martin <[email protected]>
> ---
> 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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 <[email protected]>
> Cc: Dominik Brodowski <[email protected]>
> Cc: "Eric W. Biederman" <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: Oleg Nesterov <[email protected]>
> Reported-by: Steve McIntyre <[email protected]>
> Signed-off-by: Will Deacon <[email protected]>
> ---
> 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
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
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
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
[email protected] http://www.aurel32.net
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/[email protected]/T/#u
Will