| Message ID | 20250210113442.GB887@strace.io (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ptrace: introduce PTRACE_SET_SYSCALL_INFO API | expand |
On Mon, 10 Feb 2025, Dmitry V. Levin wrote: > The first assertion is fixed for mips o32 by using struct pt_regs.pad0 > instead of get_user() to obtain syscall arguments. This approach works > due to this piece in arch/mips/kernel/scall32-o32.S: I've had a look now and I can see what's going on here. The thing is we're trying to access another task's context and obviously dereferencing $sp obtained from it is not going to work via get_user(), because that just peeks at the current task's context. It often does not crash, because the default user stack always gets assigned the same VMA, but it is pure luck which we wouldn't have if the stack was switched (via setcontext(3) or however) or say a non-default process's thread peeked at, and in any case irrelevant data is obtained just as observed with the test case. We ought to be using access_remote_vm() 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. So I think your change is actually the correct thing to do, but please let's not abuse a struct member called `pad', the name of which indicates its contents are not supposed to be of any use. I have therefore posted a preparatory cleanup[1]. May you please rebase your patch on top of that and also update the change description so as to reflect the findings? Thomas, can you please apply my cleanup soon and ahead of Dmitry's change so as to make things easy to proceed with? Or otherwise let me know what works for you best. Also I have a suspicion this stuff ought to be backported, but I guess it can be decided later on. Thank you for your patience. [1] "MIPS: Export syscall stack arguments properly for remote use", <https://lore.kernel.org/linux-mips/alpine.DEB.2.21.2502101732120.65342@angie.orcam.me.uk/> Maciej
On Tue, Feb 11, 2025 at 06:30:33PM +0000, Maciej W. Rozycki wrote: > On Mon, 10 Feb 2025, Dmitry V. Levin wrote: > > > The first assertion is fixed for mips o32 by using struct pt_regs.pad0 > > instead of get_user() to obtain syscall arguments. This approach works > > due to this piece in arch/mips/kernel/scall32-o32.S: > > I've had a look now and I can see what's going on here. > > The thing is we're trying to access another task's context and obviously > dereferencing $sp obtained from it is not going to work via get_user(), > because that just peeks at the current task's context. It often does not > crash, because the default user stack always gets assigned the same VMA, > but it is pure luck which we wouldn't have if the stack was switched (via > setcontext(3) or however) or say a non-default process's thread peeked at, > and in any case irrelevant data is obtained just as observed with the test > case. > > We ought to be using access_remote_vm() 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. > > So I think your change is actually the correct thing to do, but please > let's not abuse a struct member called `pad', the name of which indicates > its contents are not supposed to be of any use. I have therefore posted a > preparatory cleanup[1]. May you please rebase your patch on top of that > and also update the change description so as to reflect the findings? > > Thomas, can you please apply my cleanup soon and ahead of Dmitry's change > so as to make things easy to proceed with? Or otherwise let me know what > works for you best. > > Also I have a suspicion this stuff ought to be backported, but I guess it > can be decided later on. > > Thank you for your patience. > > [1] "MIPS: Export syscall stack arguments properly for remote use", > <https://lore.kernel.org/linux-mips/alpine.DEB.2.21.2502101732120.65342@angie.orcam.me.uk/> Thanks for the analysis, I'm going to rebase my fix and send it as a follow-up to your cleanup patch.
diff --git a/arch/mips/include/asm/syscall.h b/arch/mips/include/asm/syscall.h index ebdf4d910af2..b3f00ede8bb3 100644 --- a/arch/mips/include/asm/syscall.h +++ b/arch/mips/include/asm/syscall.h @@ -57,37 +57,21 @@ static inline void mips_syscall_update_nr(struct task_struct *task, static inline void mips_get_syscall_arg(unsigned long *arg, struct task_struct *task, struct pt_regs *regs, unsigned int n) { - unsigned long usp __maybe_unused = regs->regs[29]; - +#ifdef CONFIG_32BIT switch (n) { case 0: case 1: case 2: case 3: *arg = regs->regs[4 + n]; - - return; - -#ifdef CONFIG_32BIT - case 4: case 5: case 6: case 7: - get_user(*arg, (int *)usp + n); return; -#endif - -#ifdef CONFIG_64BIT case 4: case 5: case 6: case 7: -#ifdef CONFIG_MIPS32_O32 - if (test_tsk_thread_flag(task, TIF_32BIT_REGS)) - get_user(*arg, (int *)usp + n); - else -#endif - *arg = regs->regs[4 + n]; - + *arg = regs->pad0[n]; return; -#endif - - default: - BUG(); } - - unreachable(); +#else + *arg = regs->regs[4 + n]; + if ((IS_ENABLED(CONFIG_MIPS32_O32) && + test_tsk_thread_flag(task, TIF_32BIT_REGS))) + *arg = (unsigned int)*arg; +#endif } static inline long syscall_get_error(struct task_struct *task,
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 is fixed for mips o32 by using struct pt_regs.pad0 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, 16(sp) # argument #5 to ksp sw t6, 20(sp) # argument #6 to ksp sw t7, 24(sp) # argument #7 to ksp sw t8, 28(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 obtaining syscall arguments from struct pt_regs.regs[4..11] instead of get_user(). The second assertion is fixed by truncating 64-bit values to 32-bit syscall arguments. Signed-off-by: Dmitry V. Levin <ldv@strace.io> --- arch/mips/include/asm/syscall.h | 32 ++++++++------------------------ 1 file changed, 8 insertions(+), 24 deletions(-)