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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 5 Jul 2023 20:59:16 -0400
From:   Rich Felker <dalias@...c.org>
To:     Stafford Horne <shorne@...il.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Szabolcs Nagy <nsz@...t70.net>,
        Jonas Bonn <jonas@...thpole.se>,
        Stefan Kristiansson <stefan.kristiansson@...nalahti.fi>,
        linux-openrisc@...r.kernel.org
Subject: Re: [RFC PATCH v2] openrisc: Union fpcsr and oldmask in sigcontext
 to unbreak userspace ABI

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@...t70.net>
> Closes: https://lore.kernel.org/openrisc/20230626213840.GA1236108@port70.net/
> Signed-off-by: Stafford Horne <shorne@...il.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