[RFC PATCH v2] openrisc: Union fpcsr and oldmask in sigcontext to unbreak userspace ABI

[RFC PATCH v2] openrisc: Union fpcsr and oldmask in sigcontext to unbreak userspace ABI

[Stafford Horne]

With commit 27267655c531 ("openrisc: Support floating point user api") I
inproperly added an entry to the struct sigcontext which caused an
unwanted to chagne to the userspace ABI.

Since oldmask was never used in OpenRISC we now use it's space for the
floating point fpcsr state.  We do this with a union to restore the ABI
back to the pre kernel v6.4 ABI and keep API compatibility.

This does mean if there is some code somewhere that is setting oldmask
in a userspace sighandler it would end up setting the floating point
register status, but I think it's unlikely as oldmask was never used
before.

Fixes: 27267655c531 ("openrisc: Support floating point user api")
Reported-by: Szabolcs Nagy <nsz@port70.net>
Closes: https://lore.kernel.org/openrisc/20230626213840.GA1236108@port70.net/
Signed-off-by: Stafford Horne <shorne@gmail.com>
---
Changes since v1:
 - Rather than revert the change, just use the oldmask slot for fpu state.

 arch/openrisc/include/uapi/asm/sigcontext.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/openrisc/include/uapi/asm/sigcontext.h b/arch/openrisc/include/uapi/asm/sigcontext.h
index ca585e4af6b8..93817d7051aa 100644
--- a/arch/openrisc/include/uapi/asm/sigcontext.h
+++ b/arch/openrisc/include/uapi/asm/sigcontext.h
@@ -28,8 +28,10 @@
 
 struct sigcontext {
 	struct user_regs_struct regs;  /* needs to be first */
-	struct __or1k_fpu_state fpu;
-	unsigned long oldmask;
+	union {
+		struct __or1k_fpu_state fpu;
+		unsigned long oldmask;	/* unused */
+	};
 };
 
 #endif /* __ASM_OPENRISC_SIGCONTEXT_H */
-- 
2.39.1

Re: [RFC PATCH v2] openrisc: Union fpcsr and oldmask in sigcontext to unbreak userspace ABI

[Rich Felker]

On Wed, Jul 05, 2023 at 10:26:45PM +0100, Stafford Horne wrote:
> With commit 27267655c531 ("openrisc: Support floating point user api") I
> inproperly added an entry to the struct sigcontext which caused an
> unwanted to chagne to the userspace ABI.
> 
> Since oldmask was never used in OpenRISC we now use it's space for the
> floating point fpcsr state.  We do this with a union to restore the ABI
> back to the pre kernel v6.4 ABI and keep API compatibility.
> 
> This does mean if there is some code somewhere that is setting oldmask
> in a userspace sighandler it would end up setting the floating point
> register status, but I think it's unlikely as oldmask was never used
> before.
> 
> Fixes: 27267655c531 ("openrisc: Support floating point user api")
> Reported-by: Szabolcs Nagy <nsz@port70.net>
> Closes: https://lore.kernel.org/openrisc/20230626213840.GA1236108@port70.net/
> Signed-off-by: Stafford Horne <shorne@gmail.com>
> ---
> Changes since v1:
>  - Rather than revert the change, just use the oldmask slot for fpu state.
> 
>  arch/openrisc/include/uapi/asm/sigcontext.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/openrisc/include/uapi/asm/sigcontext.h b/arch/openrisc/include/uapi/asm/sigcontext.h
> index ca585e4af6b8..93817d7051aa 100644
> --- a/arch/openrisc/include/uapi/asm/sigcontext.h
> +++ b/arch/openrisc/include/uapi/asm/sigcontext.h
> @@ -28,8 +28,10 @@
>  
>  struct sigcontext {
>  	struct user_regs_struct regs;  /* needs to be first */
> -	struct __or1k_fpu_state fpu;
> -	unsigned long oldmask;
> +	union {
> +		struct __or1k_fpu_state fpu;
> +		unsigned long oldmask;	/* unused */
> +	};
>  };
>  
>  #endif /* __ASM_OPENRISC_SIGCONTEXT_H */
> -- 
> 2.39.1

Correctness/ABI-safety of this patch depends on struct
__or1k_fpu_state being just a single unsigned long and never changing.
Is this something it's possible to commit to? Otherwise, a much better
idea would be making the union contain a struct __or1k_fpu_state *
or some more general pointer type that points to an out-of-band,
expandable data structure past the end of the ucontext_t.

Rich

Re: [RFC PATCH v2] openrisc: Union fpcsr and oldmask in sigcontext to unbreak userspace ABI

[Stafford Horne]

On Wed, Jul 05, 2023 at 08:59:16PM -0400, Rich Felker wrote:
> On Wed, Jul 05, 2023 at 10:26:45PM +0100, Stafford Horne wrote:
> > With commit 27267655c531 ("openrisc: Support floating point user api") I
> > inproperly added an entry to the struct sigcontext which caused an
> > unwanted to chagne to the userspace ABI.
> > 
> > Since oldmask was never used in OpenRISC we now use it's space for the
> > floating point fpcsr state.  We do this with a union to restore the ABI
> > back to the pre kernel v6.4 ABI and keep API compatibility.
> > 
> > This does mean if there is some code somewhere that is setting oldmask
> > in a userspace sighandler it would end up setting the floating point
> > register status, but I think it's unlikely as oldmask was never used
> > before.
> > 
> > Fixes: 27267655c531 ("openrisc: Support floating point user api")
> > Reported-by: Szabolcs Nagy <nsz@port70.net>
> > Closes: https://lore.kernel.org/openrisc/20230626213840.GA1236108@port70.net/
> > Signed-off-by: Stafford Horne <shorne@gmail.com>
> > ---
> > Changes since v1:
> >  - Rather than revert the change, just use the oldmask slot for fpu state.
> > 
> >  arch/openrisc/include/uapi/asm/sigcontext.h | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/openrisc/include/uapi/asm/sigcontext.h b/arch/openrisc/include/uapi/asm/sigcontext.h
> > index ca585e4af6b8..93817d7051aa 100644
> > --- a/arch/openrisc/include/uapi/asm/sigcontext.h
> > +++ b/arch/openrisc/include/uapi/asm/sigcontext.h
> > @@ -28,8 +28,10 @@
> >  
> >  struct sigcontext {
> >  	struct user_regs_struct regs;  /* needs to be first */
> > -	struct __or1k_fpu_state fpu;
> > -	unsigned long oldmask;
> > +	union {
> > +		struct __or1k_fpu_state fpu;
> > +		unsigned long oldmask;	/* unused */
> > +	};
> >  };
> >  
> >  #endif /* __ASM_OPENRISC_SIGCONTEXT_H */
> > -- 
> > 2.39.1
> 
> Correctness/ABI-safety of this patch depends on struct
> __or1k_fpu_state being just a single unsigned long and never changing.
> Is this something it's possible to commit to? Otherwise, a much better
> idea would be making the union contain a struct __or1k_fpu_state *
> or some more general pointer type that points to an out-of-band,
> expandable data structure past the end of the ucontext_t.

Thank you for reviewing.

Yes, __or1k_fpu_state I intend to always contain just the single unsigned long.

Thinking about it, I should just change sigcontext from using struct
__or1k_fpu_state to simply 'unsigned long fpcsr' to make this more clear. I will
do this for v3.

If further expansion is needed we can add more structs after ucontext_t as you
mention.  I was considering doing that but found the unused space in oldmask.

-Stafford