| Message ID | 20250127181424.GB1373@strace.io (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] powerpc: properly negate error in syscall_set_return_value() in sc case | expand |
Le 27/01/2025 à 19:14, Dmitry V. Levin a écrit : > Since the introduction of SECCOMP_RET_TRACE support, the kernel supports > simultaneously both the generic kernel -ERRORCODE return value ABI and > the powerpc sc syscall return ABI for PTRACE_EVENT_SECCOMP tracers. > This change is an attempt to address the code inconsistencies in syscall > error return handling that were introduced as a side effect of the dual > ABI support. > > Signed-off-by: Dmitry V. Levin <ldv@strace.io> > --- > arch/powerpc/kernel/ptrace/ptrace.c | 23 ++++++++++++++++++++--- > arch/powerpc/kernel/signal.c | 11 +++-------- > arch/powerpc/kernel/syscall.c | 6 +++--- > 3 files changed, 26 insertions(+), 14 deletions(-) > > diff --git a/arch/powerpc/kernel/ptrace/ptrace.c b/arch/powerpc/kernel/ptrace/ptrace.c > index 727ed4a14545..3778775bf6ba 100644 > --- a/arch/powerpc/kernel/ptrace/ptrace.c > +++ b/arch/powerpc/kernel/ptrace/ptrace.c > @@ -207,7 +207,7 @@ static int do_seccomp(struct pt_regs *regs) > * syscall parameter. This is different to the ptrace ABI where > * both r3 and orig_gpr3 contain the first syscall parameter. > */ > - regs->gpr[3] = -ENOSYS; > + syscall_set_return_value(current, regs, -ENOSYS, 0); > > /* > * We use the __ version here because we have already checked > @@ -215,8 +215,18 @@ static int do_seccomp(struct pt_regs *regs) > * have already loaded -ENOSYS into r3, or seccomp has put > * something else in r3 (via SECCOMP_RET_ERRNO/TRACE). > */ > - if (__secure_computing(NULL)) > + if (__secure_computing(NULL)) { > + > + /* > + * Traditionally, both the generic kernel -ERRORCODE return > + * value ABI and the powerpc sc syscall return ABI is > + * supported. For consistency, if the former is detected, > + * convert it to the latter. > + */ > + if (!trap_is_scv(regs) && IS_ERR_VALUE(regs->gpr[3])) Why !trap_is_scv(regs) ? Shouldn't this also work with scv allthough it should be a noop ? > + syscall_set_return_value(current, regs, regs->gpr[3], 0); > return -1; > + } > > /* > * The syscall was allowed by seccomp, restore the register > @@ -226,6 +236,13 @@ static int do_seccomp(struct pt_regs *regs) > * allow the syscall to proceed. > */ > regs->gpr[3] = regs->orig_gpr3; > + if (!trap_is_scv(regs)) { > + /* > + * Clear SO bit that was set in this function earlier by > + * syscall_set_return_value. > + */ > + regs->ccr &= ~0x10000000L; > + } Can't we use syscall_set_return_value() to do that ? > > return 0; > } > @@ -315,7 +332,7 @@ long do_syscall_trace_enter(struct pt_regs *regs) > * If we are aborting explicitly, or if the syscall number is > * now invalid, set the return value to -ENOSYS. > */ > - regs->gpr[3] = -ENOSYS; > + syscall_set_return_value(current, regs, -ENOSYS, 0); > return -1; > } > > diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c > index aa17e62f3754..1a38d6bcaed6 100644 > --- a/arch/powerpc/kernel/signal.c > +++ b/arch/powerpc/kernel/signal.c > @@ -19,6 +19,7 @@ > #include <asm/unistd.h> > #include <asm/debug.h> > #include <asm/tm.h> > +#include <asm/syscall.h> > > #include "signal.h" > > @@ -229,14 +230,8 @@ static void check_syscall_restart(struct pt_regs *regs, struct k_sigaction *ka, > regs_add_return_ip(regs, -4); > regs->result = 0; > } else { > - if (trap_is_scv(regs)) { > - regs->result = -EINTR; > - regs->gpr[3] = -EINTR; > - } else { > - regs->result = -EINTR; > - regs->gpr[3] = EINTR; > - regs->ccr |= 0x10000000; > - } > + regs->result = -EINTR; > + syscall_set_return_value(current, regs, -EINTR, 0); > } > } > > diff --git a/arch/powerpc/kernel/syscall.c b/arch/powerpc/kernel/syscall.c > index be159ad4b77b..2fe47191e509 100644 > --- a/arch/powerpc/kernel/syscall.c > +++ b/arch/powerpc/kernel/syscall.c > @@ -122,7 +122,7 @@ notrace long system_call_exception(struct pt_regs *regs, unsigned long r0) > if (unlikely(trap_is_unsupported_scv(regs))) { > /* Unsupported scv vector */ > _exception(SIGILL, regs, ILL_ILLOPC, regs->nip); > - return regs->gpr[3]; > + return regs_return_value(regs); > } > /* > * We use the return value of do_syscall_trace_enter() as the > @@ -133,13 +133,13 @@ notrace long system_call_exception(struct pt_regs *regs, unsigned long r0) > */ > r0 = do_syscall_trace_enter(regs); > if (unlikely(r0 >= NR_syscalls)) > - return regs->gpr[3]; > + return regs_return_value(regs); > > } else if (unlikely(r0 >= NR_syscalls)) { > if (unlikely(trap_is_unsupported_scv(regs))) { > /* Unsupported scv vector */ > _exception(SIGILL, regs, ILL_ILLOPC, regs->nip); > - return regs->gpr[3]; > + return regs_return_value(regs); > } > return -ENOSYS; > }
On Tue, Jan 28, 2025 at 07:01:47PM +0100, Christophe Leroy wrote: > Le 27/01/2025 à 19:14, Dmitry V. Levin a écrit : > > Since the introduction of SECCOMP_RET_TRACE support, the kernel supports > > simultaneously both the generic kernel -ERRORCODE return value ABI and > > the powerpc sc syscall return ABI for PTRACE_EVENT_SECCOMP tracers. > > This change is an attempt to address the code inconsistencies in syscall > > error return handling that were introduced as a side effect of the dual > > ABI support. > > > > Signed-off-by: Dmitry V. Levin <ldv@strace.io> > > --- > > arch/powerpc/kernel/ptrace/ptrace.c | 23 ++++++++++++++++++++--- > > arch/powerpc/kernel/signal.c | 11 +++-------- > > arch/powerpc/kernel/syscall.c | 6 +++--- > > 3 files changed, 26 insertions(+), 14 deletions(-) > > > > diff --git a/arch/powerpc/kernel/ptrace/ptrace.c b/arch/powerpc/kernel/ptrace/ptrace.c > > index 727ed4a14545..3778775bf6ba 100644 > > --- a/arch/powerpc/kernel/ptrace/ptrace.c > > +++ b/arch/powerpc/kernel/ptrace/ptrace.c > > @@ -207,7 +207,7 @@ static int do_seccomp(struct pt_regs *regs) > > * syscall parameter. This is different to the ptrace ABI where > > * both r3 and orig_gpr3 contain the first syscall parameter. > > */ > > - regs->gpr[3] = -ENOSYS; > > + syscall_set_return_value(current, regs, -ENOSYS, 0); > > > > /* > > * We use the __ version here because we have already checked > > @@ -215,8 +215,18 @@ static int do_seccomp(struct pt_regs *regs) > > * have already loaded -ENOSYS into r3, or seccomp has put > > * something else in r3 (via SECCOMP_RET_ERRNO/TRACE). > > */ > > - if (__secure_computing(NULL)) > > + if (__secure_computing(NULL)) { > > + > > + /* > > + * Traditionally, both the generic kernel -ERRORCODE return > > + * value ABI and the powerpc sc syscall return ABI is > > + * supported. For consistency, if the former is detected, > > + * convert it to the latter. > > + */ > > + if (!trap_is_scv(regs) && IS_ERR_VALUE(regs->gpr[3])) > > Why !trap_is_scv(regs) ? Shouldn't this also work with scv allthough it > should be a noop ? In trap_is_scv(regs) case both the source and the target ABIs are -ERRORCODE so there is no subject for conversion. > > + syscall_set_return_value(current, regs, regs->gpr[3], 0); > > return -1; > > + } > > > > /* > > * The syscall was allowed by seccomp, restore the register > > @@ -226,6 +236,13 @@ static int do_seccomp(struct pt_regs *regs) > > * allow the syscall to proceed. > > */ > > regs->gpr[3] = regs->orig_gpr3; > > + if (!trap_is_scv(regs)) { > > + /* > > + * Clear SO bit that was set in this function earlier by > > + * syscall_set_return_value. > > + */ > > + regs->ccr &= ~0x10000000L; > > + } > > Can't we use syscall_set_return_value() to do that ? Of course we could do syscall_set_return_value(current, regs, 0, regs->orig_gpr3); but Michael has objected to this already, see https://lore.kernel.org/all/87jzajjde1.fsf@mpe.ellerman.id.au/
diff --git a/arch/powerpc/kernel/ptrace/ptrace.c b/arch/powerpc/kernel/ptrace/ptrace.c index 727ed4a14545..3778775bf6ba 100644 --- a/arch/powerpc/kernel/ptrace/ptrace.c +++ b/arch/powerpc/kernel/ptrace/ptrace.c @@ -207,7 +207,7 @@ static int do_seccomp(struct pt_regs *regs) * syscall parameter. This is different to the ptrace ABI where * both r3 and orig_gpr3 contain the first syscall parameter. */ - regs->gpr[3] = -ENOSYS; + syscall_set_return_value(current, regs, -ENOSYS, 0); /* * We use the __ version here because we have already checked @@ -215,8 +215,18 @@ static int do_seccomp(struct pt_regs *regs) * have already loaded -ENOSYS into r3, or seccomp has put * something else in r3 (via SECCOMP_RET_ERRNO/TRACE). */ - if (__secure_computing(NULL)) + if (__secure_computing(NULL)) { + + /* + * Traditionally, both the generic kernel -ERRORCODE return + * value ABI and the powerpc sc syscall return ABI is + * supported. For consistency, if the former is detected, + * convert it to the latter. + */ + if (!trap_is_scv(regs) && IS_ERR_VALUE(regs->gpr[3])) + syscall_set_return_value(current, regs, regs->gpr[3], 0); return -1; + } /* * The syscall was allowed by seccomp, restore the register @@ -226,6 +236,13 @@ static int do_seccomp(struct pt_regs *regs) * allow the syscall to proceed. */ regs->gpr[3] = regs->orig_gpr3; + if (!trap_is_scv(regs)) { + /* + * Clear SO bit that was set in this function earlier by + * syscall_set_return_value. + */ + regs->ccr &= ~0x10000000L; + } return 0; } @@ -315,7 +332,7 @@ long do_syscall_trace_enter(struct pt_regs *regs) * If we are aborting explicitly, or if the syscall number is * now invalid, set the return value to -ENOSYS. */ - regs->gpr[3] = -ENOSYS; + syscall_set_return_value(current, regs, -ENOSYS, 0); return -1; } diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c index aa17e62f3754..1a38d6bcaed6 100644 --- a/arch/powerpc/kernel/signal.c +++ b/arch/powerpc/kernel/signal.c @@ -19,6 +19,7 @@ #include <asm/unistd.h> #include <asm/debug.h> #include <asm/tm.h> +#include <asm/syscall.h> #include "signal.h" @@ -229,14 +230,8 @@ static void check_syscall_restart(struct pt_regs *regs, struct k_sigaction *ka, regs_add_return_ip(regs, -4); regs->result = 0; } else { - if (trap_is_scv(regs)) { - regs->result = -EINTR; - regs->gpr[3] = -EINTR; - } else { - regs->result = -EINTR; - regs->gpr[3] = EINTR; - regs->ccr |= 0x10000000; - } + regs->result = -EINTR; + syscall_set_return_value(current, regs, -EINTR, 0); } } diff --git a/arch/powerpc/kernel/syscall.c b/arch/powerpc/kernel/syscall.c index be159ad4b77b..2fe47191e509 100644 --- a/arch/powerpc/kernel/syscall.c +++ b/arch/powerpc/kernel/syscall.c @@ -122,7 +122,7 @@ notrace long system_call_exception(struct pt_regs *regs, unsigned long r0) if (unlikely(trap_is_unsupported_scv(regs))) { /* Unsupported scv vector */ _exception(SIGILL, regs, ILL_ILLOPC, regs->nip); - return regs->gpr[3]; + return regs_return_value(regs); } /* * We use the return value of do_syscall_trace_enter() as the @@ -133,13 +133,13 @@ notrace long system_call_exception(struct pt_regs *regs, unsigned long r0) */ r0 = do_syscall_trace_enter(regs); if (unlikely(r0 >= NR_syscalls)) - return regs->gpr[3]; + return regs_return_value(regs); } else if (unlikely(r0 >= NR_syscalls)) { if (unlikely(trap_is_unsupported_scv(regs))) { /* Unsupported scv vector */ _exception(SIGILL, regs, ILL_ILLOPC, regs->nip); - return regs->gpr[3]; + return regs_return_value(regs); } return -ENOSYS; }
Since the introduction of SECCOMP_RET_TRACE support, the kernel supports simultaneously both the generic kernel -ERRORCODE return value ABI and the powerpc sc syscall return ABI for PTRACE_EVENT_SECCOMP tracers. This change is an attempt to address the code inconsistencies in syscall error return handling that were introduced as a side effect of the dual ABI support. Signed-off-by: Dmitry V. Levin <ldv@strace.io> --- arch/powerpc/kernel/ptrace/ptrace.c | 23 ++++++++++++++++++++--- arch/powerpc/kernel/signal.c | 11 +++-------- arch/powerpc/kernel/syscall.c | 6 +++--- 3 files changed, 26 insertions(+), 14 deletions(-)