[PATCH] arm64: fix compat syscall return truncation

[PATCH] arm64: fix compat syscall return truncation

[Mark Rutland]

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 exists.

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().

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
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>
---
 arch/arm64/include/asm/ptrace.h  | 12 +++++++++++-
 arch/arm64/include/asm/syscall.h | 19 ++++++++++---------
 arch/arm64/kernel/ptrace.c       |  2 +-
 arch/arm64/kernel/signal.c       |  3 ++-
 arch/arm64/kernel/syscall.c      |  9 +++------
 5 files changed, 27 insertions(+), 18 deletions(-)

This patch addresses the issue reported by both He Zhe [1] and weiyuchen [2].
This was originally posted a reply, without a commit message [3].

I've tried to keep this entirely local to arm64 so that we can backport it; we
should subsequently try to cleanup audit as He Zhe was attempting in [1].

Thanks.
Mark.

[1] https://lore.kernel.org/r/20210416075533.7720-1-zhe.he@windriver.com
[2] https://lore.kernel.org/r/20210722060707.531-1-weiyuchen3@huawei.com
[3] https://lore.kernel.org/r/20210730142336.GA19569@C02TD0UTHF1T.local

diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index e58bca832dff..41b332c054ab 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h
@@ -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)
diff --git a/arch/arm64/include/asm/syscall.h b/arch/arm64/include/asm/syscall.h
index cfc0672013f6..03e20895453a 100644
--- a/arch/arm64/include/asm/syscall.h
+++ b/arch/arm64/include/asm/syscall.h
@@ -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,
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 499b6b2f9757..b381a1ee9ea7 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -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);
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index f8192f4ae0b8..23036334f4dc 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -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>
@@ -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;
 		}
 
diff --git a/arch/arm64/kernel/syscall.c b/arch/arm64/kernel/syscall.c
index 263d6c1a525f..50a0f1a38e84 100644
--- a/arch/arm64/kernel/syscall.c
+++ b/arch/arm64/kernel/syscall.c
@@ -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(),
@@ -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;
 	}
 
@@ -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;
-- 
2.11.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: fix compat syscall return truncation

[Catalin Marinas]

On Mon, Aug 02, 2021 at 11:42:00AM +0100, Mark Rutland wrote:
> 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 exists.
> 
> 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().
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> 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>

(to the best of my knowledge, I can't guarantee all cases have been
covered)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: fix compat syscall return truncation

[Will Deacon]

On Mon, 2 Aug 2021 11:42:00 +0100, Mark Rutland wrote:
> 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.
> 
> [...]

Applied to arm64 (for-next/fixes), thanks!

[1/1] arm64: fix compat syscall return truncation
      https://git.kernel.org/arm64/c/e30e8d46cf60

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel