2021-03-06 07:54:35

by Tiezhu Yang

Subject: [PATCH v2] riscv: Return -EFAULT if copy_{to,from}_user() failed in signal.c

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 <[email protected]>
---
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);
--
2.1.0

2021-03-10 03:20:48

by Palmer Dabbelt

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), [email protected] 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 <[email protected]>
> ---
> 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.