Re: [PATCH v2 04/24] arm64/fpsimd: signal: Consistently read FPSIMD context

[Will Deacon <will@kernel.org>]

On Thu, May 08, 2025 at 02:26:24PM +0100, Mark Rutland wrote:
> For historical reasons, restore_sve_fpsimd_context() has an open-coded
> copy of the logic from read_fpsimd_context(), which is used to either
> restore an FPSIMD-only context, or to merge FPSIMD state into an
> SVE state when restoring an SVE+FPSIMD context. The logic is *almost*
> identical.
> 
> Refactor the logic to avoid duplication and make this clearer.
> 
> This comes with two functional changes that I do not believe will be
> problematic in practice:
> 
> * The user_fpsimd_state::size field will be checked in all restore paths
>   that consume it user_fpsimd_state. The kernel always populates this
>   field when delivering a signal, and so this should contain the
>   expected value unless it has been corrupted.
> 
> * If a read of user_fpsimd_state fails, we will return early without
>   modifying TIF_SVE, the saved SVCR, or the save fp_type. This will
>   leave the task in a consistent state, without potentially resurrecting
>   stale FPSIMD state. A read of user_fpsimd_state should never fail
>   unless the structure has been corrupted or the stack has been
>   unmapped.
> 
> Suggested-by: Will Deacon <will@kernel.org>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/kernel/signal.c | 57 +++++++++++++++++++-------------------
>  1 file changed, 29 insertions(+), 28 deletions(-)
> 
> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> index e898948a31b65..87890d7be8de3 100644
> --- a/arch/arm64/kernel/signal.c
> +++ b/arch/arm64/kernel/signal.c
> @@ -264,30 +264,40 @@ static int preserve_fpsimd_context(struct fpsimd_context __user *ctx)
>  	return err ? -EFAULT : 0;
>  }
>  
> -static int restore_fpsimd_context(struct user_ctxs *user)
> +static int read_fpsimd_context(struct user_fpsimd_state *fpsimd,
> +			       struct user_ctxs *user)
>  {
> -	struct user_fpsimd_state fpsimd;
> -	int err = 0;
> +	int err;
>  
>  	/* check the size information */
>  	if (user->fpsimd_size != sizeof(struct fpsimd_context))
>  		return -EINVAL;
>  
>  	/* copy the FP and status/control registers */
> -	err = __copy_from_user(fpsimd.vregs, &(user->fpsimd->vregs),
> -			       sizeof(fpsimd.vregs));
> -	__get_user_error(fpsimd.fpsr, &(user->fpsimd->fpsr), err);
> -	__get_user_error(fpsimd.fpcr, &(user->fpsimd->fpcr), err);
> +	err = __copy_from_user(fpsimd->vregs, &(user->fpsimd->vregs),
> +			       sizeof(fpsimd->vregs));
> +	__get_user_error(fpsimd->fpsr, &(user->fpsimd->fpsr), err);
> +	__get_user_error(fpsimd->fpcr, &(user->fpsimd->fpcr), err);
> +
> +	return err;

I think this should return -EFAULT if 'err' is non-zero, otherwise we
can probably propagate the number of bytes not copied from
__copy_from_user().

I'll fix that up when applying.

Cheers,

Will