sending SIGFPE via kill() returns wrong values in si_pid and si_uid

sending SIGFPE via kill() returns wrong values in si_pid and si_uid

[Rolf Eike Beer]

[-- Attachment #1: Type: text/plain, Size: 3100 bytes --]

This originally came up as a failure in the testsuite of strace on Gentoo 
(T5120, 64 bit kernel, 32 bit userspace) and was reported here: 
https://bugs.gentoo.org/643060. This was then reported upstream here: 
https://github.com/strace/strace/issues/21 . I'll paste the relevant parts of
those bugs here.

The test program is as follows (done by Dmitry V. Levin): 

#include <assert.h>
#include <signal.h>
#include <stdio.h>
#include <unistd.h>

static void
handler(int sig, siginfo_t *info, void *ucontext)
{

fprintf(stderr, "sig=%d, si_signo=%d, si_pid=%d, si_uid=%d\n",
	sig, info->si_signo, info->si_pid, info->si_uid);
}

static void
setup(int sig, sigset_t *mask)
{

static const struct sigaction act = { .sa_sigaction = handler, .sa_flags = SA_SIGINFO };
assert(!sigaction(sig, &act, NULL));
assert(!sigaddset(mask, sig));
}

static void
test(int sig)
{

assert(!raise(sig));
assert(!kill(getpid(), sig));
}

int
main(void)
{

sigset_t mask;
assert(!sigemptyset(&mask));

setup(SIGFPE, &mask);
setup(SIGTERM, &mask);

assert(!sigprocmask(SIG_UNBLOCK, &mask, NULL));

test(SIGFPE);
test(SIGTERM);

return 0;
}

Running this on sparc64 userspace works fine:

sig=8, si_signo=8, si_pid=135186, si_uid=1008
sig=8, si_signo=8, si_pid=135186, si_uid=1008
sig=15, si_signo=15, si_pid=135186, si_uid=1008
sig=15, si_signo=15, si_pid=135186, si_uid=1008

However, on sparc32 things go wrong:

sig=8, si_signo=8, si_pid=135192, si_uid=1008
sig=8, si_signo=8, si_pid=1008, si_uid=0
sig=15, si_signo=15, si_pid=135192, si_uid=1008
sig=15, si_signo=15, si_pid=135192, si_uid=1008

When digging the code up and down I came up with this patch, which fixes the 
issue for me:

diff --git a/arch/sparc/kernel/signal32.c b/arch/sparc/kernel/signal32.c
index 54a6159b9cd8..a05e271e0924 100644
--- a/arch/sparc/kernel/signal32.c
+++ b/arch/sparc/kernel/signal32.c
@@ -87,7 +87,7 @@ int copy_siginfo_to_user32(compat_siginfo_t __user *to, 
        err = __put_user(from->si_signo, &to->si_signo);
        err |= __put_user(from->si_errno, &to->si_errno);
        err |= __put_user(from->si_code, &to->si_code);
-       if (from->si_code < 0)
+       if (SI_FROMUSER(from))
                err |= __copy_to_user(&to->_sifields._pad, &from->_sifields._pad, SI_PAD_SIZE);
        else {
                switch (siginfo_layout(from->si_signo, from->si_code)) {

I suspect it was broken by cc731525f26af85a1c3537da41e0abd1d35e0bdb as it is 
reported that it worked in 4.5.0. Looking at that commit other archs (at least 
arm64, mips, parisc, powerpc, s390, tile, x86) have the same code, but 
different default-cases. From a quick glance arm64, parisc, and x86 do not 
have a default case anymore since that commit, while the others do not seem to 
have changed their default case. However it could be that siginfo_layout() 
behaves slightly different than the mask code used before in the switch 
statements. I'm currently building a 4.14.9 kernel for my HP C8000, will 
report the test results once that is done.

Eike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: sending SIGFPE via kill() returns wrong values in si_pid and si_uid

[Eric W. Biederman]

Rolf Eike Beer <eike-kernel@sf-tec.de> writes:

> This originally came up as a failure in the testsuite of strace on Gentoo 
> (T5120, 64 bit kernel, 32 bit userspace) and was reported here: 
> https://bugs.gentoo.org/643060. This was then reported upstream here: 
> https://github.com/strace/strace/issues/21 . I'll paste the relevant parts of
> those bugs here.
>
> The test program is as follows (done by Dmitry V. Levin): 
>
> #include <assert.h>
> #include <signal.h>
> #include <stdio.h>
> #include <unistd.h>
>
> static void
> handler(int sig, siginfo_t *info, void *ucontext)
> {
>
> fprintf(stderr, "sig=%d, si_signo=%d, si_pid=%d, si_uid=%d\n",
> 	sig, info->si_signo, info->si_pid, info->si_uid);
> }
>
> static void
> setup(int sig, sigset_t *mask)
> {
>
> static const struct sigaction act = { .sa_sigaction = handler, .sa_flags = SA_SIGINFO };
> assert(!sigaction(sig, &act, NULL));
> assert(!sigaddset(mask, sig));
> }
>
> static void
> test(int sig)
> {
>
> assert(!raise(sig));
> assert(!kill(getpid(), sig));
> }
>
> int
> main(void)
> {
>
> sigset_t mask;
> assert(!sigemptyset(&mask));
>
> setup(SIGFPE, &mask);
> setup(SIGTERM, &mask);
>
> assert(!sigprocmask(SIG_UNBLOCK, &mask, NULL));
>
> test(SIGFPE);
> test(SIGTERM);
>
> return 0;
> }
>
> Running this on sparc64 userspace works fine:
>
> sig=8, si_signo=8, si_pid\x135186, si_uid\x1008
> sig=8, si_signo=8, si_pid\x135186, si_uid\x1008
> sig\x15, si_signo\x15, si_pid\x135186, si_uid\x1008
> sig\x15, si_signo\x15, si_pid\x135186, si_uid\x1008
>
> However, on sparc32 things go wrong:
>
> sig=8, si_signo=8, si_pid\x135192, si_uid\x1008
> sig=8, si_signo=8, si_pid\x1008, si_uid=0
> sig\x15, si_signo\x15, si_pid\x135192, si_uid\x1008
> sig\x15, si_signo\x15, si_pid\x135192, si_uid\x1008
>
> When digging the code up and down I came up with this patch, which fixes the 
> issue for me:
>
> diff --git a/arch/sparc/kernel/signal32.c b/arch/sparc/kernel/signal32.c
> index 54a6159b9cd8..a05e271e0924 100644
> --- a/arch/sparc/kernel/signal32.c
> +++ b/arch/sparc/kernel/signal32.c
> @@ -87,7 +87,7 @@ int copy_siginfo_to_user32(compat_siginfo_t __user *to, 
>         err = __put_user(from->si_signo, &to->si_signo);
>         err |= __put_user(from->si_errno, &to->si_errno);
>         err |= __put_user(from->si_code, &to->si_code);
> -       if (from->si_code < 0)
> +       if (SI_FROMUSER(from))
>                 err |= __copy_to_user(&to->_sifields._pad, &from->_sifields._pad, SI_PAD_SIZE);
>         else {
>                 switch (siginfo_layout(from->si_signo, from->si_code)) {
>
> I suspect it was broken by cc731525f26af85a1c3537da41e0abd1d35e0bdb as it is 
> reported that it worked in 4.5.0. Looking at that commit other archs (at least 
> arm64, mips, parisc, powerpc, s390, tile, x86) have the same code, but 
> different default-cases. From a quick glance arm64, parisc, and x86 do not 
> have a default case anymore since that commit, while the others do not seem to 
> have changed their default case. However it could be that siginfo_layout() 
> behaves slightly different than the mask code used before in the switch 
> statements. I'm currently building a 4.14.9 kernel for my HP C8000, will 
> report the test results once that is done.

That is definitely the wrong fix.
However it does verify that your si_code is SI_USER.

The issue is that sparc has ambiguous signal handling, and
si_code of 0 combined with the signal SIGFPE could be either
a real floating point signal or a signal sent with kill.

As it is pointless to send yourself a signal with kill I choose to
preserve the floating point signal.   That is the test is not actually
testing floating point SIGFPE so I don't know that it makes much sense.

I do have some further fixes that I was planning on testing and applying
but I ran out of time to finish last year so I will see about digging
those up.  It is definitely wrong to allow sending SIGFPE to yourself
whent si_code = SI_USER when SIGFPE_FIXME is defined.  I should fix
that as well.

Ideally someone will go through sparc and look at
arch/sparc/kernel/traps_32.c:do_fpe_trap and
arch/sparc/kernel/traps_64.c:do_fpe_common and see if they can remove
the need for FPE_FIXME (aka SI_USER, aka 0).

Eric