[PATCH 04/11] signal/parisc: Document a conflict with SI_USER with SIGFPE

[PATCH 04/11] signal/parisc: Document a conflict with SI_USER with SIGFPE

[Eric W. Biederman]

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>
---
 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;
-- 
2.14.1

Re: [PATCH 04/11] signal/parisc: Document a conflict with SI_USER with SIGFPE

[Helge Deller]

* Eric W. Biederman <ebiederm@xmission.com>:
> 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.


I think we should fix it, even if we now break the ABI.

It's about a "conditional trap" which needs to be handled by userspace.
I doubt there is any Linux code out which is utilizing this
parisc-specific trap.

I'd suggest to add a new FPE trap si_code (e.g. FPE_CONDTRAP).
While at it, maybe we should include the already existing FPE_MDAOVF
from the frv architecture, so that arch/frv/include/uapi/asm/siginfo.h
can go completely.

Suggested patch is below.

I'm willing to test the patch below on the parisc architecture for a few
weeks. And it will break arch/x86/kernel/signal_compat.c which needs
looking at then too.

Thoughts?

Helge



[PATCH] parisc: Add FPE_CONDTRAP for conditional trap handling

Posix and common sense requires that SI_USER not be a signal specific
si_code.  Thus add a new FPE_CONDTRAP si_code for conditional traps.

Signed-off-by: Helge Deller <deller@gmx.de>

diff --git a/arch/parisc/kernel/traps.c b/arch/parisc/kernel/traps.c
index 8453724b8009..13702f0f5ba1 100644
--- a/arch/parisc/kernel/traps.c
+++ b/arch/parisc/kernel/traps.c
@@ -627,9 +627,9 @@ void notrace handle_interruption(int code, struct pt_regs *regs)
 		   on condition  */
 		if(user_mode(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;
+			/* Let userspace app figure out from the insn pointed
+			 * to by si_addr */
+			si.si_code = FPE_CONDTRAP;
 			si.si_addr = (void __user *) regs->iaoq[0];
 			force_sig_info(SIGFPE, &si, current);
 			return;
diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h
index e447283b8f52..2b759fe42142 100644
--- a/include/uapi/asm-generic/siginfo.h
+++ b/include/uapi/asm-generic/siginfo.h
@@ -193,7 +193,9 @@ typedef struct siginfo {
 #define FPE_FLTRES	6	/* floating point inexact result */
 #define FPE_FLTINV	7	/* floating point invalid operation */
 #define FPE_FLTSUB	8	/* subscript out of range */
-#define NSIGFPE		8
+#define FPE_MDAOVF	9       /* media overflow */
+#define FPE_CONDTRAP	10      /* trap on condition */
+#define NSIGFPE		10
 
 /*
  * SIGSEGV si_codes

Re: [PATCH 04/11] signal/parisc: Document a conflict with SI_USER with SIGFPE

[Eric W. Biederman]

Helge Deller <deller@gmx.de> writes:

> * Eric W. Biederman <ebiederm@xmission.com>:
>> 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.
>
>
> I think we should fix it, even if we now break the ABI.
>
> It's about a "conditional trap" which needs to be handled by userspace.
> I doubt there is any Linux code out which is utilizing this
> parisc-specific trap.
>
> I'd suggest to add a new FPE trap si_code (e.g. FPE_CONDTRAP).
> While at it, maybe we should include the already existing FPE_MDAOVF
> from the frv architecture, so that arch/frv/include/uapi/asm/siginfo.h
> can go completely.
>
> Suggested patch is below.
>
> I'm willing to test the patch below on the parisc architecture for a few
> weeks. And it will break arch/x86/kernel/signal_compat.c which needs
> looking at then too.
>
> Thoughts?

I like it.

We have the option of bringing either the ia64 or the frv si_codes
into the generic fold.  Is there any reason you choose frv?
Last I looked ia64 tended in many aspects to be well thought out,
and thus worth a careful look.

Given that a couple of weeks likely puts on the other side of the merge
window I would like to start with my patch so I can close the potential
copying of unitialized memory to userspace.  Then we can build yours on
top.

Although I am more than happy to add new si_codes now.

What I am in the final stages of testing and reviewing internally is the
change to merge all of struct siginfo, struct compat_siginfo,
copy_siginfo_from_user32 and copy_siginfo_to_user32 together.

I need another couple hours and I will be ready to post that.

For long term maintenance the more we can merge together the better,
as clearly some of these bugs have persisted far too long.  And getting
collapsing the arch specific si_codes into just a set of si_codes
looks like one more good step in that direction.

Eric


> Helge
>
>
>
> [PATCH] parisc: Add FPE_CONDTRAP for conditional trap handling
>
> Posix and common sense requires that SI_USER not be a signal specific
> si_code.  Thus add a new FPE_CONDTRAP si_code for conditional traps.
>
> Signed-off-by: Helge Deller <deller@gmx.de>
>
> diff --git a/arch/parisc/kernel/traps.c b/arch/parisc/kernel/traps.c
> index 8453724b8009..13702f0f5ba1 100644
> --- a/arch/parisc/kernel/traps.c
> +++ b/arch/parisc/kernel/traps.c
> @@ -627,9 +627,9 @@ void notrace handle_interruption(int code, struct pt_regs *regs)
>  		   on condition  */
>  		if(user_mode(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;
> +			/* Let userspace app figure out from the insn pointed
> +			 * to by si_addr */
> +			si.si_code = FPE_CONDTRAP;
>  			si.si_addr = (void __user *) regs->iaoq[0];
>  			force_sig_info(SIGFPE, &si, current);
>  			return;
> diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h
> index e447283b8f52..2b759fe42142 100644
> --- a/include/uapi/asm-generic/siginfo.h
> +++ b/include/uapi/asm-generic/siginfo.h
> @@ -193,7 +193,9 @@ typedef struct siginfo {
>  #define FPE_FLTRES	6	/* floating point inexact result */
>  #define FPE_FLTINV	7	/* floating point invalid operation */
>  #define FPE_FLTSUB	8	/* subscript out of range */
> -#define NSIGFPE		8
> +#define FPE_MDAOVF	9       /* media overflow */
> +#define FPE_CONDTRAP	10      /* trap on condition */
> +#define NSIGFPE		10
>  
>  /*
>   * SIGSEGV si_codes

Re: [PATCH 04/11] signal/parisc: Document a conflict with SI_USER with SIGFPE

[Eric W. Biederman]

ebiederm@xmission.com (Eric W. Biederman) writes:

> Helge Deller <deller@gmx.de> writes:
>
>> * Eric W. Biederman <ebiederm@xmission.com>:
>>> 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.
>>
>>
>> I think we should fix it, even if we now break the ABI.
>>
>> It's about a "conditional trap" which needs to be handled by userspace.
>> I doubt there is any Linux code out which is utilizing this
>> parisc-specific trap.
>>
>> I'd suggest to add a new FPE trap si_code (e.g. FPE_CONDTRAP).
>> While at it, maybe we should include the already existing FPE_MDAOVF
>> from the frv architecture, so that arch/frv/include/uapi/asm/siginfo.h
>> can go completely.
>>
>> Suggested patch is below.
>>
>> I'm willing to test the patch below on the parisc architecture for a few
>> weeks. And it will break arch/x86/kernel/signal_compat.c which needs
>> looking at then too.
>>
>> Thoughts?
>
> I like it.

Your comments about the si_codes caused me to look into how they differ
across the architectures and realize they also all need to be merged
into uapi/asm-generic/siginfo.h for sanity sake.   In doing so I found
a couple of minor issues with my other unifications.

Rebased onto my tree your patch looks like the below.  If it does not
cause any regressions it looks like a perfect fix.  The noticable change
is that the first FPE si_code available across all architectures is 14
so I have used 14 instead of 10 for FPE_CONDTRAP.

Eric

From: Helge Deller <deller@gmx.de>
Date: Sat, 13 Jan 2018 19:32:43 -0600
Subject: [PATCH] signal/parisc: Add FPE_CONDTRAP for conditional trap handling

Posix and common sense requires that SI_USER not be a signal specific
si_code.  Thus add a new FPE_CONDTRAP si_code for conditional traps.

-- EWB rebased onto my tree.

Signed-off-by: Helge Deller <deller@gmx.de>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 arch/parisc/include/uapi/asm/siginfo.h | 7 -------
 arch/parisc/kernel/traps.c             | 7 ++++---
 include/uapi/asm-generic/siginfo.h     | 3 ++-
 3 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/arch/parisc/include/uapi/asm/siginfo.h b/arch/parisc/include/uapi/asm/siginfo.h
index be40331f757d..4a1062e05aaf 100644
--- a/arch/parisc/include/uapi/asm/siginfo.h
+++ b/arch/parisc/include/uapi/asm/siginfo.h
@@ -8,11 +8,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/parisc/kernel/traps.c b/arch/parisc/kernel/traps.c
index c919e6c0a687..68e671a11987 100644
--- a/arch/parisc/kernel/traps.c
+++ b/arch/parisc/kernel/traps.c
@@ -627,9 +627,10 @@ void notrace handle_interruption(int code, struct pt_regs *regs)
 		   on condition  */
 		if(user_mode(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 = FPE_FIXME;
+			/* Let userspace app figure it out from the insn pointed
+			 * to by si_addr.
+			 */
+			si.si_code = FPE_CONDTRAP;
 			si.si_addr = (void __user *) regs->iaoq[0];
 			force_sig_info(SIGFPE, &si, current);
 			return;
diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h
index 254afc31e3be..ab4fad1a0cf0 100644
--- a/include/uapi/asm-generic/siginfo.h
+++ b/include/uapi/asm-generic/siginfo.h
@@ -229,7 +229,8 @@ typedef struct siginfo {
 # define __FPE_INVASC	12	/* invalid ASCII digit */
 # define __FPE_INVDEC	13	/* invalid decimal digit */
 #endif
-#define NSIGFPE		13
+#define FPE_CONDTRAP	14	/* trap on condition */
+#define NSIGFPE		14
 
 /*
  * SIGSEGV si_codes
-- 
2.14.1

Re: [PATCH 04/11] signal/parisc: Document a conflict with SI_USER with SIGFPE

[Eric W. Biederman]

Helge Deller <deller@gmx.de> writes:

> On 23.02.2018 01:15, Eric W. Biederman wrote:
>> Helge Deller <deller@gmx.de> writes:
>> 
>>> * Eric W. Biederman <ebiederm@xmission.com>:
>>>> 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.
>>>
>>>
>>> I think we should fix it, even if we now break the ABI.
>>>
>>> It's about a "conditional trap" which needs to be handled by userspace.
>>> I doubt there is any Linux code out which is utilizing this
>>> parisc-specific trap.
>>>
>>> I'd suggest to add a new FPE trap si_code (e.g. FPE_CONDTRAP).
>>> While at it, maybe we should include the already existing FPE_MDAOVF
>>> from the frv architecture, so that arch/frv/include/uapi/asm/siginfo.h
>>> can go completely.
>>>
>>> Suggested patch is below.
>>>
>>> I'm willing to test the patch below on the parisc architecture for a few
>>> weeks. And it will break arch/x86/kernel/signal_compat.c which needs
>>> looking at then too.
>> 
>> Have you managed to test this change?
>
> Sadly I haven't done any further testing yet.

So at this point for purposed of testing I don't think it matters which
number FPE_CONDTRAP gets as long as it is non-zero.
>  
>> I am sitting looking at another new FPE si_code and if this has been tested
>> I figure FPE_CONDTRAP should get the next available FPE si_code and the
>> other change should get the one that follows.
>
> I'm fine either way. Do you have a git tree I can pull which includes
> all your patches? I can then start testing.

Everything finalized is in Linus's tree.  There is a patch pending
review on linux-arch that defines FPE_FLTUNK that looks to be useful
on several architectures.

I had probably misread our earlier exchange.  I had hoped you had tested
that FPE_CONDTRAP did not cause problems.

If that level of testing was complete I would have given FPE_CONDTRAP
the next FPE number and FPE_FLTUNK the one after.

As it sounds like FPE_CONDTRAP hasn't been tested enough to know if it
causes problems I will encourage the patches to be merged in the other
order.

Eric