Skip to content

Commit

Permalink
arm64: fix compat syscall return truncation
Browse files Browse the repository at this point in the history
commit e30e8d4 upstream.

Due to inconsistencies in the way we manipulate compat GPRs, we have a
few issues today:

* For audit and tracing, where error codes are handled as a (native)
  long, negative error codes are expected to be sign-extended to the
  native 64-bits, or they may fail to be matched correctly. Thus a
  syscall which fails with an error may erroneously be identified as
  failing.

* For ptrace, *all* compat return values should be sign-extended for
  consistency with 32-bit arm, but we currently only do this for
  negative return codes.

* As we may transiently set the upper 32 bits of some compat GPRs while
  in the kernel, these can be sampled by perf, which is somewhat
  confusing. This means that where a syscall returns a pointer above 2G,
  this will be sign-extended, but will not be mistaken for an error as
  error codes are constrained to the inclusive range [-4096, -1] where
  no user pointer can exist.

To fix all of these, we must consistently use helpers to get/set the
compat GPRs, ensuring that we never write the upper 32 bits of the
return code, and always sign-extend when reading the return code.  This
patch does so, with the following changes:

* We re-organise syscall_get_return_value() to always sign-extend for
  compat tasks, and reimplement syscall_get_error() atop. We update
  syscall_trace_exit() to use syscall_get_return_value().

* We consistently use syscall_set_return_value() to set the return
  value, ensureing the upper 32 bits are never set unexpectedly.

* As the core audit code currently uses regs_return_value() rather than
  syscall_get_return_value(), we special-case this for
  compat_user_mode(regs) such that this will do the right thing. Going
  forward, we should try to move the core audit code over to
  syscall_get_return_value().

Cc: <stable@vger.kernel.org>
Reported-by: He Zhe <zhe.he@windriver.com>
Reported-by: weiyuchen <weiyuchen3@huawei.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Link: https://lore.kernel.org/r/20210802104200.21390-1-mark.rutland@arm.com
Signed-off-by: Will Deacon <will@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
  • Loading branch information
Mark Rutland authored and gregkh committed Aug 12, 2021
1 parent a62784c commit b5f6bab
Show file tree
Hide file tree
Showing 5 changed files with 27 additions and 18 deletions.
12 changes: 11 additions & 1 deletion arch/arm64/include/asm/ptrace.h
Expand Up @@ -320,7 +320,17 @@ static inline unsigned long kernel_stack_pointer(struct pt_regs *regs)

static inline unsigned long regs_return_value(struct pt_regs *regs)
{
return regs->regs[0];
unsigned long val = regs->regs[0];

/*
* Audit currently uses regs_return_value() instead of
* syscall_get_return_value(). Apply the same sign-extension here until
* audit is updated to use syscall_get_return_value().
*/
if (compat_user_mode(regs))
val = sign_extend64(val, 31);

return val;
}

static inline void regs_set_return_value(struct pt_regs *regs, unsigned long rc)
Expand Down
19 changes: 10 additions & 9 deletions arch/arm64/include/asm/syscall.h
Expand Up @@ -29,22 +29,23 @@ static inline void syscall_rollback(struct task_struct *task,
regs->regs[0] = regs->orig_x0;
}


static inline long syscall_get_error(struct task_struct *task,
struct pt_regs *regs)
static inline long syscall_get_return_value(struct task_struct *task,
struct pt_regs *regs)
{
unsigned long error = regs->regs[0];
unsigned long val = regs->regs[0];

if (is_compat_thread(task_thread_info(task)))
error = sign_extend64(error, 31);
val = sign_extend64(val, 31);

return IS_ERR_VALUE(error) ? error : 0;
return val;
}

static inline long syscall_get_return_value(struct task_struct *task,
struct pt_regs *regs)
static inline long syscall_get_error(struct task_struct *task,
struct pt_regs *regs)
{
return regs->regs[0];
unsigned long error = syscall_get_return_value(task, regs);

return IS_ERR_VALUE(error) ? error : 0;
}

static inline void syscall_set_return_value(struct task_struct *task,
Expand Down
2 changes: 1 addition & 1 deletion arch/arm64/kernel/ptrace.c
Expand Up @@ -1862,7 +1862,7 @@ void syscall_trace_exit(struct pt_regs *regs)
audit_syscall_exit(regs);

if (flags & _TIF_SYSCALL_TRACEPOINT)
trace_sys_exit(regs, regs_return_value(regs));
trace_sys_exit(regs, syscall_get_return_value(current, regs));

if (flags & (_TIF_SYSCALL_TRACE | _TIF_SINGLESTEP))
tracehook_report_syscall(regs, PTRACE_SYSCALL_EXIT);
Expand Down
3 changes: 2 additions & 1 deletion arch/arm64/kernel/signal.c
Expand Up @@ -29,6 +29,7 @@
#include <asm/unistd.h>
#include <asm/fpsimd.h>
#include <asm/ptrace.h>
#include <asm/syscall.h>
#include <asm/signal32.h>
#include <asm/traps.h>
#include <asm/vdso.h>
Expand Down Expand Up @@ -890,7 +891,7 @@ static void do_signal(struct pt_regs *regs)
retval == -ERESTART_RESTARTBLOCK ||
(retval == -ERESTARTSYS &&
!(ksig.ka.sa.sa_flags & SA_RESTART)))) {
regs->regs[0] = -EINTR;
syscall_set_return_value(current, regs, -EINTR, 0);
regs->pc = continue_addr;
}

Expand Down
9 changes: 3 additions & 6 deletions arch/arm64/kernel/syscall.c
Expand Up @@ -54,10 +54,7 @@ static void invoke_syscall(struct pt_regs *regs, unsigned int scno,
ret = do_ni_syscall(regs, scno);
}

if (is_compat_task())
ret = lower_32_bits(ret);

regs->regs[0] = ret;
syscall_set_return_value(current, regs, 0, ret);

/*
* Ultimately, this value will get limited by KSTACK_OFFSET_MAX(),
Expand Down Expand Up @@ -115,7 +112,7 @@ static void el0_svc_common(struct pt_regs *regs, int scno, int sc_nr,
* syscall. do_notify_resume() will send a signal to userspace
* before the syscall is restarted.
*/
regs->regs[0] = -ERESTARTNOINTR;
syscall_set_return_value(current, regs, -ERESTARTNOINTR, 0);
return;
}

Expand All @@ -136,7 +133,7 @@ static void el0_svc_common(struct pt_regs *regs, int scno, int sc_nr,
* anyway.
*/
if (scno == NO_SYSCALL)
regs->regs[0] = -ENOSYS;
syscall_set_return_value(current, regs, -ENOSYS, 0);
scno = syscall_trace_enter(regs);
if (scno == NO_SYSCALL)
goto trace_exit;
Expand Down

0 comments on commit b5f6bab

Please sign in to comment.