* ppc compat v4.16 regression: sending SIGTRAP or SIGFPE via kill() returns wrong values in si_pid and si_uid @ 2018-04-09 15:22 Dmitry V. Levin 2018-04-12 1:34 ` sparc/ppc/arm compat siginfo ABI regressions: sending " Dmitry V. Levin 0 siblings, 1 reply; 38+ messages in thread From: Dmitry V. Levin @ 2018-04-09 15:22 UTC (permalink / raw) To: Eric W. Biederman; +Cc: linuxppc-dev, linux-kernel [-- Attachment #1: Type: text/plain, Size: 536 bytes --] Hi, There seems to be a regression in v4.16 on ppc compat very similar to sparc compat regression reported earlier at https://marc.info/?l=linux-sparc&m=151501500704383 . The symptoms are exactly the same: the same signal_receive test from the strace test suite fails with the same diagnostics: https://build.opensuse.org/public/build/home:ldv_alt/openSUSE_Factory_PowerPC/ppc/strace/_log Unfortunately, I do not have any means to investigate further, so just passing this information on to those who care. -- ldv [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid 2018-04-09 15:22 ppc compat v4.16 regression: sending SIGTRAP or SIGFPE via kill() returns wrong values in si_pid and si_uid Dmitry V. Levin @ 2018-04-12 1:34 ` Dmitry V. Levin 2018-04-12 1:45 ` Linus Torvalds 2018-04-12 9:58 ` Russell King - ARM Linux 0 siblings, 2 replies; 38+ messages in thread From: Dmitry V. Levin @ 2018-04-12 1:34 UTC (permalink / raw) To: Eric W. Biederman Cc: Linus Torvalds, linuxppc-dev, linux-sparc, linux-arm-kernel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1450 bytes --] Hi, On Mon, Apr 09, 2018 at 06:22:53PM +0300, Dmitry V. Levin wrote: > There seems to be a regression in v4.16 on ppc compat very similar > to sparc compat regression reported earlier at > https://marc.info/?l=linux-sparc&m=151501500704383 . > > The symptoms are exactly the same: the same signal_receive test from > the strace test suite fails with the same diagnostics: > https://build.opensuse.org/public/build/home:ldv_alt/openSUSE_Factory_PowerPC/ppc/strace/_log The log is big, just look for "KERNEL BUG". > Unfortunately, I do not have any means to investigate further, > so just passing this information on to those who care. OK, the faulty commit is v4.16-rc1~159^2~39 ("signal/powerpc: Document conflicts with SI_USER and SIGFPE and SIGTRAP"). One might think that a commit called "Document conflicts" shouldn't introduce an ABI regression, but this one definitely does by defining FPE_FIXME and TRAP_FIXME in arch/powerpc/include/uapi/asm/siginfo.h that affect siginfo_layout(). A similar commit v4.16-rc1~159^2~37 ("signal/arm: Document conflicts with SI_USER and SIGFPE") must have introduced a similar ABI regression to compat arm. An earlier commit v4.14-rc1~60^2^2~5 ("signal/sparc: Document a conflict with SI_USER with SIGFPE") introduced a similar ABI regression to compat sparc. There is a clear pattern of sneaking in ABI changes using innocently looking commit messages. -- ldv [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid 2018-04-12 1:34 ` sparc/ppc/arm compat siginfo ABI regressions: sending " Dmitry V. Levin @ 2018-04-12 1:45 ` Linus Torvalds 2018-04-12 9:58 ` Russell King - ARM Linux 1 sibling, 0 replies; 38+ messages in thread From: Linus Torvalds @ 2018-04-12 1:45 UTC (permalink / raw) To: Eric W. Biederman, Linus Torvalds, ppc-dev, linux-sparc, linux-arm-kernel, Linux Kernel Mailing List On Wed, Apr 11, 2018 at 6:34 PM, Dmitry V. Levin <ldv@altlinux.org> wrote: > > There is a clear pattern of sneaking in ABI changes using innocently > looking commit messages. Yes, this siginfo stuff has been a mess. Eric - this needs to stop. Or we need to revert all that garbage entirely. Send a fix. And stop changing the siginfo layout or field values. Linus ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid 2018-04-12 1:34 ` sparc/ppc/arm compat siginfo ABI regressions: sending " Dmitry V. Levin 2018-04-12 1:45 ` Linus Torvalds @ 2018-04-12 9:58 ` Russell King - ARM Linux 2018-04-12 11:03 ` Dmitry V. Levin 1 sibling, 1 reply; 38+ messages in thread From: Russell King - ARM Linux @ 2018-04-12 9:58 UTC (permalink / raw) To: Dmitry V. Levin Cc: Eric W. Biederman, linux-sparc, linuxppc-dev, Linus Torvalds, linux-kernel, linux-arm-kernel On Thu, Apr 12, 2018 at 04:34:35AM +0300, Dmitry V. Levin wrote: > A similar commit v4.16-rc1~159^2~37 > ("signal/arm: Document conflicts with SI_USER and SIGFPE") must have > introduced a similar ABI regression to compat arm. So, could you explain how can this change cause a regression? +#define FPE_FIXME 0 - vfp_raise_sigfpe(0, regs); + vfp_raise_sigfpe(FPE_FIXME, regs); I think you're talking garbage here - look at the damned change. It subsitutes a definition for a constant, and vfp_raise_sigfpe() ends up receiving exactly the same value bother before and after the change. The change is rather incomplete though because it should have also changed: int si_code = 0; as well. So, the commit log is accurate in this case: it _is_ about documenting the conflicting cases between SI_USER and SIGFPE and that bit of the change has no ABI effect. What does slightly annoy me is the creation of uapi/asm/siginfo.h to contain a definition that _isn't_ to be exposed as part of the UAPI. If it's not part of the UAPI, it doesn't belong in a UAPI header, period. In any case, I don't think that is exposed to userspace. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up According to speedtest.net: 8.21Mbps down 510kbps up ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid 2018-04-12 9:58 ` Russell King - ARM Linux @ 2018-04-12 11:03 ` Dmitry V. Levin 2018-04-12 12:19 ` Russell King - ARM Linux 0 siblings, 1 reply; 38+ messages in thread From: Dmitry V. Levin @ 2018-04-12 11:03 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Eric W. Biederman, sparclinux, linuxppc-dev, Linus Torvalds, linux-kernel, linux-arm-kernel [-- Attachment #1: Type: text/plain, Size: 1110 bytes --] On Thu, Apr 12, 2018 at 10:58:11AM +0100, Russell King - ARM Linux wrote: > On Thu, Apr 12, 2018 at 04:34:35AM +0300, Dmitry V. Levin wrote: > > A similar commit v4.16-rc1~159^2~37 > > ("signal/arm: Document conflicts with SI_USER and SIGFPE") must have > > introduced a similar ABI regression to compat arm. > > So, could you explain how can this change cause a regression? > > +#define FPE_FIXME 0 > - vfp_raise_sigfpe(0, regs); > + vfp_raise_sigfpe(FPE_FIXME, regs); No, this hunk hasn't caused the regression, but another one did: diff --git a/arch/arm/include/uapi/asm/siginfo.h b/arch/arm/include/uapi/asm/siginfo.h new file mode 100644 index 0000000..d051388 --- /dev/null +++ b/arch/arm/include/uapi/asm/siginfo.h @@ -0,0 +1,13 @@ +#ifndef __ASM_SIGINFO_H +#define __ASM_SIGINFO_H + +#include <asm-generic/siginfo.h> + +/* + * SIGFPE si_codes + */ +#ifdef __KERNEL__ +#define FPE_FIXME 0 /* Broken dup of SI_USER */ +#endif /* __KERNEL__ */ + +#endif This is due to FPE_FIXME handling in kernel/signal.c -- ldv [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid 2018-04-12 11:03 ` Dmitry V. Levin @ 2018-04-12 12:19 ` Russell King - ARM Linux 2018-04-12 12:49 ` Dmitry V. Levin 0 siblings, 1 reply; 38+ messages in thread From: Russell King - ARM Linux @ 2018-04-12 12:19 UTC (permalink / raw) To: Dmitry V. Levin Cc: Eric W. Biederman, sparclinux, linuxppc-dev, Linus Torvalds, linux-kernel, linux-arm-kernel On Thu, Apr 12, 2018 at 02:03:14PM +0300, Dmitry V. Levin wrote: > On Thu, Apr 12, 2018 at 10:58:11AM +0100, Russell King - ARM Linux wrote: > > On Thu, Apr 12, 2018 at 04:34:35AM +0300, Dmitry V. Levin wrote: > > > A similar commit v4.16-rc1~159^2~37 > > > ("signal/arm: Document conflicts with SI_USER and SIGFPE") must have > > > introduced a similar ABI regression to compat arm. > > > > So, could you explain how can this change cause a regression? > > > > +#define FPE_FIXME 0 > > - vfp_raise_sigfpe(0, regs); > > + vfp_raise_sigfpe(FPE_FIXME, regs); > > No, this hunk hasn't caused the regression, but another one did: > > diff --git a/arch/arm/include/uapi/asm/siginfo.h b/arch/arm/include/uapi/asm/siginfo.h > new file mode 100644 > index 0000000..d051388 > --- /dev/null > +++ b/arch/arm/include/uapi/asm/siginfo.h > @@ -0,0 +1,13 @@ > +#ifndef __ASM_SIGINFO_H > +#define __ASM_SIGINFO_H > + > +#include <asm-generic/siginfo.h> > + > +/* > + * SIGFPE si_codes > + */ > +#ifdef __KERNEL__ > +#define FPE_FIXME 0 /* Broken dup of SI_USER */ > +#endif /* __KERNEL__ */ > + > +#endif > > This is due to FPE_FIXME handling in kernel/signal.c Building strace 4.22 on ARM and running the test suite reveals no problems with the signal_receive test, tested on both 4.14 and 4.16 kernels - there's no "KERNEL BUG" reports in any of the test results. However, stock strace 4.22 source doesn't appear to contain the "KERNEL BUG" string anywhere, so this may be a Suse specific addition to the test: ~/src/strace-4.22$ grep -ri 'KERNEL BUG' . ./strace.1:Arguably, every instance of such behavior is a kernel bug.) ./strace.1.in:Arguably, every instance of such behavior is a kernel bug.) ./NEWS: * Worked around a kernel bug in tracing privileged executables. ./ChangeLog: aarch64: workaround gcc+kernel bug. ./ChangeLog: tests: workaround kernel bugs in seccomp-strict.test and prctl-seccomp-strict.test ./ChangeLog: instead. We don't want the testsuite failing due to kernel bugs. ./ChangeLog: First guess is that it's a workaround for old kernel bugs: ./ChangeLog: This kernel bug is fixed long ago. This change removes the workaround. Any ideas where the "KERNEL BUG" in Suse builds is coming from? Any ideas how to test it on other architectures (iow, where can we get source that contains this test?) Based on previous experience, unfortunately folk don't tend to report user ABI regressions to kernel developers, so we'd probably never know that there's a problem - I do think the safer thing would've been to leave it well alone, and just accept that we'll end up copying more words to userspace than is actually intended. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up According to speedtest.net: 8.21Mbps down 510kbps up ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid 2018-04-12 12:19 ` Russell King - ARM Linux @ 2018-04-12 12:49 ` Dmitry V. Levin 2018-04-12 13:14 ` Russell King - ARM Linux 0 siblings, 1 reply; 38+ messages in thread From: Dmitry V. Levin @ 2018-04-12 12:49 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Eric W. Biederman, sparclinux, linuxppc-dev, Linus Torvalds, linux-kernel, linux-arm-kernel [-- Attachment #1: Type: text/plain, Size: 3569 bytes --] On Thu, Apr 12, 2018 at 01:19:49PM +0100, Russell King - ARM Linux wrote: > On Thu, Apr 12, 2018 at 02:03:14PM +0300, Dmitry V. Levin wrote: > > On Thu, Apr 12, 2018 at 10:58:11AM +0100, Russell King - ARM Linux wrote: > > > On Thu, Apr 12, 2018 at 04:34:35AM +0300, Dmitry V. Levin wrote: > > > > A similar commit v4.16-rc1~159^2~37 > > > > ("signal/arm: Document conflicts with SI_USER and SIGFPE") must have > > > > introduced a similar ABI regression to compat arm. > > > > > > So, could you explain how can this change cause a regression? > > > > > > +#define FPE_FIXME 0 > > > - vfp_raise_sigfpe(0, regs); > > > + vfp_raise_sigfpe(FPE_FIXME, regs); > > > > No, this hunk hasn't caused the regression, but another one did: > > > > diff --git a/arch/arm/include/uapi/asm/siginfo.h b/arch/arm/include/uapi/asm/siginfo.h > > new file mode 100644 > > index 0000000..d051388 > > --- /dev/null > > +++ b/arch/arm/include/uapi/asm/siginfo.h > > @@ -0,0 +1,13 @@ > > +#ifndef __ASM_SIGINFO_H > > +#define __ASM_SIGINFO_H > > + > > +#include <asm-generic/siginfo.h> > > + > > +/* > > + * SIGFPE si_codes > > + */ > > +#ifdef __KERNEL__ > > +#define FPE_FIXME 0 /* Broken dup of SI_USER */ > > +#endif /* __KERNEL__ */ > > + > > +#endif > > > > This is due to FPE_FIXME handling in kernel/signal.c > > Building strace 4.22 on ARM and running the test suite reveals no > problems with the signal_receive test, tested on both 4.14 and 4.16 > kernels - there's no "KERNEL BUG" reports in any of the test results. https://build.opensuse.org/public/build/home:ldv_alt/openSUSE_Factory_ARM/armv7l/strace/_log - the test just fails there with [ 50s] + uname -a [ 50s] Linux armbuild01 4.16.0-1-lpae #1 SMP PREEMPT Wed Apr 4 13:35:56 UTC 2018 (e16f96d) armv7l armv7l armv7l GNU/Linux ... [ 570s] FAIL: signal_receive.gen [ 570s] ---- SIGFPE {si_signo=SIGFPE, si_code=SI_USER, si_pid=25332, si_uid=399} --- [ 570s] +--- SIGFPE {si_signo=SIGFPE, si_code=SI_USER, si_pid=25332, si_uid=0} --- [ 570s] signal_receive.gen.test: failed test: ../../strace -a16 -e trace=kill ../signal_receive output mismatch > However, stock strace 4.22 source doesn't appear to contain the > "KERNEL BUG" string anywhere, so this may be a Suse specific addition > to the test: The "KERNEL BUG" diagnostics I was talking about was added to strace yesterday as a part of workaround commit, see https://github.com/strace/strace/commit/34c7794cc16e2511eda7b1d5767c655a83b17309 Before that change the test just failed. [...] > Any ideas where the "KERNEL BUG" in Suse builds is coming from? strace developers use OBS to test strace.git for regressions. The build environment is provided by OBS, all the rest comes from strace.git. > Any ideas how to test it on other architectures (iow, where can we get > source that contains this test?) Just use master branch of https://github.com/strace/strace or https://gitlab.com/strace/strace (they are the same). > Based on previous experience, unfortunately folk don't tend to report > user ABI regressions to kernel developers, so we'd probably never know > that there's a problem - I do think the safer thing would've been to > leave it well alone, and just accept that we'll end up copying more > words to userspace than is actually intended. Well, these changes caused visible regressions in strace test suite on arm, ppc, and sparc - this is the reason why I have reported them to kernel developers. -- ldv [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid 2018-04-12 12:49 ` Dmitry V. Levin @ 2018-04-12 13:14 ` Russell King - ARM Linux 2018-04-12 16:50 ` Linus Torvalds 0 siblings, 1 reply; 38+ messages in thread From: Russell King - ARM Linux @ 2018-04-12 13:14 UTC (permalink / raw) To: Dmitry V. Levin, Eric W. Biederman Cc: sparclinux, linuxppc-dev, Linus Torvalds, linux-kernel, linux-arm-kernel On Thu, Apr 12, 2018 at 03:49:28PM +0300, Dmitry V. Levin wrote: > The "KERNEL BUG" diagnostics I was talking about was added to strace yesterday > as a part of workaround commit, see > https://github.com/strace/strace/commit/34c7794cc16e2511eda7b1d5767c655a83b17309 > Before that change the test just failed. Ah, seeing the test case really helps to see exactly what and why it's broken. Yes, Eric's commit was definitely wrong and needs to be reverted, because it incorrectly changes what happens when kill(1) is used to deliver a SIGFPE signal to a process. Eric, please sort this out - you have a much better handle on whether there are any dependencies here that would need to be resolved from a simple revert of the offending commits, but that revert must happen because you've caused a user visible regression. The original code _was_ safe even if it wasn't correct to the specs, as we'd end up copying the si_addr field (as the si_pid copy) and a zeroed field as the si_uid copy. It was just that si_code was technically wrong, and that's something that would be even more dangerous to change now. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up According to speedtest.net: 8.21Mbps down 510kbps up ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid 2018-04-12 13:14 ` Russell King - ARM Linux @ 2018-04-12 16:50 ` Linus Torvalds 2018-04-12 17:20 ` Russell King - ARM Linux 2018-04-12 17:35 ` Dmitry V. Levin 0 siblings, 2 replies; 38+ messages in thread From: Linus Torvalds @ 2018-04-12 16:50 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Dmitry V. Levin, Eric W. Biederman, sparclinux, ppc-dev, Linux Kernel Mailing List, linux-arm-kernel [-- Attachment #1: Type: text/plain, Size: 429 bytes --] Does this attached patch perhaps fix the ARM case? It just uses FPE_FLTUNK as the default si_code for SIGFPE, which seems sane enough. And then gets rid of FPE_FIXME, which should resolve the nasty case. Hmm? Entirely untested, and I didn't really look at the test-case in question since I can't really run it anyway. Well, I could run it all on x86-64, but it doesn't have that FPE_FIXME case at all. Linus [-- Attachment #2: patch.diff --] [-- Type: text/x-patch, Size: 1163 bytes --] arch/arm/include/uapi/asm/siginfo.h | 7 ------- arch/arm/vfp/vfpmodule.c | 4 ++-- 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/arch/arm/include/uapi/asm/siginfo.h b/arch/arm/include/uapi/asm/siginfo.h index d0513880be21..d87beeedb4c4 100644 --- a/arch/arm/include/uapi/asm/siginfo.h +++ b/arch/arm/include/uapi/asm/siginfo.h @@ -3,11 +3,4 @@ #include <asm-generic/siginfo.h> -/* - * SIGFPE si_codes - */ -#ifdef __KERNEL__ -#define FPE_FIXME 0 /* Broken dup of SI_USER */ -#endif /* __KERNEL__ */ - #endif diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c index 4c375e11ae95..012c6e690303 100644 --- a/arch/arm/vfp/vfpmodule.c +++ b/arch/arm/vfp/vfpmodule.c @@ -251,13 +251,13 @@ static void vfp_panic(char *reason, u32 inst) */ static void vfp_raise_exceptions(u32 exceptions, u32 inst, u32 fpscr, struct pt_regs *regs) { - int si_code = 0; + int si_code = FPE_FLTUNK; pr_debug("VFP: raising exceptions %08x\n", exceptions); if (exceptions == VFP_EXCEPTION_ERROR) { vfp_panic("unhandled bounce", inst); - vfp_raise_sigfpe(FPE_FIXME, regs); + vfp_raise_sigfpe(si_code, regs); return; } ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid 2018-04-12 16:50 ` Linus Torvalds @ 2018-04-12 17:20 ` Russell King - ARM Linux 2018-04-12 17:22 ` Linus Torvalds 2018-04-12 17:35 ` Dmitry V. Levin 1 sibling, 1 reply; 38+ messages in thread From: Russell King - ARM Linux @ 2018-04-12 17:20 UTC (permalink / raw) To: Linus Torvalds Cc: Linux Kernel Mailing List, Dmitry V. Levin, Eric W. Biederman, sparclinux, ppc-dev, linux-arm-kernel On Thu, Apr 12, 2018 at 09:50:26AM -0700, Linus Torvalds wrote: > Does this attached patch perhaps fix the ARM case? > > It just uses FPE_FLTUNK as the default si_code for SIGFPE, which seems > sane enough. And then gets rid of FPE_FIXME, which should resolve the > nasty case. > > Hmm? Entirely untested, and I didn't really look at the test-case in > question since I can't really run it anyway. I'll test tomorrow and let you know. > arch/arm/include/uapi/asm/siginfo.h | 7 ------- > arch/arm/vfp/vfpmodule.c | 4 ++-- > 2 files changed, 2 insertions(+), 9 deletions(-) > > diff --git a/arch/arm/include/uapi/asm/siginfo.h b/arch/arm/include/uapi/asm/siginfo.h > index d0513880be21..d87beeedb4c4 100644 > --- a/arch/arm/include/uapi/asm/siginfo.h > +++ b/arch/arm/include/uapi/asm/siginfo.h > @@ -3,11 +3,4 @@ > > #include <asm-generic/siginfo.h> > > -/* > - * SIGFPE si_codes > - */ > -#ifdef __KERNEL__ > -#define FPE_FIXME 0 /* Broken dup of SI_USER */ > -#endif /* __KERNEL__ */ > - > #endif This file was created to contain FPE_FIXME, by the "signal/arm: Document conflicts with SI_USER and SIGFPE" commit so if we're removing it, it would be better to remove the whole file. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up According to speedtest.net: 8.21Mbps down 510kbps up ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid 2018-04-12 17:20 ` Russell King - ARM Linux @ 2018-04-12 17:22 ` Linus Torvalds 2018-04-13 9:42 ` Russell King - ARM Linux 0 siblings, 1 reply; 38+ messages in thread From: Linus Torvalds @ 2018-04-12 17:22 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Linux Kernel Mailing List, Dmitry V. Levin, Eric W. Biederman, sparclinux, ppc-dev, linux-arm-kernel On Thu, Apr 12, 2018 at 10:20 AM, Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > > This file was created to contain FPE_FIXME, by the "signal/arm: Document > conflicts with SI_USER and SIGFPE" commit so if we're removing it, it > would be better to remove the whole file. Fair enough. I'm not going to commit that anyway since I can't test it, but yes, if it tests ok that sounds like the right thing to do. Linus ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid 2018-04-12 17:22 ` Linus Torvalds @ 2018-04-13 9:42 ` Russell King - ARM Linux 2018-04-13 16:33 ` Linus Torvalds 0 siblings, 1 reply; 38+ messages in thread From: Russell King - ARM Linux @ 2018-04-13 9:42 UTC (permalink / raw) To: Linus Torvalds Cc: Linux Kernel Mailing List, Dmitry V. Levin, Eric W. Biederman, sparclinux, ppc-dev, linux-arm-kernel On Thu, Apr 12, 2018 at 10:22:15AM -0700, Linus Torvalds wrote: > On Thu, Apr 12, 2018 at 10:20 AM, Russell King - ARM Linux > <linux@armlinux.org.uk> wrote: > > > > This file was created to contain FPE_FIXME, by the "signal/arm: Document > > conflicts with SI_USER and SIGFPE" commit so if we're removing it, it > > would be better to remove the whole file. > > Fair enough. I'm not going to commit that anyway since I can't test > it, but yes, if it tests ok that sounds like the right thing to do. Yes, it does solve the problem at hand with strace - the exact patch I tested against 4.16 is below. Testing this exact code path (exceptions set to VFP_EXCEPTION_ERROR) is something that can only happen if the hardware does something stupid, and I don't have a way of making it do that, so the code path can't be tested. However, FPE_FLTUNK is not defined in older kernels, so while we can fix it this way for the current merge window, that doesn't help 4.16. How we solve that depends what happens with Eric's patch (266da65e9156 ("signal: Add FPE_FLTUNK si_code for undiagnosable fp exceptions")) that introduces FPE_FLTUNK - and there's also the problem of NSIGFPE, which kernel/signal.c uses in the path that selects the siginfo layout. Given that the path we're talking about is unlikely to happen (as mentioned in my second paragraph) I still think reverting Eric's patch is the right way forward for older kernels. (Note, my previous comment about the si_code initialiser was incorrect.) diff --git a/arch/arm/include/uapi/asm/siginfo.h b/arch/arm/include/uapi/asm/siginfo.h deleted file mode 100644 index d0513880be21..000000000000 --- a/arch/arm/include/uapi/asm/siginfo.h +++ /dev/null @@ -1,13 +0,0 @@ -#ifndef __ASM_SIGINFO_H -#define __ASM_SIGINFO_H - -#include <asm-generic/siginfo.h> - -/* - * SIGFPE si_codes - */ -#ifdef __KERNEL__ -#define FPE_FIXME 0 /* Broken dup of SI_USER */ -#endif /* __KERNEL__ */ - -#endif diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c index 4c375e11ae95..8a1a5e6048d2 100644 --- a/arch/arm/vfp/vfpmodule.c +++ b/arch/arm/vfp/vfpmodule.c @@ -28,6 +28,10 @@ #include <asm/thread_notify.h> #include <asm/vfp.h> +#ifndef FPE_FLTUNK +#define FPE_FLTUNK 14 +#endif + #include "vfpinstr.h" #include "vfp.h" @@ -257,7 +261,7 @@ static void vfp_raise_exceptions(u32 exceptions, u32 inst, u32 fpscr, struct pt_ if (exceptions == VFP_EXCEPTION_ERROR) { vfp_panic("unhandled bounce", inst); - vfp_raise_sigfpe(FPE_FIXME, regs); + vfp_raise_sigfpe(FPE_FLTUNK, regs); return; } -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up According to speedtest.net: 8.21Mbps down 510kbps up ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid 2018-04-13 9:42 ` Russell King - ARM Linux @ 2018-04-13 16:33 ` Linus Torvalds 2018-04-13 17:08 ` Dave Martin 0 siblings, 1 reply; 38+ messages in thread From: Linus Torvalds @ 2018-04-13 16:33 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Linux Kernel Mailing List, Dmitry V. Levin, Eric W. Biederman, sparclinux, ppc-dev, linux-arm-kernel [-- Attachment #1: Type: text/plain, Size: 1199 bytes --] On Fri, Apr 13, 2018 at 2:42 AM, Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > > Yes, it does solve the problem at hand with strace - the exact patch I > tested against 4.16 is below. Ok, good. > However, FPE_FLTUNK is not defined in older kernels, so while we can > fix it this way for the current merge window, that doesn't help 4.16. I wonder if we should even bother with FPE_FLTUNK. I suspect we might as well use FPE_FLTINV, I suspect, and not have this complexity at all. That case is not worth worrying about, since it's a "this shouldn't happen anyway" and the *real* reason will be in the kernel logs due to vfs_panic(). So it's not like this is something that the user should ever care about the si_code about. > Given that the path we're talking about is unlikely to happen (as > mentioned in my second paragraph) I still think reverting Eric's patch > is the right way forward for older kernels. I'd much rather get rid of that FPE_FIXME, and leave that whole mess behind. So the attached patch seems simple and should work with 4.16 too. Let's not leave this as some kind of nasty maintenance issue, and just go for simple and stupid. Hmm? Linus [-- Attachment #2: patch.diff --] [-- Type: text/x-patch, Size: 1037 bytes --] arch/arm/include/uapi/asm/siginfo.h | 13 ------------- arch/arm/vfp/vfpmodule.c | 2 +- 2 files changed, 1 insertion(+), 14 deletions(-) diff --git a/arch/arm/include/uapi/asm/siginfo.h b/arch/arm/include/uapi/asm/siginfo.h deleted file mode 100644 index d0513880be21..000000000000 --- a/arch/arm/include/uapi/asm/siginfo.h +++ /dev/null @@ -1,13 +0,0 @@ -#ifndef __ASM_SIGINFO_H -#define __ASM_SIGINFO_H - -#include <asm-generic/siginfo.h> - -/* - * SIGFPE si_codes - */ -#ifdef __KERNEL__ -#define FPE_FIXME 0 /* Broken dup of SI_USER */ -#endif /* __KERNEL__ */ - -#endif diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c index 4c375e11ae95..af4ee2cef2f9 100644 --- a/arch/arm/vfp/vfpmodule.c +++ b/arch/arm/vfp/vfpmodule.c @@ -257,7 +257,7 @@ static void vfp_raise_exceptions(u32 exceptions, u32 inst, u32 fpscr, struct pt_ if (exceptions == VFP_EXCEPTION_ERROR) { vfp_panic("unhandled bounce", inst); - vfp_raise_sigfpe(FPE_FIXME, regs); + vfp_raise_sigfpe(FPE_FLTINV, regs); return; } ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid 2018-04-13 16:33 ` Linus Torvalds @ 2018-04-13 17:08 ` Dave Martin 2018-04-13 17:54 ` Russell King - ARM Linux 0 siblings, 1 reply; 38+ messages in thread From: Dave Martin @ 2018-04-13 17:08 UTC (permalink / raw) To: Linus Torvalds Cc: Russell King - ARM Linux, Linux Kernel Mailing List, Dmitry V. Levin, Eric W. Biederman, sparclinux, ppc-dev, linux-arm-kernel On Fri, Apr 13, 2018 at 09:33:17AM -0700, Linus Torvalds wrote: > On Fri, Apr 13, 2018 at 2:42 AM, Russell King - ARM Linux > <linux@armlinux.org.uk> wrote: > > > > Yes, it does solve the problem at hand with strace - the exact patch I > > tested against 4.16 is below. > > Ok, good. > > > However, FPE_FLTUNK is not defined in older kernels, so while we can > > fix it this way for the current merge window, that doesn't help 4.16. > > I wonder if we should even bother with FPE_FLTUNK. > > I suspect we might as well use FPE_FLTINV, I suspect, and not have > this complexity at all. That case is not worth worrying about, since > it's a "this shouldn't happen anyway" and the *real* reason will be in > the kernel logs due to vfs_panic(). > > So it's not like this is something that the user should ever care > about the si_code about. Ack, my intended meaning for FPE_FLTUNK is that the fp exception is either spurious or we can't tell easily (or possibly at all) which FPE_XXX should be returned. It's up to userspace to figure it out if it really cares. Previously we were accidentally returning SI_USER in si_code for arm64. This case on arm looks like a more serious error for which FPE_FLTINV may be more appropriate anyway. [...] Cheers ---Dave ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid 2018-04-13 17:08 ` Dave Martin @ 2018-04-13 17:54 ` Russell King - ARM Linux 2018-04-13 18:23 ` Linus Torvalds 2018-04-13 18:35 ` sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid Dave Martin 0 siblings, 2 replies; 38+ messages in thread From: Russell King - ARM Linux @ 2018-04-13 17:54 UTC (permalink / raw) To: Dave Martin Cc: Linus Torvalds, Linux Kernel Mailing List, Dmitry V. Levin, Eric W. Biederman, sparclinux, ppc-dev, linux-arm-kernel On Fri, Apr 13, 2018 at 06:08:28PM +0100, Dave Martin wrote: > On Fri, Apr 13, 2018 at 09:33:17AM -0700, Linus Torvalds wrote: > > On Fri, Apr 13, 2018 at 2:42 AM, Russell King - ARM Linux > > <linux@armlinux.org.uk> wrote: > > > > > > Yes, it does solve the problem at hand with strace - the exact patch I > > > tested against 4.16 is below. > > > > Ok, good. > > > > > However, FPE_FLTUNK is not defined in older kernels, so while we can > > > fix it this way for the current merge window, that doesn't help 4.16. > > > > I wonder if we should even bother with FPE_FLTUNK. > > > > I suspect we might as well use FPE_FLTINV, I suspect, and not have > > this complexity at all. That case is not worth worrying about, since > > it's a "this shouldn't happen anyway" and the *real* reason will be in > > the kernel logs due to vfs_panic(). > > > > So it's not like this is something that the user should ever care > > about the si_code about. > > Ack, my intended meaning for FPE_FLTUNK is that the fp exception is > either spurious or we can't tell easily (or possibly at all) which > FPE_XXX should be returned. It's up to userspace to figure it out > if it really cares. Previously we were accidentally returning SI_USER > in si_code for arm64. > > This case on arm looks like a more serious error for which FPE_FLTINV > may be more appropriate anyway. No. The cases where we get to this point are: 1. A trap concerning a coprocessor register transfer instruction (iow, move between a VFP register and ARM register.) 2. A trap concerning a coprocessor register load or save instruction. (In both of these, "concerning" means that the VFP hardware provides such an instruction as the reason for the fault, *not* that it is the faulting instruction.) 3. A combination of the exception bits (EX and DEX) on certain VFP implementations. All of these can be summarised as "the hardware went wrong in some way" rather than "the user program did something wrong." FPE_FLTINV means "floating point invalid operation". Does it really cover the case where hardware has failed, or is it intended to cover the case where userspace did something wrong and asked for an invalid operation from the FP hardware? -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up According to speedtest.net: 8.21Mbps down 510kbps up ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid 2018-04-13 17:54 ` Russell King - ARM Linux @ 2018-04-13 18:23 ` Linus Torvalds 2018-04-13 18:45 ` Dave Martin 2018-04-13 18:35 ` sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid Dave Martin 1 sibling, 1 reply; 38+ messages in thread From: Linus Torvalds @ 2018-04-13 18:23 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Dave Martin, Linux Kernel Mailing List, Dmitry V. Levin, Eric W. Biederman, sparclinux, ppc-dev, linux-arm-kernel On Fri, Apr 13, 2018 at 10:54 AM, Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > > FPE_FLTINV means "floating point invalid operation". Does it really > cover the case where hardware has failed, or is it intended to cover > the case where userspace did something wrong and asked for an invalid > operation from the FP hardware? Note that the number of people who actually look at the si_code is approximately zero. But the ones that _do_ check the si_code are certainly not going to check it against a new code that they don't know about. I suspect that if you start searching for FLT_xyz occurrences in code, approximately 100% of them are from the kernel code that generates them, not from any actual users. So I'd be very surprised if you can find *anybody* who cares about that exact value (with the possible exceptions of test-suites). Sadly, google code-search is no more. It was useful for things like that. Linus ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid 2018-04-13 18:23 ` Linus Torvalds @ 2018-04-13 18:45 ` Dave Martin 2018-04-13 19:53 ` Linus Torvalds 0 siblings, 1 reply; 38+ messages in thread From: Dave Martin @ 2018-04-13 18:45 UTC (permalink / raw) To: Linus Torvalds Cc: Russell King - ARM Linux, Linux Kernel Mailing List, Dmitry V. Levin, Eric W. Biederman, sparclinux, ppc-dev, linux-arm-kernel On Fri, Apr 13, 2018 at 11:23:36AM -0700, Linus Torvalds wrote: > On Fri, Apr 13, 2018 at 10:54 AM, Russell King - ARM Linux > <linux@armlinux.org.uk> wrote: > > > > FPE_FLTINV means "floating point invalid operation". Does it really > > cover the case where hardware has failed, or is it intended to cover > > the case where userspace did something wrong and asked for an invalid > > operation from the FP hardware? > > Note that the number of people who actually look at the si_code is > approximately zero. > > But the ones that _do_ check the si_code are certainly not going to > check it against a new code that they don't know about. > > I suspect that if you start searching for FLT_xyz occurrences in code, > approximately 100% of them are from the kernel code that generates > them, not from any actual users. > > So I'd be very surprised if you can find *anybody* who cares about > that exact value (with the possible exceptions of test-suites). > > Sadly, google code-search is no more. It was useful for things like that. I've found https://codesearch.debian.net/ useful for digging into this kind of question, though it tends to throw up a lot of false positives. Most uses I've seen do nothing more than use the FPE_xyz value to format diagnostic messages while dying. I struggled to find code that made a meaningful functional decision based on the value, though that's not proof... Cheers ---Dave ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid 2018-04-13 18:45 ` Dave Martin @ 2018-04-13 19:53 ` Linus Torvalds 2018-04-15 13:12 ` Russell King - ARM Linux 0 siblings, 1 reply; 38+ messages in thread From: Linus Torvalds @ 2018-04-13 19:53 UTC (permalink / raw) To: Dave Martin Cc: Russell King - ARM Linux, Linux Kernel Mailing List, Dmitry V. Levin, Eric W. Biederman, sparclinux, ppc-dev, linux-arm-kernel On Fri, Apr 13, 2018 at 11:45 AM, Dave Martin <Dave.Martin@arm.com> wrote: > > Most uses I've seen do nothing more than use the FPE_xyz value to > format diagnostic messages while dying. I struggled to find code that > made a meaningful functional decision based on the value, though that's > not proof... Yeah. I've seen code that cares about SIGFPE deeply, but it's almost invariably about some emulated environment (eg Java VM, or CPU emulation). And the siginfo data is basically never good enough for those environments anyway on its own, so they will go and look at the actual instruction that caused the fault and the register state instead, because they need *all* the information. The cases that use si_code are the ones that just trapped signals in order to give a more helpful abort message. So I could certainly imagine that si_code is actually used by somebody who then decides to actuall act differently on it, but aside from perhaps printing out a different message, it sounds far-fetched. Linus ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid 2018-04-13 19:53 ` Linus Torvalds @ 2018-04-15 13:12 ` Russell King - ARM Linux 2018-04-15 15:22 ` Eric W. Biederman 2018-04-15 15:56 ` [RFC PATCH 0/3] Dealing with the aliases of SI_USER Eric W. Biederman 0 siblings, 2 replies; 38+ messages in thread From: Russell King - ARM Linux @ 2018-04-15 13:12 UTC (permalink / raw) To: Linus Torvalds Cc: Dave Martin, Linux Kernel Mailing List, Dmitry V. Levin, Eric W. Biederman, sparclinux, ppc-dev, linux-arm-kernel On Fri, Apr 13, 2018 at 12:53:49PM -0700, Linus Torvalds wrote: > On Fri, Apr 13, 2018 at 11:45 AM, Dave Martin <Dave.Martin@arm.com> wrote: > > > > Most uses I've seen do nothing more than use the FPE_xyz value to > > format diagnostic messages while dying. I struggled to find code that > > made a meaningful functional decision based on the value, though that's > > not proof... > > Yeah. I've seen code that cares about SIGFPE deeply, but it's almost > invariably about some emulated environment (eg Java VM, or CPU > emulation). > > And the siginfo data is basically never good enough for those > environments anyway on its own, so they will go and look at the actual > instruction that caused the fault and the register state instead, > because they need *all* the information. > > The cases that use si_code are the ones that just trapped signals in > order to give a more helpful abort message. > > So I could certainly imagine that si_code is actually used by somebody > who then decides to actuall act differently on it, but aside from > perhaps printing out a different message, it sounds far-fetched. Okay, in that case let's just use FPE_FLTINV. That makes the patch easily back-portable for stable kernels. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up According to speedtest.net: 8.21Mbps down 510kbps up ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid 2018-04-15 13:12 ` Russell King - ARM Linux @ 2018-04-15 15:22 ` Eric W. Biederman 2018-04-15 15:56 ` [RFC PATCH 0/3] Dealing with the aliases of SI_USER Eric W. Biederman 1 sibling, 0 replies; 38+ messages in thread From: Eric W. Biederman @ 2018-04-15 15:22 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Linus Torvalds, Dave Martin, Linux Kernel Mailing List, Dmitry V. Levin, sparclinux, ppc-dev, linux-arm-kernel Russell King - ARM Linux <linux@armlinux.org.uk> writes: > On Fri, Apr 13, 2018 at 12:53:49PM -0700, Linus Torvalds wrote: >> On Fri, Apr 13, 2018 at 11:45 AM, Dave Martin <Dave.Martin@arm.com> wrote: >> > >> > Most uses I've seen do nothing more than use the FPE_xyz value to >> > format diagnostic messages while dying. I struggled to find code that >> > made a meaningful functional decision based on the value, though that's >> > not proof... >> >> Yeah. I've seen code that cares about SIGFPE deeply, but it's almost >> invariably about some emulated environment (eg Java VM, or CPU >> emulation). >> >> And the siginfo data is basically never good enough for those >> environments anyway on its own, so they will go and look at the actual >> instruction that caused the fault and the register state instead, >> because they need *all* the information. >> >> The cases that use si_code are the ones that just trapped signals in >> order to give a more helpful abort message. >> >> So I could certainly imagine that si_code is actually used by somebody >> who then decides to actuall act differently on it, but aside from >> perhaps printing out a different message, it sounds far-fetched. > > Okay, in that case let's just use FPE_FLTINV. That makes the patch > easily back-portable for stable kernels. If we want to I don't think backporting 266da65e9156 ("signal: Add FPE_FLTUNK si_code for undiagnosable fp exceptions") would be at all difficult. What it is changing has been stable for quite a while. The surroundings might change and so it might require some trivial manual fixup but I don't expect any problems. Not that I want to derail the consensus but if we want to backport similar fixes for arm64 or the other architectures that wind up using FPE_FLTUNK for their fix we would need to backport 266da65e9156 anyway. Eric ^ permalink raw reply [flat|nested] 38+ messages in thread
* [RFC PATCH 0/3] Dealing with the aliases of SI_USER 2018-04-15 13:12 ` Russell King - ARM Linux 2018-04-15 15:22 ` Eric W. Biederman @ 2018-04-15 15:56 ` Eric W. Biederman 2018-04-15 15:57 ` [RFC PATCH 1/3] signal: Ensure every siginfo we send has all bits initialized Eric W. Biederman ` (3 more replies) 1 sibling, 4 replies; 38+ messages in thread From: Eric W. Biederman @ 2018-04-15 15:56 UTC (permalink / raw) To: Linus Torvalds Cc: Dave Martin, Linux Kernel Mailing List, Dmitry V. Levin, sparclinux, ppc-dev, linux-arm-kernel, Russell King - ARM Linux, linux-arch Linus, Would you consider the patchset below for -rc2? Dealing with the aliases of SI_USER has been a challenge as we have had a b0rked ABI in some cases since 2.5. So far no one except myself has suggested that changing the si_code of from 0 to something else for those problematic aliases of SI_USER is going to be a problem. So it looks like just fixing the issue is a real possibility. Fixing the cases that do kill(SIGFPE, ...) because at least test cases care seems important. The best fixes to backport appear to be the real architecture fixes that remove the aliases for SI_USER as those are deep fixes that fundamentally fix the problems, and are also very small changes. I am not yet brave enough to merge architectural fixes like that without arch maintainers buy-in. Getting at least an ack if nothing else takes a little bit of time. Still we have a arm fix upthread and David Miller has given his nod to a sparc fix that uses FPE_FLTUNK. So it appears real architecture fixes are progressing. Further I have looked and that leaves only powerpc, parisc, ia64, and alpha. The new si_code FPE_FLTUNK appears to address most of those, and there is an untested patch for parisc. So real progress appears possible. The generic code can do better, and that is what this rfc patchset is about. It ensures siginfo is fully initialized and uses copy_to_user to copy siginfo to userspace. This takes siginfo_layout out of the picture and so for non-compat non-signalfd siginfos the status quo returns to what it was before I introduced siginfo_layout (AKA regressions go bye-bye). I believe given the issues these changes are a candiate for -rc2. Otherwise I will keep these changes for the next merge window. Eric W. Biederman (3): signal: Ensure every siginfo we send has all bits initialized signal: Reduce copy_siginfo_to_user to just copy_to_user signal: Stop special casing TRAP_FIXME and FPE_FIXME in siginfo_layout arch/alpha/kernel/osf_sys.c | 1 + arch/alpha/kernel/signal.c | 2 + arch/alpha/kernel/traps.c | 5 ++ arch/alpha/mm/fault.c | 2 + arch/arc/mm/fault.c | 2 + arch/arm/kernel/ptrace.c | 1 + arch/arm/kernel/swp_emulate.c | 1 + arch/arm/kernel/traps.c | 5 ++ arch/arm/mm/alignment.c | 1 + arch/arm/mm/fault.c | 5 ++ arch/arm/vfp/vfpmodule.c | 3 +- arch/arm64/kernel/fpsimd.c | 2 +- arch/arm64/kernel/sys_compat.c | 1 + arch/arm64/kernel/traps.c | 1 + arch/arm64/mm/fault.c | 18 ++++-- arch/c6x/kernel/traps.c | 1 + arch/hexagon/kernel/traps.c | 1 + arch/hexagon/mm/vm_fault.c | 1 + arch/ia64/kernel/brl_emu.c | 1 + arch/ia64/kernel/signal.c | 2 + arch/ia64/kernel/traps.c | 27 ++++++++- arch/ia64/kernel/unaligned.c | 1 + arch/ia64/mm/fault.c | 4 +- arch/m68k/kernel/traps.c | 2 + arch/microblaze/kernel/exceptions.c | 1 + arch/microblaze/mm/fault.c | 4 +- arch/mips/mm/fault.c | 1 + arch/nds32/kernel/traps.c | 6 +- arch/nds32/mm/fault.c | 1 + arch/nios2/kernel/traps.c | 1 + arch/openrisc/kernel/traps.c | 5 +- arch/openrisc/mm/fault.c | 1 + arch/parisc/kernel/ptrace.c | 1 + arch/parisc/kernel/traps.c | 2 + arch/parisc/kernel/unaligned.c | 1 + arch/parisc/math-emu/driver.c | 1 + arch/parisc/mm/fault.c | 1 + arch/powerpc/kernel/process.c | 1 + arch/powerpc/kernel/traps.c | 3 +- arch/powerpc/mm/fault.c | 1 + arch/powerpc/platforms/cell/spufs/fault.c | 2 +- arch/riscv/kernel/traps.c | 1 + arch/s390/kernel/traps.c | 5 +- arch/s390/mm/fault.c | 2 + arch/sh/kernel/hw_breakpoint.c | 1 + arch/sh/kernel/traps_32.c | 2 + arch/sh/math-emu/math.c | 1 + arch/sh/mm/fault.c | 1 + arch/sparc/kernel/process_64.c | 1 + arch/sparc/kernel/sys_sparc_32.c | 1 + arch/sparc/kernel/traps_32.c | 10 ++++ arch/sparc/kernel/traps_64.c | 14 +++++ arch/sparc/kernel/unaligned_32.c | 1 + arch/sparc/mm/fault_32.c | 1 + arch/sparc/mm/fault_64.c | 1 + arch/um/kernel/trap.c | 2 + arch/unicore32/kernel/fpu-ucf64.c | 2 +- arch/unicore32/mm/fault.c | 3 + arch/x86/entry/vsyscall/vsyscall_64.c | 2 +- arch/x86/kernel/ptrace.c | 2 +- arch/x86/kernel/traps.c | 3 + arch/x86/kernel/umip.c | 1 + arch/x86/kvm/mmu.c | 1 + arch/x86/mm/fault.c | 1 + arch/xtensa/kernel/traps.c | 1 + arch/xtensa/mm/fault.c | 1 + include/linux/ptrace.h | 1 - include/linux/tracehook.h | 1 + kernel/signal.c | 93 +------------------------------ virt/kvm/arm/mmu.c | 1 + 70 files changed, 165 insertions(+), 115 deletions(-) Eric ^ permalink raw reply [flat|nested] 38+ messages in thread
* [RFC PATCH 1/3] signal: Ensure every siginfo we send has all bits initialized 2018-04-15 15:56 ` [RFC PATCH 0/3] Dealing with the aliases of SI_USER Eric W. Biederman @ 2018-04-15 15:57 ` Eric W. Biederman 2018-04-17 13:23 ` Dave Martin 2018-04-15 15:58 ` [RFC PATCH 2/3] signal: Reduce copy_siginfo_to_user to just copy_to_user Eric W. Biederman ` (2 subsequent siblings) 3 siblings, 1 reply; 38+ messages in thread From: Eric W. Biederman @ 2018-04-15 15:57 UTC (permalink / raw) To: Linus Torvalds Cc: Dave Martin, Linux Kernel Mailing List, Dmitry V. Levin, sparclinux, ppc-dev, linux-arm-kernel, Russell King - ARM Linux, linux-arch Call clear_siginfo to ensure every stack allocated siginfo is properly initialized before being passed to the signal sending functions. Note: It is not safe to depend on C initializers to initialize struct siginfo on the stack because C is allowed to skip holes when initializing a structure. The initialization of struct siginfo in tracehook_report_syscall_exit was moved from the helper user_single_step_siginfo into tracehook_report_syscall_exit itself, to make it clear that the local variable siginfo gets fully initialized. In a few cases the scope of struct siginfo has been reduced to make it clear that siginfo siginfo is not used on other paths in the function in which it is declared. Instances of using memset to initialize siginfo have been replaced with calls clear_siginfo for clarity. Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- arch/alpha/kernel/osf_sys.c | 1 + arch/alpha/kernel/signal.c | 2 ++ arch/alpha/kernel/traps.c | 5 +++++ arch/alpha/mm/fault.c | 2 ++ arch/arc/mm/fault.c | 2 ++ arch/arm/kernel/ptrace.c | 1 + arch/arm/kernel/swp_emulate.c | 1 + arch/arm/kernel/traps.c | 5 +++++ arch/arm/mm/alignment.c | 1 + arch/arm/mm/fault.c | 5 +++++ arch/arm/vfp/vfpmodule.c | 3 +-- arch/arm64/kernel/fpsimd.c | 2 +- arch/arm64/kernel/sys_compat.c | 1 + arch/arm64/kernel/traps.c | 1 + arch/arm64/mm/fault.c | 18 ++++++++++++------ arch/c6x/kernel/traps.c | 1 + arch/hexagon/kernel/traps.c | 1 + arch/hexagon/mm/vm_fault.c | 1 + arch/ia64/kernel/brl_emu.c | 1 + arch/ia64/kernel/signal.c | 2 ++ arch/ia64/kernel/traps.c | 27 ++++++++++++++++++++++++--- arch/ia64/kernel/unaligned.c | 1 + arch/ia64/mm/fault.c | 4 +++- arch/m68k/kernel/traps.c | 2 ++ arch/microblaze/kernel/exceptions.c | 1 + arch/microblaze/mm/fault.c | 4 +++- arch/mips/mm/fault.c | 1 + arch/nds32/kernel/traps.c | 6 +++++- arch/nds32/mm/fault.c | 1 + arch/nios2/kernel/traps.c | 1 + arch/openrisc/kernel/traps.c | 5 ++++- arch/openrisc/mm/fault.c | 1 + arch/parisc/kernel/ptrace.c | 1 + arch/parisc/kernel/traps.c | 2 ++ arch/parisc/kernel/unaligned.c | 1 + arch/parisc/math-emu/driver.c | 1 + arch/parisc/mm/fault.c | 1 + arch/powerpc/kernel/process.c | 1 + arch/powerpc/kernel/traps.c | 3 +-- arch/powerpc/mm/fault.c | 1 + arch/powerpc/platforms/cell/spufs/fault.c | 2 +- arch/riscv/kernel/traps.c | 1 + arch/s390/kernel/traps.c | 5 ++++- arch/s390/mm/fault.c | 2 ++ arch/sh/kernel/hw_breakpoint.c | 1 + arch/sh/kernel/traps_32.c | 2 ++ arch/sh/math-emu/math.c | 1 + arch/sh/mm/fault.c | 1 + arch/sparc/kernel/process_64.c | 1 + arch/sparc/kernel/sys_sparc_32.c | 1 + arch/sparc/kernel/traps_32.c | 10 ++++++++++ arch/sparc/kernel/traps_64.c | 14 ++++++++++++++ arch/sparc/kernel/unaligned_32.c | 1 + arch/sparc/mm/fault_32.c | 1 + arch/sparc/mm/fault_64.c | 1 + arch/um/kernel/trap.c | 2 ++ arch/unicore32/kernel/fpu-ucf64.c | 2 +- arch/unicore32/mm/fault.c | 3 +++ arch/x86/entry/vsyscall/vsyscall_64.c | 2 +- arch/x86/kernel/ptrace.c | 2 +- arch/x86/kernel/traps.c | 3 +++ arch/x86/kernel/umip.c | 1 + arch/x86/kvm/mmu.c | 1 + arch/x86/mm/fault.c | 1 + arch/xtensa/kernel/traps.c | 1 + arch/xtensa/mm/fault.c | 1 + include/linux/ptrace.h | 1 - include/linux/tracehook.h | 1 + virt/kvm/arm/mmu.c | 1 + 69 files changed, 163 insertions(+), 24 deletions(-) diff --git a/arch/alpha/kernel/osf_sys.c b/arch/alpha/kernel/osf_sys.c index 89faa6f4de47..8ad689d6a0e4 100644 --- a/arch/alpha/kernel/osf_sys.c +++ b/arch/alpha/kernel/osf_sys.c @@ -881,6 +881,7 @@ SYSCALL_DEFINE5(osf_setsysinfo, unsigned long, op, void __user *, buffer, if (fex & IEEE_TRAP_ENABLE_DZE) si_code = FPE_FLTDIV; if (fex & IEEE_TRAP_ENABLE_INV) si_code = FPE_FLTINV; + clear_siginfo(&info); info.si_signo = SIGFPE; info.si_errno = 0; info.si_code = si_code; diff --git a/arch/alpha/kernel/signal.c b/arch/alpha/kernel/signal.c index 9ebb3bcbc626..cd306e602313 100644 --- a/arch/alpha/kernel/signal.c +++ b/arch/alpha/kernel/signal.c @@ -221,6 +221,7 @@ do_sigreturn(struct sigcontext __user *sc) if (ptrace_cancel_bpt (current)) { siginfo_t info; + clear_siginfo(&info); info.si_signo = SIGTRAP; info.si_errno = 0; info.si_code = TRAP_BRKPT; @@ -255,6 +256,7 @@ do_rt_sigreturn(struct rt_sigframe __user *frame) if (ptrace_cancel_bpt (current)) { siginfo_t info; + clear_siginfo(&info); info.si_signo = SIGTRAP; info.si_errno = 0; info.si_code = TRAP_BRKPT; diff --git a/arch/alpha/kernel/traps.c b/arch/alpha/kernel/traps.c index f43bd05dede2..91636765dd6d 100644 --- a/arch/alpha/kernel/traps.c +++ b/arch/alpha/kernel/traps.c @@ -228,6 +228,7 @@ do_entArith(unsigned long summary, unsigned long write_mask, } die_if_kernel("Arithmetic fault", regs, 0, NULL); + clear_siginfo(&info); info.si_signo = SIGFPE; info.si_errno = 0; info.si_code = si_code; @@ -241,6 +242,7 @@ do_entIF(unsigned long type, struct pt_regs *regs) siginfo_t info; int signo, code; + clear_siginfo(&info); if ((regs->ps & ~IPL_MAX) == 0) { if (type == 1) { const unsigned int *data @@ -430,6 +432,7 @@ do_entDbg(struct pt_regs *regs) die_if_kernel("Instruction fault", regs, 0, NULL); + clear_siginfo(&info); info.si_signo = SIGILL; info.si_errno = 0; info.si_code = ILL_ILLOPC; @@ -761,6 +764,8 @@ do_entUnaUser(void __user * va, unsigned long opcode, siginfo_t info; long error; + clear_siginfo(&info); + /* Check the UAC bits to decide what the user wants us to do with the unaliged access. */ diff --git a/arch/alpha/mm/fault.c b/arch/alpha/mm/fault.c index cd3c572ee912..7f2202a9f50a 100644 --- a/arch/alpha/mm/fault.c +++ b/arch/alpha/mm/fault.c @@ -91,6 +91,8 @@ do_page_fault(unsigned long address, unsigned long mmcsr, siginfo_t info; unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE; + clear_siginfo(&info); + /* As of EV6, a load into $31/$f31 is a prefetch, and never faults (or is suppressed by the PALcode). Support that for older CPUs by ignoring such an instruction. */ diff --git a/arch/arc/mm/fault.c b/arch/arc/mm/fault.c index a0b7bd6d030d..b884bbd6f354 100644 --- a/arch/arc/mm/fault.c +++ b/arch/arc/mm/fault.c @@ -70,6 +70,8 @@ void do_page_fault(unsigned long address, struct pt_regs *regs) int write = regs->ecr_cause & ECR_C_PROTV_STORE; /* ST/EX */ unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE; + clear_siginfo(&info); + /* * We fault-in kernel-space virtual memory on-demand. The * 'reference' page table is init_mm.pgd. diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c index 7724b0f661b3..36718a424358 100644 --- a/arch/arm/kernel/ptrace.c +++ b/arch/arm/kernel/ptrace.c @@ -205,6 +205,7 @@ void ptrace_break(struct task_struct *tsk, struct pt_regs *regs) { siginfo_t info; + clear_siginfo(&info); info.si_signo = SIGTRAP; info.si_errno = 0; info.si_code = TRAP_BRKPT; diff --git a/arch/arm/kernel/swp_emulate.c b/arch/arm/kernel/swp_emulate.c index 3bda08bee674..dfcb456afadd 100644 --- a/arch/arm/kernel/swp_emulate.c +++ b/arch/arm/kernel/swp_emulate.c @@ -112,6 +112,7 @@ static void set_segfault(struct pt_regs *regs, unsigned long addr) { siginfo_t info; + clear_siginfo(&info); down_read(¤t->mm->mmap_sem); if (find_vma(current->mm, addr) == NULL) info.si_code = SEGV_MAPERR; diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c index 5e3633c24e63..2584f9066da3 100644 --- a/arch/arm/kernel/traps.c +++ b/arch/arm/kernel/traps.c @@ -439,6 +439,7 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) siginfo_t info; void __user *pc; + clear_siginfo(&info); pc = (void __user *)instruction_pointer(regs); if (processor_mode(regs) == SVC_MODE) { @@ -537,6 +538,7 @@ static int bad_syscall(int n, struct pt_regs *regs) { siginfo_t info; + clear_siginfo(&info); if ((current->personality & PER_MASK) != PER_LINUX) { send_sig(SIGSEGV, current, 1); return regs->ARM_r0; @@ -604,6 +606,7 @@ asmlinkage int arm_syscall(int no, struct pt_regs *regs) { siginfo_t info; + clear_siginfo(&info); if ((no >> 16) != (__ARM_NR_BASE>> 16)) return bad_syscall(no, regs); @@ -740,6 +743,8 @@ baddataabort(int code, unsigned long instr, struct pt_regs *regs) unsigned long addr = instruction_pointer(regs); siginfo_t info; + clear_siginfo(&info); + #ifdef CONFIG_DEBUG_USER if (user_debug & UDBG_BADABORT) { pr_err("[%d] %s: bad data abort: code %d instr 0x%08lx\n", diff --git a/arch/arm/mm/alignment.c b/arch/arm/mm/alignment.c index 2c96190e018b..bd2c739d8083 100644 --- a/arch/arm/mm/alignment.c +++ b/arch/arm/mm/alignment.c @@ -950,6 +950,7 @@ do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs) if (ai_usermode & UM_SIGNAL) { siginfo_t si; + clear_siginfo(&si); si.si_signo = SIGBUS; si.si_errno = 0; si.si_code = BUS_ADRALN; diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c index b75eada23d0a..6e4e43dbdfa6 100644 --- a/arch/arm/mm/fault.c +++ b/arch/arm/mm/fault.c @@ -163,6 +163,8 @@ __do_user_fault(struct task_struct *tsk, unsigned long addr, { struct siginfo si; + clear_siginfo(&si); + #ifdef CONFIG_DEBUG_USER if (((user_debug & UDBG_SEGV) && (sig == SIGSEGV)) || ((user_debug & UDBG_BUS) && (sig == SIGBUS))) { @@ -550,6 +552,7 @@ do_DataAbort(unsigned long addr, unsigned int fsr, struct pt_regs *regs) const struct fsr_info *inf = fsr_info + fsr_fs(fsr); struct siginfo info; + if (!inf->fn(addr, fsr & ~FSR_LNX_PF, regs)) return; @@ -557,6 +560,7 @@ do_DataAbort(unsigned long addr, unsigned int fsr, struct pt_regs *regs) inf->name, fsr, addr); show_pte(current->mm, addr); + clear_siginfo(&info); info.si_signo = inf->sig; info.si_errno = 0; info.si_code = inf->code; @@ -589,6 +593,7 @@ do_PrefetchAbort(unsigned long addr, unsigned int ifsr, struct pt_regs *regs) pr_alert("Unhandled prefetch abort: %s (0x%03x) at 0x%08lx\n", inf->name, ifsr, addr); + clear_siginfo(&info); info.si_signo = inf->sig; info.si_errno = 0; info.si_code = inf->code; diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c index 4c375e11ae95..adda3fc2dde8 100644 --- a/arch/arm/vfp/vfpmodule.c +++ b/arch/arm/vfp/vfpmodule.c @@ -218,8 +218,7 @@ static void vfp_raise_sigfpe(unsigned int sicode, struct pt_regs *regs) { siginfo_t info; - memset(&info, 0, sizeof(info)); - + clear_siginfo(&info); info.si_signo = SIGFPE; info.si_code = sicode; info.si_addr = (void __user *)(instruction_pointer(regs) - 4); diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c index 87a35364e750..4bcdd0318729 100644 --- a/arch/arm64/kernel/fpsimd.c +++ b/arch/arm64/kernel/fpsimd.c @@ -882,7 +882,7 @@ asmlinkage void do_fpsimd_exc(unsigned int esr, struct pt_regs *regs) si_code = FPE_FLTRES; } - memset(&info, 0, sizeof(info)); + clear_siginfo(&info); info.si_signo = SIGFPE; info.si_code = si_code; info.si_addr = (void __user *)instruction_pointer(regs); diff --git a/arch/arm64/kernel/sys_compat.c b/arch/arm64/kernel/sys_compat.c index 93ab57dcfc14..a6109825eeb9 100644 --- a/arch/arm64/kernel/sys_compat.c +++ b/arch/arm64/kernel/sys_compat.c @@ -112,6 +112,7 @@ long compat_arm_syscall(struct pt_regs *regs) break; } + clear_siginfo(&info); info.si_signo = SIGILL; info.si_errno = 0; info.si_code = ILL_ILLTRP; diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c index ba964da31a25..7f476586cacc 100644 --- a/arch/arm64/kernel/traps.c +++ b/arch/arm64/kernel/traps.c @@ -634,6 +634,7 @@ asmlinkage void bad_el0_sync(struct pt_regs *regs, int reason, unsigned int esr) siginfo_t info; void __user *pc = (void __user *)instruction_pointer(regs); + clear_siginfo(&info); info.si_signo = SIGILL; info.si_errno = 0; info.si_code = ILL_ILLOPC; diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c index 4165485e8b6e..91c53a7d2575 100644 --- a/arch/arm64/mm/fault.c +++ b/arch/arm64/mm/fault.c @@ -305,11 +305,12 @@ static void do_bad_area(unsigned long addr, unsigned int esr, struct pt_regs *re */ if (user_mode(regs)) { const struct fault_info *inf = esr_to_fault_info(esr); - struct siginfo si = { - .si_signo = inf->sig, - .si_code = inf->code, - .si_addr = (void __user *)addr, - }; + struct siginfo si; + + clear_siginfo(&si); + si.si_signo = inf->sig; + si.si_code = inf->code; + si.si_addr = (void __user *)addr; __do_user_fault(&si, esr); } else { @@ -583,6 +584,7 @@ static int do_sea(unsigned long addr, unsigned int esr, struct pt_regs *regs) nmi_exit(); } + clear_siginfo(&info); info.si_signo = inf->sig; info.si_errno = 0; info.si_code = inf->code; @@ -687,6 +689,7 @@ asmlinkage void __exception do_mem_abort(unsigned long addr, unsigned int esr, show_pte(addr); } + clear_siginfo(&info); info.si_signo = inf->sig; info.si_errno = 0; info.si_code = inf->code; @@ -729,6 +732,7 @@ asmlinkage void __exception do_sp_pc_abort(unsigned long addr, local_irq_enable(); } + clear_siginfo(&info); info.si_signo = SIGBUS; info.si_errno = 0; info.si_code = BUS_ADRALN; @@ -772,7 +776,6 @@ asmlinkage int __exception do_debug_exception(unsigned long addr, struct pt_regs *regs) { const struct fault_info *inf = debug_fault_info + DBG_ESR_EVT(esr); - struct siginfo info; int rv; /* @@ -788,6 +791,9 @@ asmlinkage int __exception do_debug_exception(unsigned long addr, if (!inf->fn(addr, esr, regs)) { rv = 1; } else { + struct siginfo info; + + clear_siginfo(&info); info.si_signo = inf->sig; info.si_errno = 0; info.si_code = inf->code; diff --git a/arch/c6x/kernel/traps.c b/arch/c6x/kernel/traps.c index 4c1d4b84dd2b..c5feee4542b0 100644 --- a/arch/c6x/kernel/traps.c +++ b/arch/c6x/kernel/traps.c @@ -246,6 +246,7 @@ static void do_trap(struct exception_info *except_info, struct pt_regs *regs) unsigned long addr = instruction_pointer(regs); siginfo_t info; + clear_siginfo(&info); if (except_info->code != TRAP_BRKPT) pr_err("TRAP: %s PC[0x%lx] signo[%d] code[%d]\n", except_info->kernel_str, regs->pc, diff --git a/arch/hexagon/kernel/traps.c b/arch/hexagon/kernel/traps.c index 2942a9204a9a..1ff6a6a7b97c 100644 --- a/arch/hexagon/kernel/traps.c +++ b/arch/hexagon/kernel/traps.c @@ -414,6 +414,7 @@ void do_trap0(struct pt_regs *regs) if (user_mode(regs)) { struct siginfo info; + clear_siginfo(&info); info.si_signo = SIGTRAP; info.si_errno = 0; /* diff --git a/arch/hexagon/mm/vm_fault.c b/arch/hexagon/mm/vm_fault.c index 3eec33c5cfd7..2ad92edc877c 100644 --- a/arch/hexagon/mm/vm_fault.c +++ b/arch/hexagon/mm/vm_fault.c @@ -56,6 +56,7 @@ void do_page_fault(unsigned long address, long cause, struct pt_regs *regs) const struct exception_table_entry *fixup; unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE; + clear_siginfo(&info); /* * If we're in an interrupt or have no user context, * then must not take the fault. diff --git a/arch/ia64/kernel/brl_emu.c b/arch/ia64/kernel/brl_emu.c index 9bcc908bc85e..a61f6c6a36f8 100644 --- a/arch/ia64/kernel/brl_emu.c +++ b/arch/ia64/kernel/brl_emu.c @@ -62,6 +62,7 @@ ia64_emulate_brl (struct pt_regs *regs, unsigned long ar_ec) struct illegal_op_return rv; long tmp_taken, unimplemented_address; + clear_siginfo(&siginfo); rv.fkt = (unsigned long) -1; /* diff --git a/arch/ia64/kernel/signal.c b/arch/ia64/kernel/signal.c index 54547c7cf8a2..d1234a5ba4c5 100644 --- a/arch/ia64/kernel/signal.c +++ b/arch/ia64/kernel/signal.c @@ -153,6 +153,7 @@ ia64_rt_sigreturn (struct sigscratch *scr) return retval; give_sigsegv: + clear_siginfo(&si); si.si_signo = SIGSEGV; si.si_errno = 0; si.si_code = SI_KERNEL; @@ -236,6 +237,7 @@ force_sigsegv_info (int sig, void __user *addr) unsigned long flags; struct siginfo si; + clear_siginfo(&si); if (sig == SIGSEGV) { /* * Acquiring siglock around the sa_handler-update is almost diff --git a/arch/ia64/kernel/traps.c b/arch/ia64/kernel/traps.c index 6d4e76a4267f..972873ed1ae5 100644 --- a/arch/ia64/kernel/traps.c +++ b/arch/ia64/kernel/traps.c @@ -104,6 +104,7 @@ __kprobes ia64_bad_break (unsigned long break_num, struct pt_regs *regs) int sig, code; /* SIGILL, SIGFPE, SIGSEGV, and SIGBUS want these field initialized: */ + clear_siginfo(&siginfo); siginfo.si_addr = (void __user *) (regs->cr_iip + ia64_psr(regs)->ri); siginfo.si_imm = break_num; siginfo.si_flags = 0; /* clear __ISR_VALID */ @@ -293,7 +294,6 @@ handle_fpu_swa (int fp_fault, struct pt_regs *regs, unsigned long isr) { long exception, bundle[2]; unsigned long fault_ip; - struct siginfo siginfo; fault_ip = regs->cr_iip; if (!fp_fault && (ia64_psr(regs)->ri == 0)) @@ -344,10 +344,13 @@ handle_fpu_swa (int fp_fault, struct pt_regs *regs, unsigned long isr) printk(KERN_ERR "handle_fpu_swa: fp_emulate() returned -1\n"); return -1; } else { + struct siginfo siginfo; + /* is next instruction a trap? */ if (exception & 2) { ia64_increment_ip(regs); } + clear_siginfo(&siginfo); siginfo.si_signo = SIGFPE; siginfo.si_errno = 0; siginfo.si_code = FPE_FIXME; /* default code */ @@ -372,6 +375,9 @@ handle_fpu_swa (int fp_fault, struct pt_regs *regs, unsigned long isr) return -1; } else if (exception != 0) { /* raise exception */ + struct siginfo siginfo; + + clear_siginfo(&siginfo); siginfo.si_signo = SIGFPE; siginfo.si_errno = 0; siginfo.si_code = FPE_FIXME; /* default code */ @@ -420,7 +426,7 @@ ia64_illegal_op_fault (unsigned long ec, long arg1, long arg2, long arg3, if (die_if_kernel(buf, ®s, 0)) return rv; - memset(&si, 0, sizeof(si)); + clear_siginfo(&si); si.si_signo = SIGILL; si.si_code = ILL_ILLOPC; si.si_addr = (void __user *) (regs.cr_iip + ia64_psr(®s)->ri); @@ -434,7 +440,6 @@ ia64_fault (unsigned long vector, unsigned long isr, unsigned long ifa, long arg7, struct pt_regs regs) { unsigned long code, error = isr, iip; - struct siginfo siginfo; char buf[128]; int result, sig; static const char *reason[] = { @@ -485,6 +490,7 @@ ia64_fault (unsigned long vector, unsigned long isr, unsigned long ifa, case 26: /* NaT Consumption */ if (user_mode(®s)) { + struct siginfo siginfo; void __user *addr; if (((isr >> 4) & 0xf) == 2) { @@ -499,6 +505,7 @@ ia64_fault (unsigned long vector, unsigned long isr, unsigned long ifa, addr = (void __user *) (regs.cr_iip + ia64_psr(®s)->ri); } + clear_siginfo(&siginfo); siginfo.si_signo = sig; siginfo.si_code = code; siginfo.si_errno = 0; @@ -515,6 +522,9 @@ ia64_fault (unsigned long vector, unsigned long isr, unsigned long ifa, case 31: /* Unsupported Data Reference */ if (user_mode(®s)) { + struct siginfo siginfo; + + clear_siginfo(&siginfo); siginfo.si_signo = SIGILL; siginfo.si_code = ILL_ILLOPN; siginfo.si_errno = 0; @@ -531,6 +541,10 @@ ia64_fault (unsigned long vector, unsigned long isr, unsigned long ifa, case 29: /* Debug */ case 35: /* Taken Branch Trap */ case 36: /* Single Step Trap */ + { + struct siginfo siginfo; + + clear_siginfo(&siginfo); if (fsys_mode(current, ®s)) { extern char __kernel_syscall_via_break[]; /* @@ -578,11 +592,15 @@ ia64_fault (unsigned long vector, unsigned long isr, unsigned long ifa, siginfo.si_isr = isr; force_sig_info(SIGTRAP, &siginfo, current); return; + } case 32: /* fp fault */ case 33: /* fp trap */ result = handle_fpu_swa((vector == 32) ? 1 : 0, ®s, isr); if ((result < 0) || (current->thread.flags & IA64_THREAD_FPEMU_SIGFPE)) { + struct siginfo siginfo; + + clear_siginfo(&siginfo); siginfo.si_signo = SIGFPE; siginfo.si_errno = 0; siginfo.si_code = FPE_FLTINV; @@ -616,6 +634,9 @@ ia64_fault (unsigned long vector, unsigned long isr, unsigned long ifa, } else { /* Unimplemented Instr. Address Trap */ if (user_mode(®s)) { + struct siginfo siginfo; + + clear_siginfo(&siginfo); siginfo.si_signo = SIGILL; siginfo.si_code = ILL_BADIADDR; siginfo.si_errno = 0; diff --git a/arch/ia64/kernel/unaligned.c b/arch/ia64/kernel/unaligned.c index 72e9b4242564..e309f9859acc 100644 --- a/arch/ia64/kernel/unaligned.c +++ b/arch/ia64/kernel/unaligned.c @@ -1537,6 +1537,7 @@ ia64_handle_unaligned (unsigned long ifa, struct pt_regs *regs) /* NOT_REACHED */ } force_sigbus: + clear_siginfo(&si); si.si_signo = SIGBUS; si.si_errno = 0; si.si_code = BUS_ADRALN; diff --git a/arch/ia64/mm/fault.c b/arch/ia64/mm/fault.c index dfdc152d6737..817fa120645f 100644 --- a/arch/ia64/mm/fault.c +++ b/arch/ia64/mm/fault.c @@ -85,7 +85,6 @@ ia64_do_page_fault (unsigned long address, unsigned long isr, struct pt_regs *re int signal = SIGSEGV, code = SEGV_MAPERR; struct vm_area_struct *vma, *prev_vma; struct mm_struct *mm = current->mm; - struct siginfo si; unsigned long mask; int fault; unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE; @@ -249,6 +248,9 @@ ia64_do_page_fault (unsigned long address, unsigned long isr, struct pt_regs *re return; } if (user_mode(regs)) { + struct siginfo si; + + clear_siginfo(&si); si.si_signo = signal; si.si_errno = 0; si.si_code = code; diff --git a/arch/m68k/kernel/traps.c b/arch/m68k/kernel/traps.c index c1cc4e99aa94..0a00b476236d 100644 --- a/arch/m68k/kernel/traps.c +++ b/arch/m68k/kernel/traps.c @@ -1011,6 +1011,7 @@ asmlinkage void trap_c(struct frame *fp) int vector = (fp->ptregs.vector >> 2) & 0xff; siginfo_t info; + clear_siginfo(&info); if (fp->ptregs.sr & PS_S) { if (vector == VEC_TRACE) { /* traced a trapping instruction on a 68020/30, @@ -1163,6 +1164,7 @@ asmlinkage void fpemu_signal(int signal, int code, void *addr) { siginfo_t info; + clear_siginfo(&info); info.si_signo = signal; info.si_errno = 0; info.si_code = code; diff --git a/arch/microblaze/kernel/exceptions.c b/arch/microblaze/kernel/exceptions.c index e6f338d0496b..443ec1feacb4 100644 --- a/arch/microblaze/kernel/exceptions.c +++ b/arch/microblaze/kernel/exceptions.c @@ -65,6 +65,7 @@ void _exception(int signr, struct pt_regs *regs, int code, unsigned long addr) if (kernel_mode(regs)) die("Exception in kernel mode", regs, signr); + clear_siginfo(&info); info.si_signo = signr; info.si_errno = 0; info.si_code = code; diff --git a/arch/microblaze/mm/fault.c b/arch/microblaze/mm/fault.c index f91b30f8aaa8..43d92167012a 100644 --- a/arch/microblaze/mm/fault.c +++ b/arch/microblaze/mm/fault.c @@ -88,7 +88,6 @@ void do_page_fault(struct pt_regs *regs, unsigned long address, { struct vm_area_struct *vma; struct mm_struct *mm = current->mm; - siginfo_t info; int code = SEGV_MAPERR; int is_write = error_code & ESR_S; int fault; @@ -295,6 +294,9 @@ void do_page_fault(struct pt_regs *regs, unsigned long address, do_sigbus: up_read(&mm->mmap_sem); if (user_mode(regs)) { + siginfo_t info; + + clear_siginfo(&info); info.si_signo = SIGBUS; info.si_errno = 0; info.si_code = BUS_ADRERR; diff --git a/arch/mips/mm/fault.c b/arch/mips/mm/fault.c index 4f8f5bf46977..75392becd933 100644 --- a/arch/mips/mm/fault.c +++ b/arch/mips/mm/fault.c @@ -63,6 +63,7 @@ static void __kprobes __do_page_fault(struct pt_regs *regs, unsigned long write, return; #endif + clear_siginfo(&info); info.si_code = SEGV_MAPERR; /* diff --git a/arch/nds32/kernel/traps.c b/arch/nds32/kernel/traps.c index 6e34eb9824a4..35a93d10bc16 100644 --- a/arch/nds32/kernel/traps.c +++ b/arch/nds32/kernel/traps.c @@ -229,6 +229,7 @@ int bad_syscall(int n, struct pt_regs *regs) return regs->uregs[0]; } + clear_siginfo(&info); info.si_signo = SIGILL; info.si_errno = 0; info.si_code = ILL_ILLTRP; @@ -292,7 +293,7 @@ void send_sigtrap(struct task_struct *tsk, struct pt_regs *regs, tsk->thread.trap_no = ENTRY_DEBUG_RELATED; tsk->thread.error_code = error_code; - memset(&info, 0, sizeof(info)); + clear_siginfo(&info); info.si_signo = SIGTRAP; info.si_code = si_code; info.si_addr = (void __user *)instruction_pointer(regs); @@ -323,6 +324,7 @@ void unhandled_interruption(struct pt_regs *regs) show_regs(regs); if (!user_mode(regs)) do_exit(SIGKILL); + clear_siginfo(&si); si.si_signo = SIGKILL; si.si_errno = 0; force_sig_info(SIGKILL, &si, current); @@ -337,6 +339,7 @@ void unhandled_exceptions(unsigned long entry, unsigned long addr, show_regs(regs); if (!user_mode(regs)) do_exit(SIGKILL); + clear_siginfo(&si); si.si_signo = SIGKILL; si.si_errno = 0; si.si_addr = (void *)addr; @@ -368,6 +371,7 @@ void do_revinsn(struct pt_regs *regs) show_regs(regs); if (!user_mode(regs)) do_exit(SIGILL); + clear_siginfo(&si); si.si_signo = SIGILL; si.si_errno = 0; force_sig_info(SIGILL, &si, current); diff --git a/arch/nds32/mm/fault.c b/arch/nds32/mm/fault.c index 3a246fb8098c..876ee01ff80a 100644 --- a/arch/nds32/mm/fault.c +++ b/arch/nds32/mm/fault.c @@ -77,6 +77,7 @@ void do_page_fault(unsigned long entry, unsigned long addr, unsigned int mask = VM_READ | VM_WRITE | VM_EXEC; unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE; + clear_siginfo(&info); error_code = error_code & (ITYPE_mskINST | ITYPE_mskETYPE); tsk = current; mm = tsk->mm; diff --git a/arch/nios2/kernel/traps.c b/arch/nios2/kernel/traps.c index 8184e7d6b385..a69861d3e1a3 100644 --- a/arch/nios2/kernel/traps.c +++ b/arch/nios2/kernel/traps.c @@ -28,6 +28,7 @@ static void _send_sig(int signo, int code, unsigned long addr) { siginfo_t info; + clear_siginfo(&info); info.si_signo = signo; info.si_errno = 0; info.si_code = code; diff --git a/arch/openrisc/kernel/traps.c b/arch/openrisc/kernel/traps.c index 113c175fe469..1610b1d65a11 100644 --- a/arch/openrisc/kernel/traps.c +++ b/arch/openrisc/kernel/traps.c @@ -251,7 +251,7 @@ void __init trap_init(void) asmlinkage void do_trap(struct pt_regs *regs, unsigned long address) { siginfo_t info; - memset(&info, 0, sizeof(info)); + clear_siginfo(&info); info.si_signo = SIGTRAP; info.si_code = TRAP_TRACE; info.si_addr = (void *)address; @@ -266,6 +266,7 @@ asmlinkage void do_unaligned_access(struct pt_regs *regs, unsigned long address) if (user_mode(regs)) { /* Send a SIGBUS */ + clear_siginfo(&info); info.si_signo = SIGBUS; info.si_errno = 0; info.si_code = BUS_ADRALN; @@ -285,6 +286,7 @@ asmlinkage void do_bus_fault(struct pt_regs *regs, unsigned long address) if (user_mode(regs)) { /* Send a SIGBUS */ + clear_siginfo(&info); info.si_signo = SIGBUS; info.si_errno = 0; info.si_code = BUS_ADRERR; @@ -485,6 +487,7 @@ asmlinkage void do_illegal_instruction(struct pt_regs *regs, if (user_mode(regs)) { /* Send a SIGILL */ + clear_siginfo(&info); info.si_signo = SIGILL; info.si_errno = 0; info.si_code = ILL_ILLOPC; diff --git a/arch/openrisc/mm/fault.c b/arch/openrisc/mm/fault.c index d0021dfae20a..68be33e4ae17 100644 --- a/arch/openrisc/mm/fault.c +++ b/arch/openrisc/mm/fault.c @@ -56,6 +56,7 @@ asmlinkage void do_page_fault(struct pt_regs *regs, unsigned long address, int fault; unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE; + clear_siginfo(&info); tsk = current; /* diff --git a/arch/parisc/kernel/ptrace.c b/arch/parisc/kernel/ptrace.c index 1a2be6e639b5..b1c12ceb1c88 100644 --- a/arch/parisc/kernel/ptrace.c +++ b/arch/parisc/kernel/ptrace.c @@ -90,6 +90,7 @@ void user_enable_single_step(struct task_struct *task) ptrace_disable(task); /* Don't wake up the task, but let the parent know something happened. */ + clear_siginfo(&si); si.si_code = TRAP_TRACE; si.si_addr = (void __user *) (task_regs(task)->iaoq[0] & ~3); si.si_signo = SIGTRAP; diff --git a/arch/parisc/kernel/traps.c b/arch/parisc/kernel/traps.c index c919e6c0a687..cce2a63bd8f7 100644 --- a/arch/parisc/kernel/traps.c +++ b/arch/parisc/kernel/traps.c @@ -299,6 +299,7 @@ static void handle_gdb_break(struct pt_regs *regs, int wot) { struct siginfo si; + clear_siginfo(&si); si.si_signo = SIGTRAP; si.si_errno = 0; si.si_code = wot; @@ -489,6 +490,7 @@ void notrace handle_interruption(int code, struct pt_regs *regs) unsigned long fault_space = 0; struct siginfo si; + clear_siginfo(&si); if (code == 1) pdc_console_restart(); /* switch back to pdc if HPMC */ else diff --git a/arch/parisc/kernel/unaligned.c b/arch/parisc/kernel/unaligned.c index e36f7b75ab07..30b7c7f6c471 100644 --- a/arch/parisc/kernel/unaligned.c +++ b/arch/parisc/kernel/unaligned.c @@ -455,6 +455,7 @@ void handle_unaligned(struct pt_regs *regs) struct siginfo si; register int flop=0; /* true if this is a flop */ + clear_siginfo(&si); __inc_irq_stat(irq_unaligned_count); /* log a message with pacing */ diff --git a/arch/parisc/math-emu/driver.c b/arch/parisc/math-emu/driver.c index 2fb59d2e2b29..0d10efb53361 100644 --- a/arch/parisc/math-emu/driver.c +++ b/arch/parisc/math-emu/driver.c @@ -93,6 +93,7 @@ handle_fpe(struct pt_regs *regs) */ __u64 frcopy[36]; + clear_siginfo(&si); memcpy(frcopy, regs->fr, sizeof regs->fr); frcopy[32] = 0; diff --git a/arch/parisc/mm/fault.c b/arch/parisc/mm/fault.c index e247edbca68e..657b35096bd8 100644 --- a/arch/parisc/mm/fault.c +++ b/arch/parisc/mm/fault.c @@ -356,6 +356,7 @@ void do_page_fault(struct pt_regs *regs, unsigned long code, struct siginfo si; unsigned int lsb = 0; + clear_siginfo(&si); switch (code) { case 15: /* Data TLB miss fault/Data page fault */ /* send SIGSEGV when outside of vma */ diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c index 1237f13fed51..26ea9793d290 100644 --- a/arch/powerpc/kernel/process.c +++ b/arch/powerpc/kernel/process.c @@ -632,6 +632,7 @@ void do_break (struct pt_regs *regs, unsigned long address, hw_breakpoint_disable(); /* Deliver the signal to userspace */ + clear_siginfo(&info); info.si_signo = SIGTRAP; info.si_errno = 0; info.si_code = TRAP_HWBKPT; diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c index a2ef0c0e6c31..b8e61c552e05 100644 --- a/arch/powerpc/kernel/traps.c +++ b/arch/powerpc/kernel/traps.c @@ -296,7 +296,6 @@ NOKPROBE_SYMBOL(die); void user_single_step_siginfo(struct task_struct *tsk, struct pt_regs *regs, siginfo_t *info) { - memset(info, 0, sizeof(*info)); info->si_signo = SIGTRAP; info->si_code = TRAP_TRACE; info->si_addr = (void __user *)regs->nip; @@ -334,7 +333,7 @@ void _exception_pkey(int signr, struct pt_regs *regs, int code, */ thread_pkey_regs_save(¤t->thread); - memset(&info, 0, sizeof(info)); + clear_siginfo(&info); info.si_signo = signr; info.si_code = code; info.si_addr = (void __user *) addr; diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c index c01d627e687a..ef268d5d9db7 100644 --- a/arch/powerpc/mm/fault.c +++ b/arch/powerpc/mm/fault.c @@ -168,6 +168,7 @@ static int do_sigbus(struct pt_regs *regs, unsigned long address, return SIGBUS; current->thread.trap_nr = BUS_ADRERR; + clear_siginfo(&info); info.si_signo = SIGBUS; info.si_errno = 0; info.si_code = BUS_ADRERR; diff --git a/arch/powerpc/platforms/cell/spufs/fault.c b/arch/powerpc/platforms/cell/spufs/fault.c index 870c0a82d560..1e002e94d0f6 100644 --- a/arch/powerpc/platforms/cell/spufs/fault.c +++ b/arch/powerpc/platforms/cell/spufs/fault.c @@ -44,7 +44,7 @@ static void spufs_handle_event(struct spu_context *ctx, return; } - memset(&info, 0, sizeof(info)); + clear_siginfo(&info); switch (type) { case SPE_EVENT_INVALID_DMA: diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c index 93132cb59184..48aa6471cede 100644 --- a/arch/riscv/kernel/traps.c +++ b/arch/riscv/kernel/traps.c @@ -68,6 +68,7 @@ static inline void do_trap_siginfo(int signo, int code, { siginfo_t info; + clear_siginfo(&info); info.si_signo = signo; info.si_errno = 0; info.si_code = code; diff --git a/arch/s390/kernel/traps.c b/arch/s390/kernel/traps.c index a5297a22bc1e..3ba649d8aa5a 100644 --- a/arch/s390/kernel/traps.c +++ b/arch/s390/kernel/traps.c @@ -47,6 +47,7 @@ void do_report_trap(struct pt_regs *regs, int si_signo, int si_code, char *str) siginfo_t info; if (user_mode(regs)) { + clear_siginfo(&info); info.si_signo = si_signo; info.si_errno = 0; info.si_code = si_code; @@ -86,6 +87,7 @@ void do_per_trap(struct pt_regs *regs) return; if (!current->ptrace) return; + clear_siginfo(&info); info.si_signo = SIGTRAP; info.si_errno = 0; info.si_code = TRAP_HWBKPT; @@ -165,7 +167,6 @@ void translation_exception(struct pt_regs *regs) void illegal_op(struct pt_regs *regs) { - siginfo_t info; __u8 opcode[6]; __u16 __user *location; int is_uprobe_insn = 0; @@ -178,6 +179,8 @@ void illegal_op(struct pt_regs *regs) return; if (*((__u16 *) opcode) == S390_BREAKPOINT_U16) { if (current->ptrace) { + siginfo_t info; + clear_siginfo(&info); info.si_signo = SIGTRAP; info.si_errno = 0; info.si_code = TRAP_BRKPT; diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c index 93faeca52284..b3ff0e8e5860 100644 --- a/arch/s390/mm/fault.c +++ b/arch/s390/mm/fault.c @@ -268,6 +268,7 @@ static noinline void do_sigsegv(struct pt_regs *regs, int si_code) struct siginfo si; report_user_fault(regs, SIGSEGV, 1); + clear_siginfo(&si); si.si_signo = SIGSEGV; si.si_errno = 0; si.si_code = si_code; @@ -323,6 +324,7 @@ static noinline void do_sigbus(struct pt_regs *regs) * Send a sigbus, regardless of whether we were in kernel * or user mode. */ + clear_siginfo(&si); si.si_signo = SIGBUS; si.si_errno = 0; si.si_code = BUS_ADRERR; diff --git a/arch/sh/kernel/hw_breakpoint.c b/arch/sh/kernel/hw_breakpoint.c index afe965712a69..8ae91417da99 100644 --- a/arch/sh/kernel/hw_breakpoint.c +++ b/arch/sh/kernel/hw_breakpoint.c @@ -349,6 +349,7 @@ static int __kprobes hw_breakpoint_handler(struct die_args *args) if (!arch_check_bp_in_kernelspace(bp)) { siginfo_t info; + clear_siginfo(&info); info.si_signo = args->signr; info.si_errno = notifier_to_errno(rc); info.si_code = TRAP_HWBKPT; diff --git a/arch/sh/kernel/traps_32.c b/arch/sh/kernel/traps_32.c index b3770bb26211..e85e59c3d6df 100644 --- a/arch/sh/kernel/traps_32.c +++ b/arch/sh/kernel/traps_32.c @@ -537,6 +537,7 @@ asmlinkage void do_address_error(struct pt_regs *regs, "access (PC %lx PR %lx)\n", current->comm, regs->pc, regs->pr); + clear_siginfo(&info); info.si_signo = SIGBUS; info.si_errno = 0; info.si_code = si_code; @@ -600,6 +601,7 @@ asmlinkage void do_divide_error(unsigned long r4) { siginfo_t info; + clear_siginfo(&info); switch (r4) { case TRAP_DIVZERO_ERROR: info.si_code = FPE_INTDIV; diff --git a/arch/sh/math-emu/math.c b/arch/sh/math-emu/math.c index c86f4360c6ce..d6d2213df078 100644 --- a/arch/sh/math-emu/math.c +++ b/arch/sh/math-emu/math.c @@ -560,6 +560,7 @@ static int ieee_fpe_handler(struct pt_regs *regs) ~(FPSCR_CAUSE_MASK | FPSCR_FLAG_MASK); task_thread_info(tsk)->status |= TS_USEDFPU; } else { + clear_siginfo(&info); info.si_signo = SIGFPE; info.si_errno = 0; info.si_code = FPE_FLTINV; diff --git a/arch/sh/mm/fault.c b/arch/sh/mm/fault.c index 6fd1bf7481c7..4c98b6f20e02 100644 --- a/arch/sh/mm/fault.c +++ b/arch/sh/mm/fault.c @@ -44,6 +44,7 @@ force_sig_info_fault(int si_signo, int si_code, unsigned long address, { siginfo_t info; + clear_siginfo(&info); info.si_signo = si_signo; info.si_errno = 0; info.si_code = si_code; diff --git a/arch/sparc/kernel/process_64.c b/arch/sparc/kernel/process_64.c index 454a8af28f13..2219e55206b4 100644 --- a/arch/sparc/kernel/process_64.c +++ b/arch/sparc/kernel/process_64.c @@ -520,6 +520,7 @@ static void stack_unaligned(unsigned long sp) { siginfo_t info; + clear_siginfo(&info); info.si_signo = SIGBUS; info.si_errno = 0; info.si_code = BUS_ADRALN; diff --git a/arch/sparc/kernel/sys_sparc_32.c b/arch/sparc/kernel/sys_sparc_32.c index e8c3cb6b6d08..00f6353fe435 100644 --- a/arch/sparc/kernel/sys_sparc_32.c +++ b/arch/sparc/kernel/sys_sparc_32.c @@ -152,6 +152,7 @@ sparc_breakpoint (struct pt_regs *regs) #ifdef DEBUG_SPARC_BREAKPOINT printk ("TRAP: Entering kernel PC=%x, nPC=%x\n", regs->pc, regs->npc); #endif + clear_siginfo(&info); info.si_signo = SIGTRAP; info.si_errno = 0; info.si_code = TRAP_BRKPT; diff --git a/arch/sparc/kernel/traps_32.c b/arch/sparc/kernel/traps_32.c index b1ed763e4787..b5ef2c9cde48 100644 --- a/arch/sparc/kernel/traps_32.c +++ b/arch/sparc/kernel/traps_32.c @@ -104,6 +104,7 @@ void do_hw_interrupt(struct pt_regs *regs, unsigned long type) if(regs->psr & PSR_PS) die_if_kernel("Kernel bad trap", regs); + clear_siginfo(&info); info.si_signo = SIGILL; info.si_errno = 0; info.si_code = ILL_ILLTRP; @@ -124,6 +125,7 @@ void do_illegal_instruction(struct pt_regs *regs, unsigned long pc, unsigned lon regs->pc, *(unsigned long *)regs->pc); #endif + clear_siginfo(&info); info.si_signo = SIGILL; info.si_errno = 0; info.si_code = ILL_ILLOPC; @@ -139,6 +141,7 @@ void do_priv_instruction(struct pt_regs *regs, unsigned long pc, unsigned long n if(psr & PSR_PS) die_if_kernel("Penguin instruction from Penguin mode??!?!", regs); + clear_siginfo(&info); info.si_signo = SIGILL; info.si_errno = 0; info.si_code = ILL_PRVOPC; @@ -165,6 +168,7 @@ void do_memaccess_unaligned(struct pt_regs *regs, unsigned long pc, unsigned lon instruction_dump ((unsigned long *) regs->pc); printk ("do_MNA!\n"); #endif + clear_siginfo(&info); info.si_signo = SIGBUS; info.si_errno = 0; info.si_code = BUS_ADRALN; @@ -303,6 +307,7 @@ void do_fpe_trap(struct pt_regs *regs, unsigned long pc, unsigned long npc, } fsr = fpt->thread.fsr; + clear_siginfo(&info); info.si_signo = SIGFPE; info.si_errno = 0; info.si_addr = (void __user *)pc; @@ -336,6 +341,7 @@ void handle_tag_overflow(struct pt_regs *regs, unsigned long pc, unsigned long n if(psr & PSR_PS) die_if_kernel("Penguin overflow trap from kernel mode", regs); + clear_siginfo(&info); info.si_signo = SIGEMT; info.si_errno = 0; info.si_code = EMT_TAGOVF; @@ -365,6 +371,7 @@ void handle_reg_access(struct pt_regs *regs, unsigned long pc, unsigned long npc printk("Register Access Exception at PC %08lx NPC %08lx PSR %08lx\n", pc, npc, psr); #endif + clear_siginfo(&info); info.si_signo = SIGBUS; info.si_errno = 0; info.si_code = BUS_OBJERR; @@ -378,6 +385,7 @@ void handle_cp_disabled(struct pt_regs *regs, unsigned long pc, unsigned long np { siginfo_t info; + clear_siginfo(&info); info.si_signo = SIGILL; info.si_errno = 0; info.si_code = ILL_COPROC; @@ -395,6 +403,7 @@ void handle_cp_exception(struct pt_regs *regs, unsigned long pc, unsigned long n printk("Co-Processor Exception at PC %08lx NPC %08lx PSR %08lx\n", pc, npc, psr); #endif + clear_siginfo(&info); info.si_signo = SIGILL; info.si_errno = 0; info.si_code = ILL_COPROC; @@ -408,6 +417,7 @@ void handle_hw_divzero(struct pt_regs *regs, unsigned long pc, unsigned long npc { siginfo_t info; + clear_siginfo(&info); info.si_signo = SIGFPE; info.si_errno = 0; info.si_code = FPE_INTDIV; diff --git a/arch/sparc/kernel/traps_64.c b/arch/sparc/kernel/traps_64.c index 462a21abd105..1fecb3f61df5 100644 --- a/arch/sparc/kernel/traps_64.c +++ b/arch/sparc/kernel/traps_64.c @@ -107,6 +107,7 @@ void bad_trap(struct pt_regs *regs, long lvl) regs->tpc &= 0xffffffff; regs->tnpc &= 0xffffffff; } + clear_siginfo(&info); info.si_signo = SIGILL; info.si_errno = 0; info.si_code = ILL_ILLTRP; @@ -206,6 +207,7 @@ void spitfire_insn_access_exception(struct pt_regs *regs, unsigned long sfsr, un regs->tpc &= 0xffffffff; regs->tnpc &= 0xffffffff; } + clear_siginfo(&info); info.si_signo = SIGSEGV; info.si_errno = 0; info.si_code = SEGV_MAPERR; @@ -247,6 +249,7 @@ void sun4v_insn_access_exception(struct pt_regs *regs, unsigned long addr, unsig regs->tpc &= 0xffffffff; regs->tnpc &= 0xffffffff; } + clear_siginfo(&info); info.si_signo = SIGSEGV; info.si_errno = 0; info.si_code = SEGV_MAPERR; @@ -338,6 +341,7 @@ void spitfire_data_access_exception(struct pt_regs *regs, unsigned long sfsr, un if (is_no_fault_exception(regs)) return; + clear_siginfo(&info); info.si_signo = SIGSEGV; info.si_errno = 0; info.si_code = SEGV_MAPERR; @@ -595,6 +599,7 @@ static void spitfire_ue_log(unsigned long afsr, unsigned long afar, unsigned lon regs->tpc &= 0xffffffff; regs->tnpc &= 0xffffffff; } + clear_siginfo(&info); info.si_signo = SIGBUS; info.si_errno = 0; info.si_code = BUS_OBJERR; @@ -2211,6 +2216,7 @@ bool sun4v_nonresum_error_user_handled(struct pt_regs *regs, addr += PAGE_SIZE; } } + clear_siginfo(&info); info.si_signo = SIGKILL; info.si_errno = 0; info.si_trapno = 0; @@ -2221,6 +2227,7 @@ bool sun4v_nonresum_error_user_handled(struct pt_regs *regs, if (attrs & SUN4V_ERR_ATTRS_PIO) { siginfo_t info; + clear_siginfo(&info); info.si_signo = SIGBUS; info.si_code = BUS_ADRERR; info.si_addr = (void __user *)sun4v_get_vaddr(regs); @@ -2368,6 +2375,7 @@ static void do_fpe_common(struct pt_regs *regs) regs->tpc &= 0xffffffff; regs->tnpc &= 0xffffffff; } + clear_siginfo(&info); info.si_signo = SIGFPE; info.si_errno = 0; info.si_addr = (void __user *)regs->tpc; @@ -2440,6 +2448,7 @@ void do_tof(struct pt_regs *regs) regs->tpc &= 0xffffffff; regs->tnpc &= 0xffffffff; } + clear_siginfo(&info); info.si_signo = SIGEMT; info.si_errno = 0; info.si_code = EMT_TAGOVF; @@ -2465,6 +2474,7 @@ void do_div0(struct pt_regs *regs) regs->tpc &= 0xffffffff; regs->tnpc &= 0xffffffff; } + clear_siginfo(&info); info.si_signo = SIGFPE; info.si_errno = 0; info.si_code = FPE_INTDIV; @@ -2666,6 +2676,7 @@ void do_illegal_instruction(struct pt_regs *regs) } } } + clear_siginfo(&info); info.si_signo = SIGILL; info.si_errno = 0; info.si_code = ILL_ILLOPC; @@ -2692,6 +2703,7 @@ void mem_address_unaligned(struct pt_regs *regs, unsigned long sfar, unsigned lo if (is_no_fault_exception(regs)) return; + clear_siginfo(&info); info.si_signo = SIGBUS; info.si_errno = 0; info.si_code = BUS_ADRALN; @@ -2717,6 +2729,7 @@ void sun4v_do_mna(struct pt_regs *regs, unsigned long addr, unsigned long type_c if (is_no_fault_exception(regs)) return; + clear_siginfo(&info); info.si_signo = SIGBUS; info.si_errno = 0; info.si_code = BUS_ADRALN; @@ -2785,6 +2798,7 @@ void do_privop(struct pt_regs *regs) regs->tpc &= 0xffffffff; regs->tnpc &= 0xffffffff; } + clear_siginfo(&info); info.si_signo = SIGILL; info.si_errno = 0; info.si_code = ILL_PRVOPC; diff --git a/arch/sparc/kernel/unaligned_32.c b/arch/sparc/kernel/unaligned_32.c index 7642d7e4f0d9..0e4cf7217413 100644 --- a/arch/sparc/kernel/unaligned_32.c +++ b/arch/sparc/kernel/unaligned_32.c @@ -313,6 +313,7 @@ static void user_mna_trap_fault(struct pt_regs *regs, unsigned int insn) { siginfo_t info; + clear_siginfo(&info); info.si_signo = SIGBUS; info.si_errno = 0; info.si_code = BUS_ADRALN; diff --git a/arch/sparc/mm/fault_32.c b/arch/sparc/mm/fault_32.c index a8103a84b4ac..2deb586665b9 100644 --- a/arch/sparc/mm/fault_32.c +++ b/arch/sparc/mm/fault_32.c @@ -129,6 +129,7 @@ static void __do_fault_siginfo(int code, int sig, struct pt_regs *regs, { siginfo_t info; + clear_siginfo(&info); info.si_signo = sig; info.si_code = code; info.si_errno = 0; diff --git a/arch/sparc/mm/fault_64.c b/arch/sparc/mm/fault_64.c index 41363f46797b..46ccff95d10e 100644 --- a/arch/sparc/mm/fault_64.c +++ b/arch/sparc/mm/fault_64.c @@ -172,6 +172,7 @@ static void do_fault_siginfo(int code, int sig, struct pt_regs *regs, unsigned long addr; siginfo_t info; + clear_siginfo(&info); info.si_code = code; info.si_signo = sig; info.si_errno = 0; diff --git a/arch/um/kernel/trap.c b/arch/um/kernel/trap.c index b2b02df9896e..d4d38520c4c6 100644 --- a/arch/um/kernel/trap.c +++ b/arch/um/kernel/trap.c @@ -164,6 +164,7 @@ static void bad_segv(struct faultinfo fi, unsigned long ip) { struct siginfo si; + clear_siginfo(&si); si.si_signo = SIGSEGV; si.si_code = SEGV_ACCERR; si.si_addr = (void __user *) FAULT_ADDRESS(fi); @@ -220,6 +221,7 @@ unsigned long segv(struct faultinfo fi, unsigned long ip, int is_user, int is_write = FAULT_WRITE(fi); unsigned long address = FAULT_ADDRESS(fi); + clear_siginfo(&si); if (!is_user && regs) current->thread.segv_regs = container_of(regs, struct pt_regs, regs); diff --git a/arch/unicore32/kernel/fpu-ucf64.c b/arch/unicore32/kernel/fpu-ucf64.c index 12c8c9527b8e..d785955e1c29 100644 --- a/arch/unicore32/kernel/fpu-ucf64.c +++ b/arch/unicore32/kernel/fpu-ucf64.c @@ -56,7 +56,7 @@ void ucf64_raise_sigfpe(unsigned int sicode, struct pt_regs *regs) { siginfo_t info; - memset(&info, 0, sizeof(info)); + clear_siginfo(&info); info.si_signo = SIGFPE; info.si_code = sicode; diff --git a/arch/unicore32/mm/fault.c b/arch/unicore32/mm/fault.c index bbefcc46a45e..381473412937 100644 --- a/arch/unicore32/mm/fault.c +++ b/arch/unicore32/mm/fault.c @@ -125,6 +125,7 @@ static void __do_user_fault(struct task_struct *tsk, unsigned long addr, tsk->thread.address = addr; tsk->thread.error_code = fsr; tsk->thread.trap_no = 14; + clear_siginfo(&si); si.si_signo = sig; si.si_errno = 0; si.si_code = code; @@ -472,6 +473,7 @@ asmlinkage void do_DataAbort(unsigned long addr, unsigned int fsr, printk(KERN_ALERT "Unhandled fault: %s (0x%03x) at 0x%08lx\n", inf->name, fsr, addr); + clear_siginfo(&info); info.si_signo = inf->sig; info.si_errno = 0; info.si_code = inf->code; @@ -491,6 +493,7 @@ asmlinkage void do_PrefetchAbort(unsigned long addr, printk(KERN_ALERT "Unhandled prefetch abort: %s (0x%03x) at 0x%08lx\n", inf->name, ifsr, addr); + clear_siginfo(&info); info.si_signo = inf->sig; info.si_errno = 0; info.si_code = inf->code; diff --git a/arch/x86/entry/vsyscall/vsyscall_64.c b/arch/x86/entry/vsyscall/vsyscall_64.c index 317be365bce3..9c2fdfed0529 100644 --- a/arch/x86/entry/vsyscall/vsyscall_64.c +++ b/arch/x86/entry/vsyscall/vsyscall_64.c @@ -107,7 +107,7 @@ static bool write_ok_or_segv(unsigned long ptr, size_t size) thread->cr2 = ptr; thread->trap_nr = X86_TRAP_PF; - memset(&info, 0, sizeof(info)); + clear_siginfo(&info); info.si_signo = SIGSEGV; info.si_errno = 0; info.si_code = SEGV_MAPERR; diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c index ed5c4cdf0a34..e2ee403865eb 100644 --- a/arch/x86/kernel/ptrace.c +++ b/arch/x86/kernel/ptrace.c @@ -1377,7 +1377,6 @@ static void fill_sigtrap_info(struct task_struct *tsk, tsk->thread.trap_nr = X86_TRAP_DB; tsk->thread.error_code = error_code; - memset(info, 0, sizeof(*info)); info->si_signo = SIGTRAP; info->si_code = si_code; info->si_addr = user_mode(regs) ? (void __user *)regs->ip : NULL; @@ -1395,6 +1394,7 @@ void send_sigtrap(struct task_struct *tsk, struct pt_regs *regs, { struct siginfo info; + clear_siginfo(&info); fill_sigtrap_info(tsk, regs, error_code, si_code, &info); /* Send us the fake SIGTRAP */ force_sig_info(SIGTRAP, &info, tsk); diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c index 03f3d7695dac..a535dd64de63 100644 --- a/arch/x86/kernel/traps.c +++ b/arch/x86/kernel/traps.c @@ -299,6 +299,7 @@ static void do_error_trap(struct pt_regs *regs, long error_code, char *str, if (notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) != NOTIFY_STOP) { cond_local_irq_enable(regs); + clear_siginfo(&info); do_trap(trapnr, signr, str, regs, error_code, fill_trap_info(regs, signr, trapnr, &info)); } @@ -854,6 +855,7 @@ static void math_error(struct pt_regs *regs, int error_code, int trapnr) task->thread.trap_nr = trapnr; task->thread.error_code = error_code; + clear_siginfo(&info); info.si_signo = SIGFPE; info.si_errno = 0; info.si_addr = (void __user *)uprobe_get_trap_addr(regs); @@ -929,6 +931,7 @@ dotraplinkage void do_iret_error(struct pt_regs *regs, long error_code) RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU"); local_irq_enable(); + clear_siginfo(&info); info.si_signo = SIGILL; info.si_errno = 0; info.si_code = ILL_BADSTK; diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c index f44ce0fb3583..ff20b35e98dd 100644 --- a/arch/x86/kernel/umip.c +++ b/arch/x86/kernel/umip.c @@ -278,6 +278,7 @@ static void force_sig_info_umip_fault(void __user *addr, struct pt_regs *regs) tsk->thread.error_code = X86_PF_USER | X86_PF_WRITE; tsk->thread.trap_nr = X86_TRAP_PF; + clear_siginfo(&info); info.si_signo = SIGSEGV; info.si_errno = 0; info.si_code = SEGV_MAPERR; diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c index 763bb3bade63..b501e7b86e71 100644 --- a/arch/x86/kvm/mmu.c +++ b/arch/x86/kvm/mmu.c @@ -3007,6 +3007,7 @@ static void kvm_send_hwpoison_signal(unsigned long address, struct task_struct * { siginfo_t info; + clear_siginfo(&info); info.si_signo = SIGBUS; info.si_errno = 0; info.si_code = BUS_MCEERR_AR; diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c index 73bd8c95ac71..2a5a2920203d 100644 --- a/arch/x86/mm/fault.c +++ b/arch/x86/mm/fault.c @@ -209,6 +209,7 @@ force_sig_info_fault(int si_signo, int si_code, unsigned long address, unsigned lsb = 0; siginfo_t info; + clear_siginfo(&info); info.si_signo = si_signo; info.si_errno = 0; info.si_code = si_code; diff --git a/arch/xtensa/kernel/traps.c b/arch/xtensa/kernel/traps.c index 32c5207f1226..51771929f341 100644 --- a/arch/xtensa/kernel/traps.c +++ b/arch/xtensa/kernel/traps.c @@ -334,6 +334,7 @@ do_unaligned_user (struct pt_regs *regs) "(pid = %d, pc = %#010lx)\n", regs->excvaddr, current->comm, task_pid_nr(current), regs->pc); + clear_siginfo(&info); info.si_signo = SIGBUS; info.si_errno = 0; info.si_code = BUS_ADRALN; diff --git a/arch/xtensa/mm/fault.c b/arch/xtensa/mm/fault.c index 8b9b6f44bb06..f9323a3e61ce 100644 --- a/arch/xtensa/mm/fault.c +++ b/arch/xtensa/mm/fault.c @@ -45,6 +45,7 @@ void do_page_fault(struct pt_regs *regs) int fault; unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE; + clear_siginfo(&info); info.si_code = SEGV_MAPERR; /* We fault-in kernel-space virtual memory on-demand. The diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h index 919b2a0b0307..037bf0ef1ae9 100644 --- a/include/linux/ptrace.h +++ b/include/linux/ptrace.h @@ -345,7 +345,6 @@ extern void user_single_step_siginfo(struct task_struct *tsk, static inline void user_single_step_siginfo(struct task_struct *tsk, struct pt_regs *regs, siginfo_t *info) { - memset(info, 0, sizeof(*info)); info->si_signo = SIGTRAP; } #endif diff --git a/include/linux/tracehook.h b/include/linux/tracehook.h index 26c152122a42..4a8841963c2e 100644 --- a/include/linux/tracehook.h +++ b/include/linux/tracehook.h @@ -124,6 +124,7 @@ static inline void tracehook_report_syscall_exit(struct pt_regs *regs, int step) { if (step) { siginfo_t info; + clear_siginfo(&info); user_single_step_siginfo(current, regs, &info); force_sig_info(SIGTRAP, &info, current); return; diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c index b960acdd0c05..4525936d9591 100644 --- a/virt/kvm/arm/mmu.c +++ b/virt/kvm/arm/mmu.c @@ -1289,6 +1289,7 @@ static void kvm_send_hwpoison_signal(unsigned long address, { siginfo_t info; + clear_siginfo(&info); info.si_signo = SIGBUS; info.si_errno = 0; info.si_code = BUS_MCEERR_AR; -- 2.14.1 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [RFC PATCH 1/3] signal: Ensure every siginfo we send has all bits initialized 2018-04-15 15:57 ` [RFC PATCH 1/3] signal: Ensure every siginfo we send has all bits initialized Eric W. Biederman @ 2018-04-17 13:23 ` Dave Martin 2018-04-17 19:37 ` Eric W. Biederman 0 siblings, 1 reply; 38+ messages in thread From: Dave Martin @ 2018-04-17 13:23 UTC (permalink / raw) To: Eric W. Biederman Cc: Linus Torvalds, linux-arch, Linux Kernel Mailing List, Dmitry V. Levin, sparclinux, Russell King - ARM Linux, ppc-dev, linux-arm-kernel On Sun, Apr 15, 2018 at 10:57:33AM -0500, Eric W. Biederman wrote: > > Call clear_siginfo to ensure every stack allocated siginfo is properly > initialized before being passed to the signal sending functions. > > Note: It is not safe to depend on C initializers to initialize struct > siginfo on the stack because C is allowed to skip holes when > initializing a structure. > > The initialization of struct siginfo in tracehook_report_syscall_exit > was moved from the helper user_single_step_siginfo into > tracehook_report_syscall_exit itself, to make it clear that the local > variable siginfo gets fully initialized. > > In a few cases the scope of struct siginfo has been reduced to make it > clear that siginfo siginfo is not used on other paths in the function > in which it is declared. > > Instances of using memset to initialize siginfo have been replaced > with calls clear_siginfo for clarity. > > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> [...] Hmmm memset()/clear_siginfo() may ensure that there are no uninitialised explicit fields except for those in inactive union members, but I'm not sure that this approach is guaranteed to sanitise the padding seen by userspace. Rationale below, though it's a bit theoretical... With this in mind, I tend agree with Linus that hiding memset() calls from the maintainer may be a bad idea unless they are also hidden from the compiler. If the compiler sees the memset() it may be able to optimise it in ways that wouldn't be possible for some other random external function call, including optimising all or part of the call out. As a result, the breakdown into individual put_user()s etc. in copy_siginfo_to_user() may still be valuable even if all paths have the memset(). (Rationale for an arch/arm example:) > diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c > index 4c375e11ae95..adda3fc2dde8 100644 > --- a/arch/arm/vfp/vfpmodule.c > +++ b/arch/arm/vfp/vfpmodule.c > @@ -218,8 +218,7 @@ static void vfp_raise_sigfpe(unsigned int sicode, struct pt_regs *regs) > { > siginfo_t info; > > - memset(&info, 0, sizeof(info)); > - > + clear_siginfo(&info); > info.si_signo = SIGFPE; /* by c11 (n1570) 6.2.6.1 para 6 [1], all padding bytes in info now take unspecified values */ > info.si_code = sicode; > info.si_addr = (void __user *)(instruction_pointer(regs) - 4); /* by c11 (n1570) 6.2.6.1 para 7 [2], all bytes of the union info._sifields other than than those corresponding to _sigfault take unspecified values */ So I don't see why the compiler needs to ensure that any of the affected bytes are zero: it could potentially skip a lot of the memset() as a result, in theory. I've not seen a compiler actually take advantage of that, but I'm now not sure what forbids it. If this can happen, I only see two watertight workarounds: 1) Ensure that there is no implicit padding in any UAPI structure, e.g. aeb1f39d814b: ("arm64/ptrace: Avoid uninitialised struct padding in fpr_set()"). This would include tail-padding of any union member that is smaller than the containing union. It would be significantly more effort to ensure this for siginfo though. 2) Poke all values directly into allocated or user memory directly via pointers to paddingless types; never assign to objects on the kernel stack if you care what ends up in the padding, e.g., what your copy_siginfo_to_user() does prior to this series. If I'm not barking up the wrong tree, memset() cannot generally be used to determine the value of padding bytes, but it may still be useful for forcing otherwise uninitialised members to sane initial values. This likely affects many more things than just siginfo. [...] Cheers ---Dave [1] n1570 6.2.6.1.6: When a value is stored in an object of structure or union type, including in a member object, the bytes of the object representation that correspond to any padding bytes take unspecified values [...] [2] n1570 6.2.6.1.7: When a value is stored in a member of an object of union type, the bytes of the object representation that do not correspond to that member but do correspond to other members take unspecified values. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [RFC PATCH 1/3] signal: Ensure every siginfo we send has all bits initialized 2018-04-17 13:23 ` Dave Martin @ 2018-04-17 19:37 ` Eric W. Biederman 2018-04-18 12:47 ` Dave Martin 0 siblings, 1 reply; 38+ messages in thread From: Eric W. Biederman @ 2018-04-17 19:37 UTC (permalink / raw) To: Dave Martin Cc: Linus Torvalds, linux-arch, Linux Kernel Mailing List, Dmitry V. Levin, sparclinux, Russell King - ARM Linux, ppc-dev, linux-arm-kernel Dave Martin <Dave.Martin@arm.com> writes: > Hmmm > > memset()/clear_siginfo() may ensure that there are no uninitialised > explicit fields except for those in inactive union members, but I'm not > sure that this approach is guaranteed to sanitise the padding seen by > userspace. > > Rationale below, though it's a bit theoretical... > > With this in mind, I tend agree with Linus that hiding memset() calls > from the maintainer may be a bad idea unless they are also hidden from > the compiler. If the compiler sees the memset() it may be able to > optimise it in ways that wouldn't be possible for some other random > external function call, including optimising all or part of the call > out. > > As a result, the breakdown into individual put_user()s etc. in > copy_siginfo_to_user() may still be valuable even if all paths have the > memset(). The breakdown into individual put_user()s is known to be problematically slow, and is actually wrong. Even exclusing the SI_USER duplication in a small number of cases the fields filled out in siginfo by architecture code are not the fields that copy_siginfo_to_user is copying. Which is much worse. The code looks safe but is not. My intention is to leave 0 instances of clear_siginfo in the architecture specific code. Ideally struct siginfo will be limited to kernel/signal.c but I am not certain I can quite get that far. The function do_coredump appears to have a legit need for siginfo. > (Rationale for an arch/arm example:) > >> diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c >> index 4c375e11ae95..adda3fc2dde8 100644 >> --- a/arch/arm/vfp/vfpmodule.c >> +++ b/arch/arm/vfp/vfpmodule.c >> @@ -218,8 +218,7 @@ static void vfp_raise_sigfpe(unsigned int sicode, struct pt_regs *regs) >> { >> siginfo_t info; >> >> - memset(&info, 0, sizeof(info)); >> - >> + clear_siginfo(&info); >> info.si_signo = SIGFPE; > > /* by c11 (n1570) 6.2.6.1 para 6 [1], all padding bytes in info now take > unspecified values */ > >> info.si_code = sicode; >> info.si_addr = (void __user *)(instruction_pointer(regs) - 4); > > /* by c11 (n1570) 6.2.6.1 para 7 [2], all bytes of the union info._sifields > other than than those corresponding to _sigfault take unspecified > values */ > > So I don't see why the compiler needs to ensure that any of the affected > bytes are zero: it could potentially skip a lot of the memset() as a > result, in theory. > > I've not seen a compiler actually take advantage of that, but I'm now > not sure what forbids it. I took a quick look at gcc-4.9 which I have handy. The passes -f-no-strict-aliasing which helps, and gcc actually documents that if you access things through the union it will not take advantage of c11. gcc-4.9 Documents it this way: > -fstrict-aliasing' > Allow the compiler to assume the strictest aliasing rules > applicable to the language being compiled. For C (and C++), this > activates optimizations based on the type of expressions. In > particular, an object of one type is assumed never to reside at the > same address as an object of a different type, unless the types are > almost the same. For example, an 'unsigned int' can alias an > 'int', but not a 'void*' or a 'double'. A character type may alias > any other type. > > Pay special attention to code like this: > union a_union { > int i; > double d; > }; > > int f() { > union a_union t; > t.d = 3.0; > return t.i; > } > The practice of reading from a different union member than the one > most recently written to (called "type-punning") is common. Even > with '-fstrict-aliasing', type-punning is allowed, provided the > memory is accessed through the union type. So, the code above > works as expected. > If this can happen, I only see two watertight workarounds: > > 1) Ensure that there is no implicit padding in any UAPI structure, e.g. > aeb1f39d814b: ("arm64/ptrace: Avoid uninitialised struct padding in > fpr_set()"). This would include tail-padding of any union member that > is smaller than the containing union. > > It would be significantly more effort to ensure this for siginfo though. > > 2) Poke all values directly into allocated or user memory directly > via pointers to paddingless types; never assign to objects on the kernel > stack if you care what ends up in the padding, e.g., what your > copy_siginfo_to_user() does prior to this series. > > > If I'm not barking up the wrong tree, memset() cannot generally be > used to determine the value of padding bytes, but it may still be > useful for forcing otherwise uninitialised members to sane initial > values. > > This likely affects many more things than just siginfo. Unless gcc has changed it's stance on type-punning through unions or it's semantics with -fno-strict_aliasing we should be good. Eric ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [RFC PATCH 1/3] signal: Ensure every siginfo we send has all bits initialized 2018-04-17 19:37 ` Eric W. Biederman @ 2018-04-18 12:47 ` Dave Martin 2018-04-18 14:22 ` Eric W. Biederman 0 siblings, 1 reply; 38+ messages in thread From: Dave Martin @ 2018-04-18 12:47 UTC (permalink / raw) To: Eric W. Biederman Cc: linux-arch, ppc-dev, Linux Kernel Mailing List, Russell King - ARM Linux, sparclinux, Dmitry V. Levin, Linus Torvalds, linux-arm-kernel On Tue, Apr 17, 2018 at 02:37:38PM -0500, Eric W. Biederman wrote: > Dave Martin <Dave.Martin@arm.com> writes: > > > Hmmm > > > > memset()/clear_siginfo() may ensure that there are no uninitialised > > explicit fields except for those in inactive union members, but I'm not > > sure that this approach is guaranteed to sanitise the padding seen by > > userspace. > > > > Rationale below, though it's a bit theoretical... > > > > With this in mind, I tend agree with Linus that hiding memset() calls > > from the maintainer may be a bad idea unless they are also hidden from > > the compiler. If the compiler sees the memset() it may be able to > > optimise it in ways that wouldn't be possible for some other random > > external function call, including optimising all or part of the call > > out. > > > > As a result, the breakdown into individual put_user()s etc. in > > copy_siginfo_to_user() may still be valuable even if all paths have the > > memset(). > > The breakdown into individual put_user()s is known to be problematically > slow, and is actually wrong. Slowness certainly looked like a potential problem. > Even exclusing the SI_USER duplication in a small number of cases the > fields filled out in siginfo by architecture code are not the fields > that copy_siginfo_to_user is copying. Which is much worse. The code > looks safe but is not. > > My intention is to leave 0 instances of clear_siginfo in the > architecture specific code. Ideally struct siginfo will be limited to > kernel/signal.c but I am not certain I can quite get that far. > The function do_coredump appears to have a legit need for siginfo. So, you mean we can't detect that the caller didn't initialise all the members, or initialised the wrong union member? What would be the alternative? Have a separate interface for each SIL_ type, with only kernel/signal.c translating that into the siginfo_t that userspace sees? Either way, I don't see how we force the caller to initilise the whole structure. > > (Rationale for an arch/arm example:) > > > >> diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c > >> index 4c375e11ae95..adda3fc2dde8 100644 > >> --- a/arch/arm/vfp/vfpmodule.c > >> +++ b/arch/arm/vfp/vfpmodule.c > >> @@ -218,8 +218,7 @@ static void vfp_raise_sigfpe(unsigned int sicode, struct pt_regs *regs) > >> { > >> siginfo_t info; > >> > >> - memset(&info, 0, sizeof(info)); > >> - > >> + clear_siginfo(&info); > >> info.si_signo = SIGFPE; > > > > /* by c11 (n1570) 6.2.6.1 para 6 [1], all padding bytes in info now take > > unspecified values */ > > > >> info.si_code = sicode; > >> info.si_addr = (void __user *)(instruction_pointer(regs) - 4); > > > > /* by c11 (n1570) 6.2.6.1 para 7 [2], all bytes of the union info._sifields > > other than than those corresponding to _sigfault take unspecified > > values */ > > > > So I don't see why the compiler needs to ensure that any of the affected > > bytes are zero: it could potentially skip a lot of the memset() as a > > result, in theory. > > > > I've not seen a compiler actually take advantage of that, but I'm now > > not sure what forbids it. > > I took a quick look at gcc-4.9 which I have handy. > > The passes -f-no-strict-aliasing which helps, and gcc actually > documents that if you access things through the union it will > not take advantage of c11. > > gcc-4.9 Documents it this way: > > > -fstrict-aliasing' > > Allow the compiler to assume the strictest aliasing rules > > applicable to the language being compiled. For C (and C++), this > > activates optimizations based on the type of expressions. In > > particular, an object of one type is assumed never to reside at the > > same address as an object of a different type, unless the types are > > almost the same. For example, an 'unsigned int' can alias an > > 'int', but not a 'void*' or a 'double'. A character type may alias > > any other type. > > > > Pay special attention to code like this: > > union a_union { > > int i; > > double d; > > }; > > > > int f() { > > union a_union t; > > t.d = 3.0; > > return t.i; > > } > > The practice of reading from a different union member than the one > > most recently written to (called "type-punning") is common. Even > > with '-fstrict-aliasing', type-punning is allowed, provided the > > memory is accessed through the union type. So, the code above > > works as expected. This makes the C standard look precise (I love the "works as expected"), and says nothing about the cumulative effect of assigning to multiple members of a union, or about the effects on padding bytes. I'm not convinced that all of this falls under strict-aliasing, but I'd have to do more digging to confirm it. > > If this can happen, I only see two watertight workarounds: > > > > 1) Ensure that there is no implicit padding in any UAPI structure, e.g. > > aeb1f39d814b: ("arm64/ptrace: Avoid uninitialised struct padding in > > fpr_set()"). This would include tail-padding of any union member that > > is smaller than the containing union. > > > > It would be significantly more effort to ensure this for siginfo though. > > > > 2) Poke all values directly into allocated or user memory directly > > via pointers to paddingless types; never assign to objects on the kernel > > stack if you care what ends up in the padding, e.g., what your > > copy_siginfo_to_user() does prior to this series. > > > > > > If I'm not barking up the wrong tree, memset() cannot generally be > > used to determine the value of padding bytes, but it may still be > > useful for forcing otherwise uninitialised members to sane initial > > values. > > > > This likely affects many more things than just siginfo. > > Unless gcc has changed it's stance on type-punning through unions > or it's semantics with -fno-strict_aliasing we should be good. In practice you're probably right. Today, gcc is pretty conservative in this area, and I haven't been able to convince clang to optimise away memset in this way either. My concern is that is this assumption turns out to be wrong it may be some time before anybody notices, because the leakage of kernel stack may be the only symptom. I'll try to nail down a compiler guy to see if we can get a promise on this at least with -fno-strict-aliasing. I wonder whether it's worth protecting ourselves with something like: static void clear_siginfo(siginfo_t *si) { asm ("" : "=m" (*si)); memset(si, 0, sizeof(*si)); asm ("" : "+m" (*si)); } Probably needs to be thought about more widely though. I guess it's out of scope for this series. Cheers ---Dave ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [RFC PATCH 1/3] signal: Ensure every siginfo we send has all bits initialized 2018-04-18 12:47 ` Dave Martin @ 2018-04-18 14:22 ` Eric W. Biederman 2018-04-19 8:26 ` Dave Martin 0 siblings, 1 reply; 38+ messages in thread From: Eric W. Biederman @ 2018-04-18 14:22 UTC (permalink / raw) To: Dave Martin Cc: linux-arch, ppc-dev, Linux Kernel Mailing List, Russell King - ARM Linux, sparclinux, Dmitry V. Levin, Linus Torvalds, linux-arm-kernel Dave Martin <Dave.Martin@arm.com> writes: > On Tue, Apr 17, 2018 at 02:37:38PM -0500, Eric W. Biederman wrote: >> Dave Martin <Dave.Martin@arm.com> writes: >> >> > Hmmm >> > >> > memset()/clear_siginfo() may ensure that there are no uninitialised >> > explicit fields except for those in inactive union members, but I'm not >> > sure that this approach is guaranteed to sanitise the padding seen by >> > userspace. >> > >> > Rationale below, though it's a bit theoretical... >> > >> > With this in mind, I tend agree with Linus that hiding memset() calls >> > from the maintainer may be a bad idea unless they are also hidden from >> > the compiler. If the compiler sees the memset() it may be able to >> > optimise it in ways that wouldn't be possible for some other random >> > external function call, including optimising all or part of the call >> > out. >> > >> > As a result, the breakdown into individual put_user()s etc. in >> > copy_siginfo_to_user() may still be valuable even if all paths have the >> > memset(). >> >> The breakdown into individual put_user()s is known to be problematically >> slow, and is actually wrong. > > Slowness certainly looked like a potential problem. > >> Even exclusing the SI_USER duplication in a small number of cases the >> fields filled out in siginfo by architecture code are not the fields >> that copy_siginfo_to_user is copying. Which is much worse. The code >> looks safe but is not. >> >> My intention is to leave 0 instances of clear_siginfo in the >> architecture specific code. Ideally struct siginfo will be limited to >> kernel/signal.c but I am not certain I can quite get that far. >> The function do_coredump appears to have a legit need for siginfo. > > So, you mean we can't detect that the caller didn't initialise all the > members, or initialised the wrong union member? Correct. Even when we smuggled the the union member in the upper bits of si_code we got it wrong. So an interface that helps out and does more and is harder to misues looks desirable. > What would be the alternative? Have a separate interface for each SIL_ > type, with only kernel/signal.c translating that into the siginfo_t that > userspace sees? Yes. It really isn't bad as architecture specific code only generates faults. In general faults only take a pointer. I have already merged the needed helpers into kernel/signal.c > Either way, I don't see how we force the caller to initilise the whole > structure. In general the plan is to convert the callers to call force_sig_fault, and then there is no need to have siginfo in the architecture specific code. I have all of the necessary helpers are already merged into kernel/signal.c > >> > (Rationale for an arch/arm example:) >> > >> >> diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c >> >> index 4c375e11ae95..adda3fc2dde8 100644 >> >> --- a/arch/arm/vfp/vfpmodule.c >> >> +++ b/arch/arm/vfp/vfpmodule.c >> >> @@ -218,8 +218,7 @@ static void vfp_raise_sigfpe(unsigned int sicode, struct pt_regs *regs) >> >> { >> >> siginfo_t info; >> >> >> >> - memset(&info, 0, sizeof(info)); >> >> - >> >> + clear_siginfo(&info); >> >> info.si_signo = SIGFPE; >> > >> > /* by c11 (n1570) 6.2.6.1 para 6 [1], all padding bytes in info now take >> > unspecified values */ >> > >> >> info.si_code = sicode; >> >> info.si_addr = (void __user *)(instruction_pointer(regs) - 4); >> > >> > /* by c11 (n1570) 6.2.6.1 para 7 [2], all bytes of the union info._sifields >> > other than than those corresponding to _sigfault take unspecified >> > values */ >> > >> > So I don't see why the compiler needs to ensure that any of the affected >> > bytes are zero: it could potentially skip a lot of the memset() as a >> > result, in theory. >> > >> > I've not seen a compiler actually take advantage of that, but I'm now >> > not sure what forbids it. >> >> I took a quick look at gcc-4.9 which I have handy. >> >> The passes -f-no-strict-aliasing which helps, and gcc actually >> documents that if you access things through the union it will >> not take advantage of c11. >> >> gcc-4.9 Documents it this way: >> >> > -fstrict-aliasing' >> > Allow the compiler to assume the strictest aliasing rules >> > applicable to the language being compiled. For C (and C++), this >> > activates optimizations based on the type of expressions. In >> > particular, an object of one type is assumed never to reside at the >> > same address as an object of a different type, unless the types are >> > almost the same. For example, an 'unsigned int' can alias an >> > 'int', but not a 'void*' or a 'double'. A character type may alias >> > any other type. >> > >> > Pay special attention to code like this: >> > union a_union { >> > int i; >> > double d; >> > }; >> > >> > int f() { >> > union a_union t; >> > t.d = 3.0; >> > return t.i; >> > } >> > The practice of reading from a different union member than the one >> > most recently written to (called "type-punning") is common. Even >> > with '-fstrict-aliasing', type-punning is allowed, provided the >> > memory is accessed through the union type. So, the code above >> > works as expected. > > This makes the C standard look precise (I love the "works as expected"), > and says nothing about the cumulative effect of assigning to multiple > members of a union, or about the effects on padding bytes. > > I'm not convinced that all of this falls under strict-aliasing, but > I'd have to do more digging to confirm it. >> > If this can happen, I only see two watertight workarounds: >> > >> > 1) Ensure that there is no implicit padding in any UAPI structure, e.g. >> > aeb1f39d814b: ("arm64/ptrace: Avoid uninitialised struct padding in >> > fpr_set()"). This would include tail-padding of any union member that >> > is smaller than the containing union. >> > >> > It would be significantly more effort to ensure this for siginfo though. >> > >> > 2) Poke all values directly into allocated or user memory directly >> > via pointers to paddingless types; never assign to objects on the kernel >> > stack if you care what ends up in the padding, e.g., what your >> > copy_siginfo_to_user() does prior to this series. >> > >> > >> > If I'm not barking up the wrong tree, memset() cannot generally be >> > used to determine the value of padding bytes, but it may still be >> > useful for forcing otherwise uninitialised members to sane initial >> > values. >> > >> > This likely affects many more things than just siginfo. >> >> Unless gcc has changed it's stance on type-punning through unions >> or it's semantics with -fno-strict_aliasing we should be good. > > In practice you're probably right. > > Today, gcc is pretty conservative in this area, and I haven't been able > to convince clang to optimise away memset in this way either. > > My concern is that is this assumption turns out to be wrong it may be > some time before anybody notices, because the leakage of kernel stack may > be the only symptom. > > I'll try to nail down a compiler guy to see if we can get a promise on > this at least with -fno-strict-aliasing. > > > I wonder whether it's worth protecting ourselves with something like: > > > static void clear_siginfo(siginfo_t *si) > { > asm ("" : "=m" (*si)); > memset(si, 0, sizeof(*si)); > asm ("" : "+m" (*si)); > } > > Probably needs to be thought about more widely though. I guess it's out > of scope for this series. It is definitely a question worth asking. Eric ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [RFC PATCH 1/3] signal: Ensure every siginfo we send has all bits initialized 2018-04-18 14:22 ` Eric W. Biederman @ 2018-04-19 8:26 ` Dave Martin 0 siblings, 0 replies; 38+ messages in thread From: Dave Martin @ 2018-04-19 8:26 UTC (permalink / raw) To: Eric W. Biederman Cc: linux-arch, Linus Torvalds, Linux Kernel Mailing List, Dmitry V. Levin, sparclinux, Russell King - ARM Linux, ppc-dev, linux-arm-kernel On Wed, Apr 18, 2018 at 09:22:09AM -0500, Eric W. Biederman wrote: > Dave Martin <Dave.Martin@arm.com> writes: > > > On Tue, Apr 17, 2018 at 02:37:38PM -0500, Eric W. Biederman wrote: [...] > >> My intention is to leave 0 instances of clear_siginfo in the > >> architecture specific code. Ideally struct siginfo will be limited to > >> kernel/signal.c but I am not certain I can quite get that far. > >> The function do_coredump appears to have a legit need for siginfo. > > > > So, you mean we can't detect that the caller didn't initialise all the > > members, or initialised the wrong union member? > > Correct. Even when we smuggled the the union member in the upper bits > of si_code we got it wrong. So an interface that helps out and does > more and is harder to misues looks desirable. > > > What would be the alternative? Have a separate interface for each SIL_ > > type, with only kernel/signal.c translating that into the siginfo_t that > > userspace sees? > > Yes. It really isn't bad as architecture specific code only generates > faults. In general faults only take a pointer. I have already merged > the needed helpers into kernel/signal.c Good point. I hadn't considered that only one class of signal comes from the arch code, but now that you point it out, it sounds right. > > Either way, I don't see how we force the caller to initilise the whole > > structure. > > In general the plan is to convert the callers to call force_sig_fault, > and then there is no need to have siginfo in the architecture specific > code. I have all of the necessary helpers are already merged into > kernel/signal.c Makes sense. I wonder if all the relevant siginfo data could be passed to force_sig_fault() (or whatever) as arguments. Then the problem of uninitialised fields goes away. Perhaps that's what you had in mind. [...] > >> Unless gcc has changed it's stance on type-punning through unions > >> or it's semantics with -fno-strict_aliasing we should be good. > > > > In practice you're probably right. > > > > Today, gcc is pretty conservative in this area, and I haven't been able > > to convince clang to optimise away memset in this way either. > > > > My concern is that is this assumption turns out to be wrong it may be > > some time before anybody notices, because the leakage of kernel stack may > > be the only symptom. > > > > I'll try to nail down a compiler guy to see if we can get a promise on > > this at least with -fno-strict-aliasing. > > > > > > I wonder whether it's worth protecting ourselves with something like: > > > > > > static void clear_siginfo(siginfo_t *si) > > { > > asm ("" : "=m" (*si)); > > memset(si, 0, sizeof(*si)); > > asm ("" : "+m" (*si)); > > } > > > > Probably needs to be thought about more widely though. I guess it's out > > of scope for this series. > > It is definitely a question worth asking. I may follow it up later if I find myself at a loose end... Cheers ---Dave ^ permalink raw reply [flat|nested] 38+ messages in thread
* [RFC PATCH 2/3] signal: Reduce copy_siginfo_to_user to just copy_to_user 2018-04-15 15:56 ` [RFC PATCH 0/3] Dealing with the aliases of SI_USER Eric W. Biederman 2018-04-15 15:57 ` [RFC PATCH 1/3] signal: Ensure every siginfo we send has all bits initialized Eric W. Biederman @ 2018-04-15 15:58 ` Eric W. Biederman 2018-04-15 15:59 ` [RFC PATCH 3/3] signal: Stop special casing TRAP_FIXME and FPE_FIXME in siginfo_layout Eric W. Biederman 2018-04-15 18:16 ` [RFC PATCH 0/3] Dealing with the aliases of SI_USER Linus Torvalds 3 siblings, 0 replies; 38+ messages in thread From: Eric W. Biederman @ 2018-04-15 15:58 UTC (permalink / raw) To: Linus Torvalds Cc: Dave Martin, Linux Kernel Mailing List, Dmitry V. Levin, sparclinux, ppc-dev, linux-arm-kernel, Russell King - ARM Linux, linux-arch Now that every instance of struct siginfo is now initialized it is no longer necessary to copy struct siginfo piece by piece to userspace but instead the entire structure can be copied. As well as making the code simpler and more efficient this means that copy_sinfo_to_user no longer cares which union member of struct siginfo is in use. In practice this means that all 32bit architectures that define FPE_FIXME will handle properly send SI_USER when kill(SIGFPE) is sent. While still performing their historic architectural brokenness when 0 is used a floating pointer signal. This matches the current behavior of 64bit architectures that define FPE_FIXME who get lucky and an overloaded SI_USER has continuted to work through copy_siginfo_to_user because the 8 byte si_addr occupies the same bytes in struct siginfo as the 4 byte si_pid and the 4 byte si_uid. Problematic architectures still need to fix their ABI so that signalfd and 32bit compat code will work properly. Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- kernel/signal.c | 84 ++------------------------------------------------------- 1 file changed, 2 insertions(+), 82 deletions(-) diff --git a/kernel/signal.c b/kernel/signal.c index d4ccea599692..d56f4d496c89 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -2850,89 +2850,9 @@ enum siginfo_layout siginfo_layout(int sig, int si_code) int copy_siginfo_to_user(siginfo_t __user *to, const siginfo_t *from) { - int err; - - if (!access_ok (VERIFY_WRITE, to, sizeof(siginfo_t))) + if (copy_to_user(to, from , sizeof(struct siginfo))) return -EFAULT; - if (from->si_code < 0) - return __copy_to_user(to, from, sizeof(siginfo_t)) - ? -EFAULT : 0; - /* - * If you change siginfo_t structure, please be sure - * this code is fixed accordingly. - * Please remember to update the signalfd_copyinfo() function - * inside fs/signalfd.c too, in case siginfo_t changes. - * It should never copy any pad contained in the structure - * to avoid security leaks, but must copy the generic - * 3 ints plus the relevant union member. - */ - 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); - switch (siginfo_layout(from->si_signo, from->si_code)) { - case SIL_KILL: - err |= __put_user(from->si_pid, &to->si_pid); - err |= __put_user(from->si_uid, &to->si_uid); - break; - case SIL_TIMER: - /* Unreached SI_TIMER is negative */ - break; - case SIL_POLL: - err |= __put_user(from->si_band, &to->si_band); - err |= __put_user(from->si_fd, &to->si_fd); - break; - case SIL_FAULT: - err |= __put_user(from->si_addr, &to->si_addr); -#ifdef __ARCH_SI_TRAPNO - err |= __put_user(from->si_trapno, &to->si_trapno); -#endif -#ifdef __ia64__ - err |= __put_user(from->si_imm, &to->si_imm); - err |= __put_user(from->si_flags, &to->si_flags); - err |= __put_user(from->si_isr, &to->si_isr); -#endif - /* - * Other callers might not initialize the si_lsb field, - * so check explicitly for the right codes here. - */ -#ifdef BUS_MCEERR_AR - if (from->si_signo == SIGBUS && from->si_code == BUS_MCEERR_AR) - err |= __put_user(from->si_addr_lsb, &to->si_addr_lsb); -#endif -#ifdef BUS_MCEERR_AO - if (from->si_signo == SIGBUS && from->si_code == BUS_MCEERR_AO) - err |= __put_user(from->si_addr_lsb, &to->si_addr_lsb); -#endif -#ifdef SEGV_BNDERR - if (from->si_signo == SIGSEGV && from->si_code == SEGV_BNDERR) { - err |= __put_user(from->si_lower, &to->si_lower); - err |= __put_user(from->si_upper, &to->si_upper); - } -#endif -#ifdef SEGV_PKUERR - if (from->si_signo == SIGSEGV && from->si_code == SEGV_PKUERR) - err |= __put_user(from->si_pkey, &to->si_pkey); -#endif - break; - case SIL_CHLD: - err |= __put_user(from->si_pid, &to->si_pid); - err |= __put_user(from->si_uid, &to->si_uid); - err |= __put_user(from->si_status, &to->si_status); - err |= __put_user(from->si_utime, &to->si_utime); - err |= __put_user(from->si_stime, &to->si_stime); - break; - case SIL_RT: - err |= __put_user(from->si_pid, &to->si_pid); - err |= __put_user(from->si_uid, &to->si_uid); - err |= __put_user(from->si_ptr, &to->si_ptr); - break; - case SIL_SYS: - err |= __put_user(from->si_call_addr, &to->si_call_addr); - err |= __put_user(from->si_syscall, &to->si_syscall); - err |= __put_user(from->si_arch, &to->si_arch); - break; - } - return err; + return 0; } #ifdef CONFIG_COMPAT -- 2.14.1 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [RFC PATCH 3/3] signal: Stop special casing TRAP_FIXME and FPE_FIXME in siginfo_layout 2018-04-15 15:56 ` [RFC PATCH 0/3] Dealing with the aliases of SI_USER Eric W. Biederman 2018-04-15 15:57 ` [RFC PATCH 1/3] signal: Ensure every siginfo we send has all bits initialized Eric W. Biederman 2018-04-15 15:58 ` [RFC PATCH 2/3] signal: Reduce copy_siginfo_to_user to just copy_to_user Eric W. Biederman @ 2018-04-15 15:59 ` Eric W. Biederman 2018-04-15 18:16 ` [RFC PATCH 0/3] Dealing with the aliases of SI_USER Linus Torvalds 3 siblings, 0 replies; 38+ messages in thread From: Eric W. Biederman @ 2018-04-15 15:59 UTC (permalink / raw) To: Linus Torvalds Cc: Dave Martin, Linux Kernel Mailing List, Dmitry V. Levin, sparclinux, ppc-dev, linux-arm-kernel, Russell King - ARM Linux, linux-arch After more experience with the cases where no one the si_code of 0 is used both as a signal specific si_code, and as SI_USER it appears that no one cares about the signal specific si_code case and the good solution is to just fix the architectures by using a different si_code. In none of the conversations has anyone even suggested that anything depends on the signal specific redefinition of SI_USER. There are at least test cases that care when si_code as 0 does not work as si_user. So make things simple and keep the generic code from introducing problems by removing the special casing of TRAP_FIXME and FPE_FIXME. This will ensure the generic case of sending a signal with kill will always set SI_USER and work. The architecture specific, and signal specific overloads that set si_code to 0 will now have problems with signalfd and the 32bit compat versions of siginfo copying. At least until they are fixed. Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- kernel/signal.c | 9 --------- 1 file changed, 9 deletions(-) diff --git a/kernel/signal.c b/kernel/signal.c index d56f4d496c89..fc82d2c0918f 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -2835,15 +2835,6 @@ enum siginfo_layout siginfo_layout(int sig, int si_code) layout = SIL_POLL; else if (si_code < 0) layout = SIL_RT; - /* Tests to support buggy kernel ABIs */ -#ifdef TRAP_FIXME - if ((sig == SIGTRAP) && (si_code == TRAP_FIXME)) - layout = SIL_FAULT; -#endif -#ifdef FPE_FIXME - if ((sig == SIGFPE) && (si_code == FPE_FIXME)) - layout = SIL_FAULT; -#endif } return layout; } -- 2.14.1 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [RFC PATCH 0/3] Dealing with the aliases of SI_USER 2018-04-15 15:56 ` [RFC PATCH 0/3] Dealing with the aliases of SI_USER Eric W. Biederman ` (2 preceding siblings ...) 2018-04-15 15:59 ` [RFC PATCH 3/3] signal: Stop special casing TRAP_FIXME and FPE_FIXME in siginfo_layout Eric W. Biederman @ 2018-04-15 18:16 ` Linus Torvalds 2018-04-16 2:03 ` Eric W. Biederman ` (2 more replies) 3 siblings, 3 replies; 38+ messages in thread From: Linus Torvalds @ 2018-04-15 18:16 UTC (permalink / raw) To: Eric W. Biederman Cc: Dave Martin, Linux Kernel Mailing List, Dmitry V. Levin, sparclinux, ppc-dev, linux-arm-kernel, Russell King - ARM Linux, linux-arch ( On Sun, Apr 15, 2018 at 8:56 AM, Eric W. Biederman <ebiederm@xmission.com> wrote: > > Would you consider the patchset below for -rc2? Ugh. I have an irrational dislike of "clear_siginfo()". It's a nasty broken interface, which just mis-spells "memset()", and makes it non-obvious that you could clear it other ways too (eg "kzalloc()" or whatever). And this series brings in a lot of new users. Honestly, both "clear_siginfo()" and "copy_siginfo()" are crazy interfaces. They may have made sense when converting code, but not so much now. If we want to have a function that initializes a siginfo, I think it should _actually_ initialize it, and at least take the two fields that a siginfo has to have to be valid: si_signo and si_code. So I'd rather see a patch that does something like this: - clear_siginfo(info); - info->si_signo = sig; - info->si_errno = 0; - info->si_code = SI_USER; - info->si_pid = 0; - info->si_uid = 0; + init_siginfo(info, sig, SI_USER); which at least makes that function be *worth* something, not just a bad spelling of memset. It's not just the removal of pointless "set to zero", it's the whole concept of "this function actually makes a valid siginfo", which is lacking in the existing function. (Yeah, yeah, si_errno is "generic" too and always part of a siginfo, but nobody cares. It's pretty much always set to zero, it would be stupid to add that interface to the "init_siginfo()" function. So just clearing it is fine, the one or two places that want to set it to some silly value can do it themselves). Then your series would incidentally also: (a) make for fewer lines overall, rather than add lines (b) make it clear that we always initialize si_code, which now *must* be a valid value with all the recent siginfo changes. Hmm? The other thing we should do is to get rid of the stupid padding. Right now "struct siginfo" is pointlessly padded to 128 bytes. That is completely insane, when it's always just zero in the kernel. So put that _pad[] thing inside #ifndef __KERNEL__, and make copy_siginfo_to_user() write the padding zeroes when copying to user space. The reason for the padding is "future expansion", so we do want to tell the user space that it's maybe up to 128 bytes in size, but if we don't fill it all, we shouldn't waste time and memory on clearing the padding internally. I'm certainly *hoping* nobody depends on the whole 128 bytes in rt_sigqueueinfo(). In theory you can fill it all (as long as si_code is negative), but the man-page only says "si_value", and the compat function doesn't copy any more than that either, so any user that tries to fill in more than si_value is already broken. In fact, it might even be worth enforcing that in rt_sigqueueinfo(), just to see if anybody plays any games.. On x86-64, without the pointless padding, the size of 'struct siginfo' inside the kernel would be 48 bytes. That's quite a big difference for something that is often allocated on the kernel stack. So I'm certainly willing to make those kinds of changes, but let's make them real *improvements* now, ok? Wasn't that the point of all the cleanups in the end? Linus ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [RFC PATCH 0/3] Dealing with the aliases of SI_USER 2018-04-15 18:16 ` [RFC PATCH 0/3] Dealing with the aliases of SI_USER Linus Torvalds @ 2018-04-16 2:03 ` Eric W. Biederman 2018-04-18 17:58 ` Eric W. Biederman 2018-04-19 9:28 ` Dave Martin 2 siblings, 0 replies; 38+ messages in thread From: Eric W. Biederman @ 2018-04-16 2:03 UTC (permalink / raw) To: Linus Torvalds Cc: Dave Martin, Linux Kernel Mailing List, Dmitry V. Levin, sparclinux, ppc-dev, linux-arm-kernel, Russell King - ARM Linux, linux-arch Linus Torvalds <torvalds@linux-foundation.org> writes: > ( > > On Sun, Apr 15, 2018 at 8:56 AM, Eric W. Biederman > <ebiederm@xmission.com> wrote: >> >> Would you consider the patchset below for -rc2? > > Ugh. The point of this series is to squash the potential for regressions even from the weird broken code that fills in fields for si_code 0 that do not match SI_USER. The copy_siginfo_to_user32 never handled that so we don't need to worry about that. All of the signals that abuse si_code 0 go through force_sig_info so signalfd can not catch them. Which means that only copy_siginfo_to_user needs to be worried about. Last time I was benchmarking I could not see a difference between copying the entire siginfo and only a small part so I don't see the point of optimizing now. For an actual cleanup I intend to go farther than you are proposing and convert everthing to the set of helper functions I have added to kernel/signal.c force_sig_fault, force_sig_mceerr, force_sig_bnderr, force_sig_pkuerr. When I am done there will be maybe 5 instances of clear_siginfo outside of those helper functions. At which point I won't care if we remove clear_siginfo and just use memset. That is a real improvement that looks something like: "105 files changed, 933 insertions(+), 1698 deletions(-)" That is my end goal with all of this. You complained to me about regressions and you are right with the current handling of FPE_FIXME TRAP_FIXME the code is not bug compatible with old versions of linux, and people have noticed. What this patchset represents is the bare minimum needed to convert copy_siginfo_to_user to a copy_to_user, and remove the possibility of regressions. To that end I need to ensure every struct siginfo in the entire kernel is memset to 0. A structure initializer is absolutely not enough. So I don't mind people thinking clear_siginfo is necessary. To that end clear_siginfo where it does not take the size of struct siginfo as a parameter is much less suceptible to typos, and allows me to ensure every instance of struct siginfo is fully initialized. I fully intend to remove practically every instance of struct siginfo from the architecture specific code, and use the aforementioned helpers. The trouble is that some of the architecture code need refactoring for that to happen. Even your proposed init_siginfo can't be widely used as a common pattern is to fill in siginfo partially in the code with si_code and si_signo being filled in almost last. So in short. I intend to remove most of these clear_siginfo calls when I remove the siginfos later. This series is focused on making copy_siginfo_to_user simple enough we don't need to use siginfo_layout, and thus can be fully backwards compatibile with ourselves and we won't need to worry about regressions. I have aimed to keep this simple enough we can merge this after -rc1 because we don't want regressions. Eric ps. I intend to place this change first in my series wether or not it makes it into -rc2 so that I can be certain we remove any possible regressions in behavior on the buggy architectures. Then I can take my time and ensure the non-trivial changes of refactoring etc are done carefully so I don't introduce other bugs. I need that so I can sleep at night. pps. I can look at some of your other suggestions but cleverness leads to regressions, and if you are going to complain at me harshly when I have been being careful and taking things seriously I am not particularly willing to take unnecessary chances. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [RFC PATCH 0/3] Dealing with the aliases of SI_USER 2018-04-15 18:16 ` [RFC PATCH 0/3] Dealing with the aliases of SI_USER Linus Torvalds 2018-04-16 2:03 ` Eric W. Biederman @ 2018-04-18 17:58 ` Eric W. Biederman 2018-04-19 9:28 ` Dave Martin 2 siblings, 0 replies; 38+ messages in thread From: Eric W. Biederman @ 2018-04-18 17:58 UTC (permalink / raw) To: Linus Torvalds Cc: Dave Martin, Linux Kernel Mailing List, Dmitry V. Levin, sparclinux, ppc-dev, linux-arm-kernel, Russell King - ARM Linux, linux-arch Linus Torvalds <torvalds@linux-foundation.org> writes: > ( > > On Sun, Apr 15, 2018 at 8:56 AM, Eric W. Biederman > <ebiederm@xmission.com> wrote: [snip bit about wanting what is effectively force_sig_fault instead of clear_siginfo everywhere] > The other thing we should do is to get rid of the stupid padding. > Right now "struct siginfo" is pointlessly padded to 128 bytes. That is > completely insane, when it's always just zero in the kernel. > > So put that _pad[] thing inside #ifndef __KERNEL__, and make > copy_siginfo_to_user() write the padding zeroes when copying to user > space. The reason for the padding is "future expansion", so we do want > to tell the user space that it's maybe up to 128 bytes in size, but if > we don't fill it all, we shouldn't waste time and memory on clearing > the padding internally. > > I'm certainly *hoping* nobody depends on the whole 128 bytes in > rt_sigqueueinfo(). In theory you can fill it all (as long as si_code > is negative), but the man-page only says "si_value", and the compat > function doesn't copy any more than that either, so any user that > tries to fill in more than si_value is already broken. In fact, it > might even be worth enforcing that in rt_sigqueueinfo(), just to see > if anybody plays any games.. >From my earlier looking I think this is all doable except detecting if > On x86-64, without the pointless padding, the size of 'struct siginfo' > inside the kernel would be 48 bytes. That's quite a big difference for > something that is often allocated on the kernel stack. >From my earlier looking I can say that I know of no case where signals are injected into the kernel that we need more bytes than the kernel currently provides. The two cases I know of are signal reinjection for checkpoint/restart or something that just uses pid, uid and ptr. Generally that is enough. If we just truncate siginfo to everything except the pad bytes in the kernel there should be no problems. What I don't see how to do is to limit the size of struct siginfo the kernel accepts in a forward compatible way. Something that would fail if for some reason you used more siginfo bytes. Looking at userspace. glibc always memsets siginfo_t to 0. Criu just uses whatever it captured with PTRACE_PEEKSIGINFO, and criu uses unitialized memory for the target of PTRACE_PEEKSIGINFO. If we truncate siginfo in the kernel then we check for a known si_code in which case we are safe to truncate siginfo. If the si_code is unknown then we should check to see if the extra bytes are 0. That should work with everything. Plus it is a natural point to test to see if userspace is using signals that the kernel does not currently support. I will put that in my queue. > So I'm certainly willing to make those kinds of changes, but let's > make them real *improvements* now, ok? Wasn't that the point of all > the cleanups in the end? Definitely. With the strace test case causing people to talk about regressions I was just looking to see if it would make sense to do something minimal for -rc2 so reduce concerns about regressions. Now I am going to focus on getting what I can ready for the next merge window. Eric ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [RFC PATCH 0/3] Dealing with the aliases of SI_USER 2018-04-15 18:16 ` [RFC PATCH 0/3] Dealing with the aliases of SI_USER Linus Torvalds 2018-04-16 2:03 ` Eric W. Biederman 2018-04-18 17:58 ` Eric W. Biederman @ 2018-04-19 9:28 ` Dave Martin 2018-04-19 14:40 ` Eric W. Biederman 2 siblings, 1 reply; 38+ messages in thread From: Dave Martin @ 2018-04-19 9:28 UTC (permalink / raw) To: Linus Torvalds Cc: Eric W. Biederman, linux-arch, Linux Kernel Mailing List, Dmitry V. Levin, sparclinux, Russell King - ARM Linux, ppc-dev, linux-arm-kernel On Sun, Apr 15, 2018 at 11:16:04AM -0700, Linus Torvalds wrote: [...] > The other thing we should do is to get rid of the stupid padding. > Right now "struct siginfo" is pointlessly padded to 128 bytes. That is > completely insane, when it's always just zero in the kernel. Agreed, inside the kernel the padding achieves nothing. > So put that _pad[] thing inside #ifndef __KERNEL__, and make > copy_siginfo_to_user() write the padding zeroes when copying to user > space. The reason for the padding is "future expansion", so we do want > to tell the user space that it's maybe up to 128 bytes in size, but if > we don't fill it all, we shouldn't waste time and memory on clearing > the padding internally. > > I'm certainly *hoping* nobody depends on the whole 128 bytes in > rt_sigqueueinfo(). In theory you can fill it all (as long as si_code > is negative), but the man-page only says "si_value", and the compat > function doesn't copy any more than that either, so any user that > tries to fill in more than si_value is already broken. In fact, it > might even be worth enforcing that in rt_sigqueueinfo(), just to see > if anybody plays any games.. [...] Digression: Since we don't traditionally zero the tail-padding in the user sigframe, is there a reliable way for userspace to detect newly-added fields in siginfo other than by having an explicit sigaction sa_flags flag to request them? I imagine something like [1] below from the userspace perspective. On a separate thread, the issue of how to report syndrome information for SIGSEGV came up [2] (such as whether the faulting instruction was a read or a write). This information is useful (and used) by things like userspace sanitisers and qemu. Currently, reporting this to userspace relies on arch-specific cruft in the sigframe. We're committed to maintaining what's already in each arch sigframe, but it would be preferable to have a portable way of adding information to siginfo in a generic way. si_code doesn't really work for that, since si_codes are mutually exclusive: I can't see a way of adding supplementary information using si_code. Anyway, that would be a separate RFC in the future (if ever). Cheers ---Dave [1] static volatile int have_extflags = 0; static void handler(int n, siginfo_t *si, void *uc) { /* ... */ if (have_extflags) { /* Check si->si_extflags */ } else { /* fallback */ } /* ... */ } int main(void) { /* ... */ struct sigaction sa; /* ... */ sa.sa_flags = SA_SIGINFO | SA_SIGINFO_EXTFLAGS; sa.sa_sigaction = handler; if (!sigaction(SIGSEGV, &sa, NULL)) { have_extflags = 1; } else { sa.sa_flags &= ~SA_SIGINFO_EXTFLAGS; if (sigaction(SIGSEGV, &sa, NULL)) goto error; } /* ... */ } [2] [RFC PATCH] arm64: fault: Don't leak data in ESR context for user fault on kernel VA http://lists.infradead.org/pipermail/linux-arm-kernel/2018-April/571428.html ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [RFC PATCH 0/3] Dealing with the aliases of SI_USER 2018-04-19 9:28 ` Dave Martin @ 2018-04-19 14:40 ` Eric W. Biederman 0 siblings, 0 replies; 38+ messages in thread From: Eric W. Biederman @ 2018-04-19 14:40 UTC (permalink / raw) To: Dave Martin Cc: Linus Torvalds, linux-arch, Linux Kernel Mailing List, Dmitry V. Levin, sparclinux, Russell King - ARM Linux, ppc-dev, linux-arm-kernel Dave Martin <Dave.Martin@arm.com> writes: > On Sun, Apr 15, 2018 at 11:16:04AM -0700, Linus Torvalds wrote: > > [...] > >> The other thing we should do is to get rid of the stupid padding. >> Right now "struct siginfo" is pointlessly padded to 128 bytes. That is >> completely insane, when it's always just zero in the kernel. > > Agreed, inside the kernel the padding achieves nothing. > >> So put that _pad[] thing inside #ifndef __KERNEL__, and make >> copy_siginfo_to_user() write the padding zeroes when copying to user >> space. The reason for the padding is "future expansion", so we do want >> to tell the user space that it's maybe up to 128 bytes in size, but if >> we don't fill it all, we shouldn't waste time and memory on clearing >> the padding internally. >> >> I'm certainly *hoping* nobody depends on the whole 128 bytes in >> rt_sigqueueinfo(). In theory you can fill it all (as long as si_code >> is negative), but the man-page only says "si_value", and the compat >> function doesn't copy any more than that either, so any user that >> tries to fill in more than si_value is already broken. In fact, it >> might even be worth enforcing that in rt_sigqueueinfo(), just to see >> if anybody plays any games.. > > [...] > > Digression: > > Since we don't traditionally zero the tail-padding in the user sigframe, > is there a reliable way for userspace to detect newly-added fields in > siginfo other than by having an explicit sigaction sa_flags flag to > request them? I imagine something like [1] below from the userspace > perspective. > > On a separate thread, the issue of how to report syndrome information > for SIGSEGV came up [2] (such as whether the faulting instruction was a > read or a write). This information is useful (and used) by things like > userspace sanitisers and qemu. Currently, reporting this to userspace > relies on arch-specific cruft in the sigframe. > > We're committed to maintaining what's already in each arch sigframe, > but it would be preferable to have a portable way of adding information > to siginfo in a generic way. si_code doesn't really work for that, > since si_codes are mutually exclusive: I can't see a way of adding > supplementary information using si_code. > > Anyway, that would be a separate RFC in the future (if ever). So far what I have seen done is ``registers'' added to sigcontext. Which it looks like you have done with esr. Scrubbing information from faults to where the addresses point outside of the userspace mapping makes sense. I think before I would pursue what you are talking about on a generic level I would want to look at the fact that we handle unblockable faults wrong. While unlikely it is possible for someone to send a thread specific signal at just the right time, and have that signal delivered before the synchronous fault. Then we could pass through additional arguments through that new ``generic'' path. Especially what are arguments such as tsk->thread.fault_address and tsk->thread.fault_code. We can do anything we like with a new SA_flag as that allows us to change the format of the sigframe. If we are very careful we can add generic fields after that crazy union anonymous union in the _sigfault case of struct siginfo. The trick would be to find something that would be enough so that people don't need to implement their own instruction decoder to see what is going on. Something that is applicable to every sigfault case not just SIGSEGV. Something that we can and want to implement on multiple architectures. The point being doing something generic can be a lot of work, even if it is worth it in the end. > [1] > > static volatile int have_extflags = 0; > > static void handler(int n, siginfo_t *si, void *uc) > { > /* ... */ > > if (have_extflags) { > /* Check si->si_extflags */ > } else { > /* fallback */ > } > > /* ... */ > } > > int main(void) > { > /* ... */ > > struct sigaction sa; > > /* ... */ > > sa.sa_flags = SA_SIGINFO | SA_SIGINFO_EXTFLAGS; > sa.sa_sigaction = handler; > if (!sigaction(SIGSEGV, &sa, NULL)) { > have_extflags = 1; > } else { > sa.sa_flags &= ~SA_SIGINFO_EXTFLAGS; > if (sigaction(SIGSEGV, &sa, NULL)) > goto error; > } > > /* ... */ > } > > [2] [RFC PATCH] arm64: fault: Don't leak data in ESR context for user fault on kernel VA > http://lists.infradead.org/pipermail/linux-arm-kernel/2018-April/571428.html Eric ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid 2018-04-13 17:54 ` Russell King - ARM Linux 2018-04-13 18:23 ` Linus Torvalds @ 2018-04-13 18:35 ` Dave Martin 2018-04-13 18:50 ` Russell King - ARM Linux 1 sibling, 1 reply; 38+ messages in thread From: Dave Martin @ 2018-04-13 18:35 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Linus Torvalds, Linux Kernel Mailing List, Dmitry V. Levin, Eric W. Biederman, sparclinux, ppc-dev, linux-arm-kernel On Fri, Apr 13, 2018 at 06:54:08PM +0100, Russell King - ARM Linux wrote: > On Fri, Apr 13, 2018 at 06:08:28PM +0100, Dave Martin wrote: > > On Fri, Apr 13, 2018 at 09:33:17AM -0700, Linus Torvalds wrote: > > > On Fri, Apr 13, 2018 at 2:42 AM, Russell King - ARM Linux > > > <linux@armlinux.org.uk> wrote: > > > > > > > > Yes, it does solve the problem at hand with strace - the exact patch I > > > > tested against 4.16 is below. > > > > > > Ok, good. > > > > > > > However, FPE_FLTUNK is not defined in older kernels, so while we can > > > > fix it this way for the current merge window, that doesn't help 4.16. > > > > > > I wonder if we should even bother with FPE_FLTUNK. > > > > > > I suspect we might as well use FPE_FLTINV, I suspect, and not have > > > this complexity at all. That case is not worth worrying about, since > > > it's a "this shouldn't happen anyway" and the *real* reason will be in > > > the kernel logs due to vfs_panic(). > > > > > > So it's not like this is something that the user should ever care > > > about the si_code about. > > > > Ack, my intended meaning for FPE_FLTUNK is that the fp exception is > > either spurious or we can't tell easily (or possibly at all) which > > FPE_XXX should be returned. It's up to userspace to figure it out > > if it really cares. Previously we were accidentally returning SI_USER > > in si_code for arm64. > > > > This case on arm looks like a more serious error for which FPE_FLTINV > > may be more appropriate anyway. > > No. The cases where we get to this point are: > > 1. A trap concerning a coprocessor register transfer instruction (iow, move > between a VFP register and ARM register.) > 2. A trap concerning a coprocessor register load or save instruction. > > (In both of these, "concerning" means that the VFP hardware provides > such an instruction as the reason for the fault, *not* that it is the > faulting instruction.) > > 3. A combination of the exception bits (EX and DEX) on certain VFP > implementations. > > All of these can be summarised as "the hardware went wrong in some way" > rather than "the user program did something wrong." Although my understanding of VFP bounces is a bit hazy, I think this is broadly in line with my assumptions. > FPE_FLTINV means "floating point invalid operation". Does it really > cover the case where hardware has failed, or is it intended to cover > the case where userspace did something wrong and asked for an invalid > operation from the FP hardware? So, there's an argument that FPE_FLTINV is not really correct. My rationale was that there is nothing correct that we can return, and FPE_FLTINV may be no worse than the alternatives. If we can only hit this case as the result of a hardware failure or kernel bug though, should this be delivered as SIGKILL instead? That's the approach I eventually followed for various exceptions on arm64 that were theoretically delivered to userspace with si_code==0, but really should be impossible unless and kernel and/or hardware is buggy. If that's the case though, I don't see how a userspace testsuite is hitting this code path. Maybe I've misunderstood the context of this thread. Cheers ---Dave ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid 2018-04-13 18:35 ` sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid Dave Martin @ 2018-04-13 18:50 ` Russell King - ARM Linux 2018-04-13 18:56 ` Dave Martin 0 siblings, 1 reply; 38+ messages in thread From: Russell King - ARM Linux @ 2018-04-13 18:50 UTC (permalink / raw) To: Dave Martin Cc: Linus Torvalds, Linux Kernel Mailing List, Dmitry V. Levin, Eric W. Biederman, sparclinux, ppc-dev, linux-arm-kernel On Fri, Apr 13, 2018 at 07:35:38PM +0100, Dave Martin wrote: > If that's the case though, I don't see how a userspace testsuite is > hitting this code path. Maybe I've misunderstood the context of this > thread. It isn't hitting this exact case. The userspace testsuite is hitting an entirely different case: kill(getpid(), SIGFPE); As one expects, this generates a SIGFPE to the current process, which then inspects the siginfo structure. Being a userspace generated signal, si_code is set to SI_USER, which has the value 0. With FPE_FIXME defined to zero, as Eric has done: enum siginfo_layout siginfo_layout(int sig, int si_code) { enum siginfo_layout layout = SIL_KILL; if ((si_code > SI_USER) && (si_code < SI_KERNEL)) { ... } else { ... #ifdef FPE_FIXME if ((sig == SIGFPE) && (si_code == FPE_FIXME)) layout = SIL_FAULT; #endif } return layout; } This causes siginfo_layout() to return SIL_FAULT for this userspace generated signal, rather than the correct SIL_KILL. This affects which fields we copy to userspace. SI_USER is defined to pass si_pid and si_uid to the userspace process, which on ARM are the first two consecutive 32-bit quantities in the union, which is done when siginfo_layout() returns SIL_KILL. However, when SIL_FAULT is returned, we only copy si_addr in the union, which on ARM is just one 32-bit quantity. Consequently, userspace sees a correct value for si_pid, and si_uid remains set to whatever was there in userspace. In the case of the strace program, that's zero. This means if you run the strace testsuite as root, the problem doesn't appear, but if you run it as a non-root user, it will. So, the testsuite case has little to do with the behaviour of the VFP handling - it's to do with the behaviour of the kernel's signal handling. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up According to speedtest.net: 8.21Mbps down 510kbps up ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid 2018-04-13 18:50 ` Russell King - ARM Linux @ 2018-04-13 18:56 ` Dave Martin 0 siblings, 0 replies; 38+ messages in thread From: Dave Martin @ 2018-04-13 18:56 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Linus Torvalds, Linux Kernel Mailing List, Dmitry V. Levin, Eric W. Biederman, sparclinux, ppc-dev, linux-arm-kernel On Fri, Apr 13, 2018 at 07:50:17PM +0100, Russell King - ARM Linux wrote: > On Fri, Apr 13, 2018 at 07:35:38PM +0100, Dave Martin wrote: > > If that's the case though, I don't see how a userspace testsuite is > > hitting this code path. Maybe I've misunderstood the context of this > > thread. > > It isn't hitting this exact case. > > The userspace testsuite is hitting an entirely different case: > > kill(getpid(), SIGFPE); > > As one expects, this generates a SIGFPE to the current process, which > then inspects the siginfo structure. Being a userspace generated > signal, si_code is set to SI_USER, which has the value 0. > > With FPE_FIXME defined to zero, as Eric has done: > > enum siginfo_layout siginfo_layout(int sig, int si_code) > { > enum siginfo_layout layout = SIL_KILL; > if ((si_code > SI_USER) && (si_code < SI_KERNEL)) { > ... > } else { > ... > #ifdef FPE_FIXME > if ((sig == SIGFPE) && (si_code == FPE_FIXME)) > layout = SIL_FAULT; > #endif > } > return layout; > } > > This causes siginfo_layout() to return SIL_FAULT for this userspace > generated signal, rather than the correct SIL_KILL. > > This affects which fields we copy to userspace. > > SI_USER is defined to pass si_pid and si_uid to the userspace process, > which on ARM are the first two consecutive 32-bit quantities in the > union, which is done when siginfo_layout() returns SIL_KILL. However, > when SIL_FAULT is returned, we only copy si_addr in the union, which > on ARM is just one 32-bit quantity. > > Consequently, userspace sees a correct value for si_pid, and si_uid > remains set to whatever was there in userspace. In the case of the > strace program, that's zero. This means if you run the strace > testsuite as root, the problem doesn't appear, but if you run it as > a non-root user, it will. > > So, the testsuite case has little to do with the behaviour of the VFP > handling - it's to do with the behaviour of the kernel's signal handling. Oh, right. So, going back to the unhandled VFP bounce question, is it reasonable for that to be a SIGKILL? That avoids the question of what si_code userspace should see, because userspace doesn't get to see siginfo at all in that case: it's dead. Or do we hit this in real situations that we want userspace to bail out of more gracefully? Cheers ---Dave ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid 2018-04-12 16:50 ` Linus Torvalds 2018-04-12 17:20 ` Russell King - ARM Linux @ 2018-04-12 17:35 ` Dmitry V. Levin 1 sibling, 0 replies; 38+ messages in thread From: Dmitry V. Levin @ 2018-04-12 17:35 UTC (permalink / raw) To: Linus Torvalds Cc: Russell King - ARM Linux, Eric W. Biederman, sparclinux, ppc-dev, Linux Kernel Mailing List, linux-arm-kernel [-- Attachment #1: Type: text/plain, Size: 2526 bytes --] On Thu, Apr 12, 2018 at 09:50:26AM -0700, Linus Torvalds wrote: > Does this attached patch perhaps fix the ARM case? > > It just uses FPE_FLTUNK as the default si_code for SIGFPE, which seems > sane enough. And then gets rid of FPE_FIXME, which should resolve the > nasty case. > > Hmm? Entirely untested, and I didn't really look at the test-case in > question since I can't really run it anyway. > > Well, I could run it all on x86-64, but it doesn't have that FPE_FIXME > case at all. > > Linus > arch/arm/include/uapi/asm/siginfo.h | 7 ------- > arch/arm/vfp/vfpmodule.c | 4 ++-- > 2 files changed, 2 insertions(+), 9 deletions(-) > > diff --git a/arch/arm/include/uapi/asm/siginfo.h b/arch/arm/include/uapi/asm/siginfo.h > index d0513880be21..d87beeedb4c4 100644 > --- a/arch/arm/include/uapi/asm/siginfo.h > +++ b/arch/arm/include/uapi/asm/siginfo.h > @@ -3,11 +3,4 @@ > > #include <asm-generic/siginfo.h> > > -/* > - * SIGFPE si_codes > - */ > -#ifdef __KERNEL__ > -#define FPE_FIXME 0 /* Broken dup of SI_USER */ > -#endif /* __KERNEL__ */ > - > #endif Looks like the whole file should go away. > diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c > index 4c375e11ae95..012c6e690303 100644 > --- a/arch/arm/vfp/vfpmodule.c > +++ b/arch/arm/vfp/vfpmodule.c > @@ -251,13 +251,13 @@ static void vfp_panic(char *reason, u32 inst) > */ > static void vfp_raise_exceptions(u32 exceptions, u32 inst, u32 fpscr, struct pt_regs *regs) > { > - int si_code = 0; > + int si_code = FPE_FLTUNK; Note that this change would affect the following code at the end of vfp_raise_exceptions: if (si_code) vfp_raise_sigfpe(si_code, regs); > pr_debug("VFP: raising exceptions %08x\n", exceptions); > > if (exceptions == VFP_EXCEPTION_ERROR) { > vfp_panic("unhandled bounce", inst); > - vfp_raise_sigfpe(FPE_FIXME, regs); > + vfp_raise_sigfpe(si_code, regs); > return; > } > To be on the safe side, I'd just change it this way: diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c index 4c375e1..66a73ba 100644 --- a/arch/arm/vfp/vfpmodule.c +++ b/arch/arm/vfp/vfpmodule.c @@ -257,7 +257,7 @@ static void vfp_raise_exceptions(u32 exceptions, u32 inst, u32 fpscr, struct pt_ if (exceptions == VFP_EXCEPTION_ERROR) { vfp_panic("unhandled bounce", inst); - vfp_raise_sigfpe(FPE_FIXME, regs); + vfp_raise_sigfpe(FPE_FLTUNK, regs); return; } -- ldv [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply related [flat|nested] 38+ messages in thread
end of thread, other threads:[~2018-04-19 14:41 UTC | newest] Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-09 15:22 ppc compat v4.16 regression: sending SIGTRAP or SIGFPE via kill() returns wrong values in si_pid and si_uid Dmitry V. Levin 2018-04-12 1:34 ` sparc/ppc/arm compat siginfo ABI regressions: sending " Dmitry V. Levin 2018-04-12 1:45 ` Linus Torvalds 2018-04-12 9:58 ` Russell King - ARM Linux 2018-04-12 11:03 ` Dmitry V. Levin 2018-04-12 12:19 ` Russell King - ARM Linux 2018-04-12 12:49 ` Dmitry V. Levin 2018-04-12 13:14 ` Russell King - ARM Linux 2018-04-12 16:50 ` Linus Torvalds 2018-04-12 17:20 ` Russell King - ARM Linux 2018-04-12 17:22 ` Linus Torvalds 2018-04-13 9:42 ` Russell King - ARM Linux 2018-04-13 16:33 ` Linus Torvalds 2018-04-13 17:08 ` Dave Martin 2018-04-13 17:54 ` Russell King - ARM Linux 2018-04-13 18:23 ` Linus Torvalds 2018-04-13 18:45 ` Dave Martin 2018-04-13 19:53 ` Linus Torvalds 2018-04-15 13:12 ` Russell King - ARM Linux 2018-04-15 15:22 ` Eric W. Biederman 2018-04-15 15:56 ` [RFC PATCH 0/3] Dealing with the aliases of SI_USER Eric W. Biederman 2018-04-15 15:57 ` [RFC PATCH 1/3] signal: Ensure every siginfo we send has all bits initialized Eric W. Biederman 2018-04-17 13:23 ` Dave Martin 2018-04-17 19:37 ` Eric W. Biederman 2018-04-18 12:47 ` Dave Martin 2018-04-18 14:22 ` Eric W. Biederman 2018-04-19 8:26 ` Dave Martin 2018-04-15 15:58 ` [RFC PATCH 2/3] signal: Reduce copy_siginfo_to_user to just copy_to_user Eric W. Biederman 2018-04-15 15:59 ` [RFC PATCH 3/3] signal: Stop special casing TRAP_FIXME and FPE_FIXME in siginfo_layout Eric W. Biederman 2018-04-15 18:16 ` [RFC PATCH 0/3] Dealing with the aliases of SI_USER Linus Torvalds 2018-04-16 2:03 ` Eric W. Biederman 2018-04-18 17:58 ` Eric W. Biederman 2018-04-19 9:28 ` Dave Martin 2018-04-19 14:40 ` Eric W. Biederman 2018-04-13 18:35 ` sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid Dave Martin 2018-04-13 18:50 ` Russell King - ARM Linux 2018-04-13 18:56 ` Dave Martin 2018-04-12 17:35 ` Dmitry V. Levin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).