Re: [PATCH] MIPS: fix mips_get_syscall_arg() for o32

[Thomas Bogendoerfer <tsbogend@alpha.franken.de>]

On Wed, Feb 12, 2025 at 01:02:09AM +0200, Dmitry V. Levin wrote:
> This makes ptrace/get_syscall_info selftest pass on mips o32 and
> mips64 o32 by fixing the following two test assertions:
> 
> 1. get_syscall_info test assertion on mips o32:
>   # get_syscall_info.c:218:get_syscall_info:Expected exp_args[5] (3134521044) == info.entry.args[4] (4911432)
>   # get_syscall_info.c:219:get_syscall_info:wait #1: entry stop mismatch
> 
> 2. get_syscall_info test assertion on mips64 o32:
>   # get_syscall_info.c:209:get_syscall_info:Expected exp_args[2] (3134324433) == info.entry.args[1] (18446744072548908753)
>   # get_syscall_info.c:210:get_syscall_info:wait #1: entry stop mismatch
> 
> The first assertion happens due to mips_get_syscall_arg() trying to access
> another task's context but failing to do it properly because get_user() it
> calls just peeks at the current task's context.  It usually does not crash
> because the default user stack always gets assigned the same VMA, but it
> is pure luck which mips_get_syscall_arg() wouldn't have if e.g. the stack
> was switched (via setcontext(3) or however) or a non-default process's
> thread peeked at, and in any case irrelevant data is obtained just as
> observed with the test case.
> 
> mips_get_syscall_arg() ought to be using access_remote_vm() instead to
> retrieve the other task's stack contents, but given that the data has been
> already obtained and saved in `struct pt_regs' it would be an overkill.
> 
> The first assertion is fixed for mips o32 by using struct pt_regs.args
> instead of get_user() to obtain syscall arguments.  This approach works
> due to this piece in arch/mips/kernel/scall32-o32.S:
> 
>         /*
>          * Ok, copy the args from the luser stack to the kernel stack.
>          */
> 
>         .set    push
>         .set    noreorder
>         .set    nomacro
> 
>     load_a4: user_lw(t5, 16(t0))		# argument #5 from usp
>     load_a5: user_lw(t6, 20(t0))		# argument #6 from usp
>     load_a6: user_lw(t7, 24(t0))		# argument #7 from usp
>     load_a7: user_lw(t8, 28(t0))		# argument #8 from usp
>     loads_done:
> 
>         sw	t5, PT_ARG4(sp)		# argument #5 to ksp
>         sw	t6, PT_ARG5(sp)		# argument #6 to ksp
>         sw	t7, PT_ARG6(sp)		# argument #7 to ksp
>         sw	t8, PT_ARG7(sp)		# argument #8 to ksp
>         .set	pop
> 
>         .section __ex_table,"a"
>         PTR_WD	load_a4, bad_stack_a4
>         PTR_WD	load_a5, bad_stack_a5
>         PTR_WD	load_a6, bad_stack_a6
>         PTR_WD	load_a7, bad_stack_a7
>         .previous
> 
> arch/mips/kernel/scall64-o32.S has analogous code for mips64 o32 that
> allows fixing the issue by obtaining syscall arguments from struct
> pt_regs.regs[4..11] instead of the erroneous use of get_user().
> 
> The second assertion is fixed by truncating 64-bit values to 32-bit
> syscall arguments.
> 
> Fixes: c0ff3c53d4f9 ("MIPS: Enable HAVE_ARCH_TRACEHOOK.")
> Signed-off-by: Dmitry V. Levin <ldv@strace.io>
> ---
> 
> This started as a part of PTRACE_SET_SYSCALL_INFO API series[1].
> It has been rebased on top of [2] as suggested by Maciej in [3].
> 
> [1] https://lore.kernel.org/all/20250210113336.GA887@strace.io/
> [2] https://lore.kernel.org/all/alpine.DEB.2.21.2502101732120.65342@angie.orcam.me.uk/
> [3] https://lore.kernel.org/all/alpine.DEB.2.21.2502111530080.65342@angie.orcam.me.uk/
> 
>  arch/mips/include/asm/syscall.h | 32 ++++++++------------------------
>  1 file changed, 8 insertions(+), 24 deletions(-)

applied to mips-fixes

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]