linux-kernel - Re: [PATCH v2] riscv: Return -EFAULT if copy_{to,from}

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] [day] [month] [year] [list]

Message-ID: <mhng-33c9663e-90b5-48a8-b089-54f16f6b494a@penguin>
Date:   Tue, 09 Mar 2021 19:16:45 -0800 (PST)
From:   Palmer Dabbelt <palmer@...belt.com>
To:     yangtiezhu@...ngson.cn
CC:     Paul Walmsley <paul.walmsley@...ive.com>, aou@...s.berkeley.edu,
        linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject:     Re: [PATCH v2] riscv: Return -EFAULT if copy_{to,from}_user() failed in signal.c

On Fri, 05 Mar 2021 23:52:29 PST (-0800), yangtiezhu@...ngson.cn wrote:
> copy_{to,from}_user() returns the amount left to copy, it should return
> -EFAULT error code if copy {to,from} user failed, just like the return
> value is an error code when {put,get}_user() failed, this is to make the
> return value consistent, no function change.
>
> Signed-off-by: Tiezhu Yang <yangtiezhu@...ngson.cn>
> ---
>  arch/riscv/kernel/signal.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/arch/riscv/kernel/signal.c b/arch/riscv/kernel/signal.c
> index 65942b3..c76d877 100644
> --- a/arch/riscv/kernel/signal.c
> +++ b/arch/riscv/kernel/signal.c
> @@ -39,7 +39,7 @@ static long restore_fp_state(struct pt_regs *regs,
>
>  	err = __copy_from_user(&current->thread.fstate, state, sizeof(*state));
>  	if (unlikely(err))
> -		return err;
> +		return -EFAULT;
>
>  	fstate_restore(current, regs);
>
> @@ -67,7 +67,7 @@ static long save_fp_state(struct pt_regs *regs,
>  	fstate_save(current, regs);
>  	err = __copy_to_user(state, &current->thread.fstate, sizeof(*state));
>  	if (unlikely(err))
> -		return err;
> +		return -EFAULT;
>
>  	/* We support no other extension state at this time. */
>  	for (i = 0; i < ARRAY_SIZE(sc_fpregs->q.reserved); i++) {
> @@ -87,8 +87,12 @@ static long restore_sigcontext(struct pt_regs *regs,
>  	struct sigcontext __user *sc)
>  {
>  	long err;
> +
>  	/* sc_regs is structured the same as the start of pt_regs */
>  	err = __copy_from_user(regs, &sc->sc_regs, sizeof(sc->sc_regs));
> +	if (unlikely(err))
> +		return -EFAULT;
> +
>  	/* Restore the floating-point state. */
>  	if (has_fpu)
>  		err |= restore_fp_state(regs, &sc->sc_fpregs);
> @@ -140,8 +144,12 @@ static long setup_sigcontext(struct rt_sigframe __user *frame,
>  {
>  	struct sigcontext __user *sc = &frame->uc.uc_mcontext;
>  	long err;
> +
>  	/* sc_regs is structured the same as the start of pt_regs */
>  	err = __copy_to_user(&sc->sc_regs, regs, sizeof(sc->sc_regs));
> +	if (unlikely(err))
> +		return -EFAULT;
> +
>  	/* Save the floating-point state. */
>  	if (has_fpu)
>  		err |= save_fp_state(regs, &sc->sc_fpregs);

I don't really see any benefit to this way of doing it over what's there: these 
are only used within this file, and the caller is just doing this return 
conversion already.  If anything I find the current code easier to understand, 
as error juggling is always one of the trickier things to get right and I 
always find it easier to reason about code that's just passing through errors.

If you have some new user of this code where it makes more sense to do it this 
way then I'd be happy to take a look, but this as it stands doesn't really look 
better.