Re: [PATCH v6 2/6] syscall.h: add syscall_set_arguments()

["Dmitry V. Levin" <ldv@strace.io>]

On Wed, Feb 19, 2025 at 05:15:48PM +0000, Maciej W. Rozycki wrote:
> On Mon, 17 Feb 2025, Dmitry V. Levin wrote:
[...]
> > diff --git a/arch/csky/include/asm/syscall.h b/arch/csky/include/asm/syscall.h
> > index 0de5734950bf..30403f7a0487 100644
> > --- a/arch/csky/include/asm/syscall.h
> > +++ b/arch/csky/include/asm/syscall.h
> > @@ -59,6 +59,19 @@ syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
> >  	memcpy(args, &regs->a1, 5 * sizeof(args[0]));
> >  }
> >  
> > +static inline void
> > +syscall_set_arguments(struct task_struct *task, struct pt_regs *regs,
> > +		      const unsigned long *args)
> > +{
> > +	memcpy(&regs->a0, args, 6 * sizeof(regs->a0));
> > +	/*
> > +	 * Also copy the first argument into orig_x0
>                                                   ^
>  Typo here, s/orig_x0/orig_a0/; see below.
> 
> > +	 * so that syscall_get_arguments() would return it
> > +	 * instead of the previous value.
> > +	 */
> > +	regs->orig_a0 = regs->a0;

Indeed, thanks for spotting this.

>  Also:
> 
> > diff --git a/arch/mips/include/asm/syscall.h b/arch/mips/include/asm/syscall.h
> > index 056aa1b713e2..ea050b23d428 100644
> > --- a/arch/mips/include/asm/syscall.h
> > +++ b/arch/mips/include/asm/syscall.h
> > @@ -120,6 +137,21 @@ static inline void syscall_get_arguments(struct task_struct *task,
> >  		mips_get_syscall_arg(args++, task, regs, i++);
> >  }
> >  
> > +static inline void syscall_set_arguments(struct task_struct *task,
> > +					 struct pt_regs *regs,
> > +					 unsigned long *args)
> > +{
> > +	unsigned int i = 0;
> > +	unsigned int n = 6;
> > +
> > +	/* O32 ABI syscall() */
> > +	if (mips_syscall_is_indirect(task, regs))
> > +		i++;
> 
> -- given MIPS syscall_set_nr() implementation in 3/6 this conditional is 
> supposed to never be true.  Should it be BUG_ON() or discarded entirely?

I agree it should be discarded: given that the syscall number read from
regs[2] after syscall_trace_enter() invocation is not treated in any
special way with regards to __NR_syscall, it would be incorrect to do
it here either.  In fact, user space is allowed to set regs[2] to
__NR_syscall, even though it's pointless, but it's definitely not a
BUG_ON() situation.

I'll remove this check, thanks for the review!


-- 
ldv