Re: [PATCH 3/6] syscall.h: introduce syscall_set_nr()

On Fri, Jan 10, 2025 at 08:37:46AM +0100, Sven Schnelle wrote:
> "Dmitry V. Levin" <[email protected]> writes:
> 
> > Similar to syscall_set_arguments() that complements
> > syscall_get_arguments(), introduce syscall_set_nr()
> > that complements syscall_get_nr().
> >
> > syscall_set_nr() is going to be needed along with
> > syscall_set_arguments() on all HAVE_ARCH_TRACEHOOK
> > architectures to implement PTRACE_SET_SYSCALL_INFO API.
[...]
> > diff --git a/arch/s390/include/asm/syscall.h b/arch/s390/include/asm/syscall.h
> > index b3dd883699e7..1c0e349fd5c9 100644
> > --- a/arch/s390/include/asm/syscall.h
> > +++ b/arch/s390/include/asm/syscall.h
> > @@ -24,6 +24,13 @@ static inline long syscall_get_nr(struct task_struct *task,
> >  		(regs->int_code & 0xffff) : -1;
> >  }
> >  
> > +static inline void syscall_set_nr(struct task_struct *task,
> > +				  struct pt_regs *regs,
> > +				  int nr)
> > +{
> 
> I think there should be a
> 
> 	if (!test_pt_regs_flags(regs, PIF_SYSCALL))
> 		return;
> 
> before the modification so a user can't accidentally change int_code
> when ptrace stopped in a non-syscall path.

The reason why syscall_get_nr() has this check on s390 (and similar checks
on arc, powerpc, and sparc) is that syscall_get_nr() can be called while
the target task is not in syscall.

Unlike syscall_get_nr(), syscall_set_nr() can be called only when the
target task is stopped for tracing on entering syscall: the description in
include/asm-generic/syscall.h explicitly states that, and the follow-up
patch that introduces PTRACE_SET_SYSCALL_INFO adds a syscall_set_nr() call
when the tracee is stopped on entering syscall in either
PTRACE_SYSCALL_INFO_ENTRY or PTRACE_SYSCALL_INFO_SECCOMP state.

I don't mind adding a check, but syscall_set_nr() invocation while the
target task is not in syscall wouldn't be a result of user actions but
a kernel programing error, and in that case WARN_ON_ONCE() would be more
appropriate.

If calling syscall_set_nr() while the target task is not in syscall was
legal, then syscall_set_nr() would have been designed to return a value
indicating the status of operation.

Anyway, I'll add an explanatory comment to syscall_set_nr() on all
architectures where syscall_get_nr() has a check.


-- 
ldv