[PATCH] x86/signals: Fix save/restore signal stack to correctly support sigset_t

[PATCH] x86/signals: Fix save/restore signal stack to correctly support sigset_t

[Walt Drummond]

The macro unsafe_put_sigmask() only handles the first 64 bits of the
sigmask_t, which works today.  However, if the definition of the
sigset_t structure ever changed, this would fail to setup/restore the
signal stack properly and likely corrupt the sigset. This patch
updates unsafe_put_sigmask() to correctly save all the fields in the
sigmask_t struct, and adds unsafe_put_compat_sigmask() to handle the
compat_sigset_t cases.

Signed-off-by: Walt Drummond <walt@drummond.us>
---
 arch/x86/kernel/signal.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index be0d7d4152ec..4d5134b4bb5f 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -203,11 +203,18 @@ do {									\
 		goto label;						\
 } while(0);
 
-#define unsafe_put_sigmask(set, frame, label) \
+#define unsafe_put_compat_sigmask(set, frame, label) \
 	unsafe_put_user(*(__u64 *)(set), \
 			(__u64 __user *)&(frame)->uc.uc_sigmask, \
 			label)
 
+#define unsafe_put_sigmask(set, frame, label)           \
+do {                                                    \
+	int i;								\
+	for (i = 0; i < _NSIG_WORDS; i++)				\
+		unsafe_put_user((set)->sig[i], &(frame)->uc.uc_sigmask.sig[i], label); \
+} while(0);
+
 /*
  * Set up a signal frame.
  */
@@ -566,7 +573,7 @@ static int x32_setup_rt_frame(struct ksignal *ksig,
 	restorer = ksig->ka.sa.sa_restorer;
 	unsafe_put_user(restorer, (unsigned long __user *)&frame->pretcode, Efault);
 	unsafe_put_sigcontext(&frame->uc.uc_mcontext, fp, regs, set, Efault);
-	unsafe_put_sigmask(set, frame, Efault);
+	unsafe_put_compat_sigmask(set, frame, Efault);
 	user_access_end();
 
 	if (ksig->ka.sa.sa_flags & SA_SIGINFO) {
@@ -643,7 +650,7 @@ SYSCALL_DEFINE0(rt_sigreturn)
 	frame = (struct rt_sigframe __user *)(regs->sp - sizeof(long));
 	if (!access_ok(frame, sizeof(*frame)))
 		goto badframe;
-	if (__get_user(*(__u64 *)&set, (__u64 __user *)&frame->uc.uc_sigmask))
+	if (copy_from_user(&set, &frame->uc.uc_sigmask, sizeof(sigset_t)))
 		goto badframe;
 	if (__get_user(uc_flags, &frame->uc.uc_flags))
 		goto badframe;
-- 
2.27.0

Re: [PATCH] x86/signals: Fix save/restore signal stack to correctly support sigset_t

[Al Viro]

On Thu, Nov 19, 2020 at 02:11:33PM -0800, Walt Drummond wrote:
> The macro unsafe_put_sigmask() only handles the first 64 bits of the
> sigmask_t, which works today.  However, if the definition of the
> sigset_t structure ever changed,

... existing userland would get fucked over, since sigset_t is
present in user-visible data structures.  Including the ones
we are using that thing for - struct rt_sigframe, for starters.

Layout of those suckers is very much cast in stone.  We *can't*
change it, no matter what we do kernel-side.

NAKed-by: Al Viro <viro@zeniv.linux.org.uk>

Re: [PATCH] x86/signals: Fix save/restore signal stack to correctly support sigset_t

[Walt Drummond]

(Sorry, resending as Gmail decided to ignore "Plaintext mode")

Thanks Al.  I want to understand the nuance, so please bear with me as
I reason this out.   The cast in stone nature of this is due to both
the need to keep userspace and kernel space in sync (ie, you'd have to
coordinate libc and kernel changes super tightly to pull this off),
and any change in the size of struct rt_sigframe would break backwards
compatibility with older binaries, is that correct?

Thanks, appreciate the help here.
--Walt


On Sat, Nov 28, 2020 at 6:19 PM Walt Drummond <walt@drummond.us> wrote:
>
> Thanks Al.  I want to understand the nuance, so please bear with me as I reason this out.   The cast in stone nature of this is due to both the need to keep userspace and kernel space in sync (ie, you'd have to coordinate libc and kernel changes super tightly to pull this off), and any change in the size of struct rt_sigframe would break backwards compatibility with older binaries, is that correct?
>
> Thanks, appreciate the help here.
> --Walt
>
>
> On Fri, Nov 27, 2020 at 9:23 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>>
>> On Thu, Nov 19, 2020 at 02:11:33PM -0800, Walt Drummond wrote:
>> > The macro unsafe_put_sigmask() only handles the first 64 bits of the
>> > sigmask_t, which works today.  However, if the definition of the
>> > sigset_t structure ever changed,
>>
>> ... existing userland would get fucked over, since sigset_t is
>> present in user-visible data structures.  Including the ones
>> we are using that thing for - struct rt_sigframe, for starters.
>>
>> Layout of those suckers is very much cast in stone.  We *can't*
>> change it, no matter what we do kernel-side.
>>
>> NAKed-by: Al Viro <viro@zeniv.linux.org.uk>

Re: [PATCH] x86/signals: Fix save/restore signal stack to correctly support sigset_t

[Al Viro]

On Sat, Nov 28, 2020 at 06:19:31PM -0800, Walt Drummond wrote:
> Thanks Al.  I want to understand the nuance, so please bear with me as I
> reason this out.   The cast in stone nature of this is due to both the need
> to keep userspace and kernel space in sync (ie, you'd have to coordinate
> libc and kernel changes super tightly to pull this off), and any change in
> the size of struct rt_sigframe would break backwards compatibility with
> older binaries, is that correct?

Pretty much so.  I would expect gdb and friends to be very unhappy about
that, for starters, along with a bunch of fun stuff like JVM, etc.

Ask the userland folks (libc, gdb, etc.) how would they feel about such
changes.  I'm fairly sure that it's _not_ going to be a matter of
changing _NSIG, rebuilding the kernel and living happily ever after.

Re: [PATCH] x86/signals: Fix save/restore signal stack to correctly support sigset_t

[Walt Drummond]

Got it.  Thanks again Al.

On Sat, Nov 28, 2020 at 7:28 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Sat, Nov 28, 2020 at 06:19:31PM -0800, Walt Drummond wrote:
> > Thanks Al.  I want to understand the nuance, so please bear with me as I
> > reason this out.   The cast in stone nature of this is due to both the need
> > to keep userspace and kernel space in sync (ie, you'd have to coordinate
> > libc and kernel changes super tightly to pull this off), and any change in
> > the size of struct rt_sigframe would break backwards compatibility with
> > older binaries, is that correct?
>
> Pretty much so.  I would expect gdb and friends to be very unhappy about
> that, for starters, along with a bunch of fun stuff like JVM, etc.
>
> Ask the userland folks (libc, gdb, etc.) how would they feel about such
> changes.  I'm fairly sure that it's _not_ going to be a matter of
> changing _NSIG, rebuilding the kernel and living happily ever after.