* [PATCH AUTOSEL for 4.15 118/124] signal/parisc: Document a conflict with SI_USER with SIGFPE [not found] <20180319154645.11350-1-alexander.levin@microsoft.com> @ 2018-03-19 15:49 ` Sasha Levin 2018-03-20 15:20 ` Eric W. Biederman 0 siblings, 1 reply; 5+ messages in thread From: Sasha Levin @ 2018-03-19 15:49 UTC (permalink / raw) To: linux-kernel@vger.kernel.org, stable@vger.kernel.org Cc: Eric W. Biederman, James E.J. Bottomley, Helge Deller, linux-parisc@vger.kernel.org, Sasha Levin From: "Eric W. Biederman" <ebiederm@xmission.com> [ Upstream commit b5daf2b9d1c9a2b4f03ca93f75913ba2da3b3eaa ] Setting si_code to 0 results in a userspace seeing an si_code of 0. This is the same si_code as SI_USER. Posix and common sense requires that SI_USER not be a signal specific si_code. As such this use of 0 for the si_code is a pretty horribly broken ABI. Further use of si_code =3D=3D 0 guaranteed that copy_siginfo_to_user saw a value of __SI_KILL and now sees a value of SIL_KILL with the result that uid and pid fields are copied and which might copying the si_addr field by accident but certainly not by design. Making this a very flakey implementation. Utilizing FPE_FIXME siginfo_layout will now return SIL_FAULT and the appropriate fields will reliably be copied. This bug is 13 years old and parsic machines are no longer being built so I don't know if it possible or worth fixing it. But it is at least worth documenting this so other architectures don't make the same mistake. Possible ABI fixes includee: - Send the signal without siginfo - Don't generate a signal - Possibly assign and use an appropriate si_code - Don't handle cases which can't happen Cc: "James E.J. Bottomley" <jejb@parisc-linux.org> Cc: Helge Deller <deller@gmx.de> Cc: linux-parisc@vger.kernel.org Ref: 313c01d3e3fd ("[PATCH] PA-RISC update for 2.6.0") Histroy Tree: https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.= git Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> Signed-off-by: Sasha Levin <alexander.levin@microsoft.com> --- arch/parisc/include/uapi/asm/siginfo.h | 7 +++++++ arch/parisc/kernel/traps.c | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/arch/parisc/include/uapi/asm/siginfo.h b/arch/parisc/include/u= api/asm/siginfo.h index 4a1062e05aaf..be40331f757d 100644 --- a/arch/parisc/include/uapi/asm/siginfo.h +++ b/arch/parisc/include/uapi/asm/siginfo.h @@ -8,4 +8,11 @@ =20 #include <asm-generic/siginfo.h> =20 +/* + * SIGFPE si_codes + */ +#ifdef __KERNEL__ +#define FPE_FIXME 0 /* Broken dup of SI_USER */ +#endif /* __KERNEL__ */ + #endif diff --git a/arch/parisc/kernel/traps.c b/arch/parisc/kernel/traps.c index 8453724b8009..c919e6c0a687 100644 --- a/arch/parisc/kernel/traps.c +++ b/arch/parisc/kernel/traps.c @@ -629,7 +629,7 @@ void notrace handle_interruption(int code, struct pt_re= gs *regs) si.si_signo =3D SIGFPE; /* Set to zero, and let the userspace app figure it out from the insn pointed to by si_addr */ - si.si_code =3D 0; + si.si_code =3D FPE_FIXME; si.si_addr =3D (void __user *) regs->iaoq[0]; force_sig_info(SIGFPE, &si, current); return; --=20 2.14.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH AUTOSEL for 4.15 118/124] signal/parisc: Document a conflict with SI_USER with SIGFPE 2018-03-19 15:49 ` [PATCH AUTOSEL for 4.15 118/124] signal/parisc: Document a conflict with SI_USER with SIGFPE Sasha Levin @ 2018-03-20 15:20 ` Eric W. Biederman 2018-03-21 18:18 ` Sasha Levin 0 siblings, 1 reply; 5+ messages in thread From: Eric W. Biederman @ 2018-03-20 15:20 UTC (permalink / raw) To: Sasha Levin Cc: linux-kernel@vger.kernel.org, stable@vger.kernel.org, James E.J. Bottomley, Helge Deller, linux-parisc@vger.kernel.org Sasha Levin <Alexander.Levin@microsoft.com> writes: What is the justification for backporting this and the other similar Documentation commits? These commits just introduce a define _FIXME with value of 0, to document that the userspace ABI was handled incorrectly long ago. These commits do not fix anything. Thes commits do not change anything except a little how they are handled in siginfo_layout. And I don't see the changes that introduce siginfo_layout in kernel/signal.c being backported. Further these commits don't even have a fixes tag so I am curious what is triggering them for backport. Eric > From: "Eric W. Biederman" <ebiederm@xmission.com> > > [ Upstream commit b5daf2b9d1c9a2b4f03ca93f75913ba2da3b3eaa ] > > Setting si_code to 0 results in a userspace seeing an si_code of 0. > This is the same si_code as SI_USER. Posix and common sense requires > that SI_USER not be a signal specific si_code. As such this use of 0 > for the si_code is a pretty horribly broken ABI. > > Further use of si_code == 0 guaranteed that copy_siginfo_to_user saw a > value of __SI_KILL and now sees a value of SIL_KILL with the result > that uid and pid fields are copied and which might copying the si_addr > field by accident but certainly not by design. Making this a very > flakey implementation. > > Utilizing FPE_FIXME siginfo_layout will now return SIL_FAULT and the > appropriate fields will reliably be copied. > > This bug is 13 years old and parsic machines are no longer being built > so I don't know if it possible or worth fixing it. But it is at least > worth documenting this so other architectures don't make the same > mistake. > > Possible ABI fixes includee: > - Send the signal without siginfo > - Don't generate a signal > - Possibly assign and use an appropriate si_code > - Don't handle cases which can't happen > > Cc: "James E.J. Bottomley" <jejb@parisc-linux.org> > Cc: Helge Deller <deller@gmx.de> > Cc: linux-parisc@vger.kernel.org > Ref: 313c01d3e3fd ("[PATCH] PA-RISC update for 2.6.0") > Histroy Tree: https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> > Signed-off-by: Sasha Levin <alexander.levin@microsoft.com> > --- > arch/parisc/include/uapi/asm/siginfo.h | 7 +++++++ > arch/parisc/kernel/traps.c | 2 +- > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/arch/parisc/include/uapi/asm/siginfo.h b/arch/parisc/include/uapi/asm/siginfo.h > index 4a1062e05aaf..be40331f757d 100644 > --- a/arch/parisc/include/uapi/asm/siginfo.h > +++ b/arch/parisc/include/uapi/asm/siginfo.h > @@ -8,4 +8,11 @@ > > #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/parisc/kernel/traps.c b/arch/parisc/kernel/traps.c > index 8453724b8009..c919e6c0a687 100644 > --- a/arch/parisc/kernel/traps.c > +++ b/arch/parisc/kernel/traps.c > @@ -629,7 +629,7 @@ void notrace handle_interruption(int code, struct pt_regs *regs) > si.si_signo = SIGFPE; > /* Set to zero, and let the userspace app figure it out from > the insn pointed to by si_addr */ > - si.si_code = 0; > + si.si_code = FPE_FIXME; > si.si_addr = (void __user *) regs->iaoq[0]; > force_sig_info(SIGFPE, &si, current); > return; ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH AUTOSEL for 4.15 118/124] signal/parisc: Document a conflict with SI_USER with SIGFPE 2018-03-20 15:20 ` Eric W. Biederman @ 2018-03-21 18:18 ` Sasha Levin 2018-03-21 19:49 ` Eric W. Biederman 0 siblings, 1 reply; 5+ messages in thread From: Sasha Levin @ 2018-03-21 18:18 UTC (permalink / raw) To: Eric W. Biederman Cc: linux-kernel@vger.kernel.org, stable@vger.kernel.org, James E.J. Bottomley, Helge Deller, linux-parisc@vger.kernel.org Hey Eric, On Tue, Mar 20, 2018 at 10:20:21AM -0500, Eric W. Biederman wrote: >Sasha Levin <Alexander.Levin@microsoft.com> writes: > >What is the justification for backporting this and the other similar >Documentation commits? It was flagged as a bug fixing patch by a new process we're testing, and when I looked at it I thought that the commit message suggests it fixes an ABI issue. >These commits just introduce a define _FIXME with value of 0, to >document that the userspace ABI was handled incorrectly long ago. > >These commits do not fix anything. Thes commits do not change anything >except a little how they are handled in siginfo_layout. And I don't see >the changes that introduce siginfo_layout in kernel/signal.c being >backported. > >Further these commits don't even have a fixes tag so I am curious >what is triggering them for backport. We're testing out a new mechanism where we train a neural network to detect bug fixing patches and flag them for manual review. We're working on a FAQ + more detailed information right now. >Eric > >> From: "Eric W. Biederman" <ebiederm@xmission.com> >> >> [ Upstream commit b5daf2b9d1c9a2b4f03ca93f75913ba2da3b3eaa ] >> >> Setting si_code to 0 results in a userspace seeing an si_code of 0. >> This is the same si_code as SI_USER. Posix and common sense requires >> that SI_USER not be a signal specific si_code. As such this use of 0 >> for the si_code is a pretty horribly broken ABI. >> >> Further use of si_code =3D=3D 0 guaranteed that copy_siginfo_to_user saw= a >> value of __SI_KILL and now sees a value of SIL_KILL with the result >> that uid and pid fields are copied and which might copying the si_addr >> field by accident but certainly not by design. Making this a very >> flakey implementation. >> >> Utilizing FPE_FIXME siginfo_layout will now return SIL_FAULT and the >> appropriate fields will reliably be copied. >> >> This bug is 13 years old and parsic machines are no longer being built >> so I don't know if it possible or worth fixing it. But it is at least >> worth documenting this so other architectures don't make the same >> mistake. >> >> Possible ABI fixes includee: >> - Send the signal without siginfo >> - Don't generate a signal >> - Possibly assign and use an appropriate si_code >> - Don't handle cases which can't happen >> >> Cc: "James E.J. Bottomley" <jejb@parisc-linux.org> >> Cc: Helge Deller <deller@gmx.de> >> Cc: linux-parisc@vger.kernel.org >> Ref: 313c01d3e3fd ("[PATCH] PA-RISC update for 2.6.0") >> Histroy Tree: https://na01.safelinks.protection.outlook.com/?url=3Dhttps= %3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftglx%2Fhistory= .git&data=3D04%7C01%7CAlexander.Levin%40microsoft.com%7C3dfe7dd42625456fdb0= f08d58e7639e6%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C1%7C6365715607897505= 33%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1h= aWwifQ%3D%3D%7C-1&sdata=3DlXlraRxI0IHdS736PA%2BLO8A4JQJveGitz1pPfpo7QKM%3D&= reserved=3D0 >> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> >> Signed-off-by: Sasha Levin <alexander.levin@microsoft.com> >> --- >> arch/parisc/include/uapi/asm/siginfo.h | 7 +++++++ >> arch/parisc/kernel/traps.c | 2 +- >> 2 files changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/arch/parisc/include/uapi/asm/siginfo.h b/arch/parisc/includ= e/uapi/asm/siginfo.h >> index 4a1062e05aaf..be40331f757d 100644 >> --- a/arch/parisc/include/uapi/asm/siginfo.h >> +++ b/arch/parisc/include/uapi/asm/siginfo.h >> @@ -8,4 +8,11 @@ >> >> #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/parisc/kernel/traps.c b/arch/parisc/kernel/traps.c >> index 8453724b8009..c919e6c0a687 100644 >> --- a/arch/parisc/kernel/traps.c >> +++ b/arch/parisc/kernel/traps.c >> @@ -629,7 +629,7 @@ void notrace handle_interruption(int code, struct pt= _regs *regs) >> si.si_signo =3D SIGFPE; >> /* Set to zero, and let the userspace app figure it out from >> the insn pointed to by si_addr */ >> - si.si_code =3D 0; >> + si.si_code =3D FPE_FIXME; >> si.si_addr =3D (void __user *) regs->iaoq[0]; >> force_sig_info(SIGFPE, &si, current); >> return; --=20 Thanks, Sasha= ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH AUTOSEL for 4.15 118/124] signal/parisc: Document a conflict with SI_USER with SIGFPE 2018-03-21 18:18 ` Sasha Levin @ 2018-03-21 19:49 ` Eric W. Biederman 2018-03-21 19:58 ` Sasha Levin 0 siblings, 1 reply; 5+ messages in thread From: Eric W. Biederman @ 2018-03-21 19:49 UTC (permalink / raw) To: Sasha Levin Cc: linux-kernel@vger.kernel.org, stable@vger.kernel.org, James E.J. Bottomley, Helge Deller, linux-parisc@vger.kernel.org Sasha Levin <Alexander.Levin@microsoft.com> writes: > Hey Eric, > > On Tue, Mar 20, 2018 at 10:20:21AM -0500, Eric W. Biederman wrote: >>Sasha Levin <Alexander.Levin@microsoft.com> writes: >> >>What is the justification for backporting this and the other similar >>Documentation commits? > > It was flagged as a bug fixing patch by a new process we're testing, and > when I looked at it I thought that the commit message suggests it fixes > an ABI issue. Unfortunately they just reveal an ABI issue. I believe there are some fixes coming but given that the issues are a decade old in many cases actually fixing these things must be approach with care so as not to create regressions. >>These commits just introduce a define _FIXME with value of 0, to >>document that the userspace ABI was handled incorrectly long ago. >> >>These commits do not fix anything. Thes commits do not change anything >>except a little how they are handled in siginfo_layout. And I don't see >>the changes that introduce siginfo_layout in kernel/signal.c being >>backported. >> >>Further these commits don't even have a fixes tag so I am curious >>what is triggering them for backport. > > We're testing out a new mechanism where we train a neural network to > detect bug fixing patches and flag them for manual review. We're working > on a FAQ + more detailed information right now. The neural network did seem to pick up on something that is worth looking at. Eric ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH AUTOSEL for 4.15 118/124] signal/parisc: Document a conflict with SI_USER with SIGFPE 2018-03-21 19:49 ` Eric W. Biederman @ 2018-03-21 19:58 ` Sasha Levin 0 siblings, 0 replies; 5+ messages in thread From: Sasha Levin @ 2018-03-21 19:58 UTC (permalink / raw) To: Eric W. Biederman Cc: linux-kernel@vger.kernel.org, stable@vger.kernel.org, James E.J. Bottomley, Helge Deller, linux-parisc@vger.kernel.org On Wed, Mar 21, 2018 at 02:49:25PM -0500, Eric W. Biederman wrote: >Sasha Levin <Alexander.Levin@microsoft.com> writes: > >> Hey Eric, >> >> On Tue, Mar 20, 2018 at 10:20:21AM -0500, Eric W. Biederman wrote: >>>Sasha Levin <Alexander.Levin@microsoft.com> writes: >>> >>>What is the justification for backporting this and the other similar >>>Documentation commits? >> >> It was flagged as a bug fixing patch by a new process we're testing, and >> when I looked at it I thought that the commit message suggests it fixes >> an ABI issue. > >Unfortunately they just reveal an ABI issue. I believe there are some >fixes coming but given that the issues are a decade old in many cases >actually fixing these things must be approach with care so as not to >create regressions. I've removed these commits. >>>These commits just introduce a define _FIXME with value of 0, to >>>document that the userspace ABI was handled incorrectly long ago. >>> >>>These commits do not fix anything. Thes commits do not change anything >>>except a little how they are handled in siginfo_layout. And I don't see >>>the changes that introduce siginfo_layout in kernel/signal.c being >>>backported. >>> >>>Further these commits don't even have a fixes tag so I am curious >>>what is triggering them for backport. >> >> We're testing out a new mechanism where we train a neural network to >> detect bug fixing patches and flag them for manual review. We're working >> on a FAQ + more detailed information right now. > >The neural network did seem to pick up on something that is worth >looking at. Indeed, and we use review input to retrain the NN on these commits. Thank you! --=20 Thanks, Sasha= ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-03-21 19:58 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20180319154645.11350-1-alexander.levin@microsoft.com>
2018-03-19 15:49 ` [PATCH AUTOSEL for 4.15 118/124] signal/parisc: Document a conflict with SI_USER with SIGFPE Sasha Levin
2018-03-20 15:20 ` Eric W. Biederman
2018-03-21 18:18 ` Sasha Levin
2018-03-21 19:49 ` Eric W. Biederman
2018-03-21 19:58 ` Sasha Levin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox