On 04/16, He Zhe wrote: I can't really review these arm-specific patches, but with this change both syscall_get_error() and is_syscall_success() can use syscall_get_return_value() to avoid the code duplication. Oleg.

On Fri, Apr 16, 2021 at 03:55:32PM +0800, He Zhe wrote: If a compat syscall can ever legitimately return a non-error value with bit 31 set, and this sign-extends it, is that ever going to reach userspace as a 64-bit value? IIUC things like mmap() can return pointers above 2GiB for a compat task, so I'm a bit uneasy that we'd handle those wrong. I can't see a way of preventing that unless we keep the upper 32 bits for errors. Mark.

regs_return_value for some architectures like arm64 simply retrieve register value from pt_regs without sign extension in 32-bit compatible case and cause audit to have false syscall return code. For example, 32-bit -13 would be treated as 4294967283 below. type=SYSCALL msg=audit(1611110715.887:582): arch=40000028 syscall=322 success=yes exit=4294967283 We just added proper sign extension in syscall_get_return_value which should be used instead. Signed-off-by: He Zhe <zhe.he(a)windriver.com&gt; --- include/linux/audit.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/linux/audit.h b/include/linux/audit.h index 82b7c1116a85..135adbe22c19 100644 --- a/include/linux/audit.h +++ b/include/linux/audit.h @@ -334,7 +334,7 @@ static inline void audit_syscall_exit(void *pt_regs) { if (unlikely(audit_context())) { int success = is_syscall_success(pt_regs); - long return_code = regs_return_value(pt_regs); + long return_code = syscall_get_return_value(current, pt_regs); __audit_syscall_exit(success, return_code); } -- 2.17.1

On Fri, Apr 16, 2021 at 01:33:22PM +0100, Catalin Marinas wrote: I think this is a problem we created for ourselves back in commit: 15956689a0e60aa0 ("arm64: compat: Ensure upper 32 bits of x0 are zero on syscall return) AFAICT, the perf regs samples are the only place this matters, since for ptrace the compat regs are implicitly truncated to compat_ulong_t, and audit expects the non-truncated return value. Other architectures don't truncate here, so I think we're setting ourselves up for a game of whack-a-mole to truncate and extend wherever we need to. Given that, I suspect it'd be better to do something like the below. Will, thoughts? Mark. ---->8---- From: Mark Rutland <mark.rutland(a)arm.com&gt; Date: Fri, 16 Apr 2021 13:58:54 +0100 Subject: [PATCH] arm64: perf: truncate compat regs For compat userspace, it doesn't generally make sense for the upper 32 bits of GPRs to be set, as these bits don't really exist in AArch32. However, for structural reasons the kernel may transiently set the upper 32 bits of registers in pt_regs at points where a perf sample can be taken. We tried to avoid this happening in commit: 15956689a0e60aa0 ("arm64: compat: Ensure upper 32 bits of x0 are zero on syscall return") ... by having invoke_syscall() truncate the return value for compat tasks, with helpers in <asm/syscall.h> extending the return value when required. Unfortunately this is not complete, as there are other places where we assign the return value, such as when el0_svc_common() sets up a return of -ENOSYS. Further, this approach breaks the audit code, which relies on the upper 32 bits of the return value. Instead, let's have the perf code explicitly truncate the user regs to 32 bits, and otherwise preserve those within the kernel. Fixes: 15956689a0e60aa0 ("arm64: compat: Ensure upper 32 bits of x0 are zero on syscall return") Signed-off-by: Mark Rutland <mark.rutland(a)arm.com&gt; Cc: Will Deacon <will(a)kernel.org&gt; --- arch/arm64/include/asm/syscall.h | 11 +---------- arch/arm64/kernel/perf_regs.c | 26 ++++++++++++++++---------- arch/arm64/kernel/syscall.c | 3 --- 3 files changed, 17 insertions(+), 23 deletions(-) diff --git a/arch/arm64/include/asm/syscall.h b/arch/arm64/include/asm/syscall.h index cfc0672013f6..0ebeaf6dbd45 100644 --- a/arch/arm64/include/asm/syscall.h +++ b/arch/arm64/include/asm/syscall.h @@ -35,9 +35,6 @@ static inline long syscall_get_error(struct task_struct *task, { unsigned long error = regs->regs[0]; - if (is_compat_thread(task_thread_info(task))) - error = sign_extend64(error, 31); - return IS_ERR_VALUE(error) ? error : 0; } @@ -51,13 +48,7 @@ static inline void syscall_set_return_value(struct task_struct *task, struct pt_regs *regs, int error, long val) { - if (error) - val = error; - - if (is_compat_thread(task_thread_info(task))) - val = lower_32_bits(val); - - regs->regs[0] = val; + regs->regs[0] = (long) error ? error : val; } #define SYSCALL_MAX_ARGS 6 diff --git a/arch/arm64/kernel/perf_regs.c b/arch/arm64/kernel/perf_regs.c index f6f58e6265df..296f0c55b4e2 100644 --- a/arch/arm64/kernel/perf_regs.c +++ b/arch/arm64/kernel/perf_regs.c @@ -9,6 +9,17 @@ #include <asm/perf_regs.h> #include <asm/ptrace.h> +static u64 __perf_reg_value(struct pt_regs *regs, int idx) +{ + if ((u32)idx == PERF_REG_ARM64_SP) + return regs->sp; + + if ((u32)idx == PERF_REG_ARM64_PC) + return regs->pc; + + return regs->regs[idx]; +} + u64 perf_reg_value(struct pt_regs *regs, int idx) { if (WARN_ON_ONCE((u32)idx >= PERF_REG_ARM64_MAX)) @@ -38,20 +49,15 @@ u64 perf_reg_value(struct pt_regs *regs, int idx) */ if (compat_user_mode(regs)) { if ((u32)idx == PERF_REG_ARM64_SP) - return regs->compat_sp; + return lower_32_bits(regs->compat_sp); if ((u32)idx == PERF_REG_ARM64_LR) - return regs->compat_lr; + return lower_32_bits(regs->compat_lr); if (idx == 15) - return regs->pc; + return lower_32_bits(regs->pc); + return lower_32_bits(__perf_reg_value(regs, idx)); } - if ((u32)idx == PERF_REG_ARM64_SP) - return regs->sp; - - if ((u32)idx == PERF_REG_ARM64_PC) - return regs->pc; - - return regs->regs[idx]; + return __perf_reg_value(regs, idx); } #define REG_RESERVED (~((1ULL << PERF_REG_ARM64_MAX) - 1)) diff --git a/arch/arm64/kernel/syscall.c b/arch/arm64/kernel/syscall.c index b9cf12b271d7..84314fae4b5c 100644 --- a/arch/arm64/kernel/syscall.c +++ b/arch/arm64/kernel/syscall.c @@ -51,9 +51,6 @@ 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; } -- 2.11.0

On Fri, Apr 16, 2021 at 02:34:41PM +0100, Mark Rutland wrote: I think perf is one example, but this is also visible to userspace via the native ptrace interface and I distinctly remember needing this for some versions of arm64 strace to work correctly when tracing compat tasks. So I do think that clearing the upper bits on the return path is the right approach, but it sounds like we need some more work to handle syscall(-1) and audit (what exactly is the problem here after these patches have been applied?) Will

On 4/16/21 8:33 PM, Catalin Marinas wrote: Thanks. I'll use this for v2. IMHO, now that we have had specific function, syscall_get_return_value, to get syscall return code, we might as well use it. regs_return_value may be left for where we want internal return code. I found such places below and haven't found other places that syscall sign extension is concerned about. kernel/test_kprobes.c kernel/trace/trace_kprobe.c samples/kprobes/kretprobe_example.c Regards, Zhe