Skip to content

Commit af33d24

Browse files
tych0palmer-dabbelt
authored andcommitted
riscv: fix seccomp reject syscall code path
If secure_computing() rejected a system call, we were previously setting the system call number to -1, to indicate to later code that the syscall failed. However, if something (e.g. a user notification) was sleeping, and received a signal, we may set a0 to -ERESTARTSYS and re-try the system call again. In this case, seccomp "denies" the syscall (because of the signal), and we would set a7 to -1, thus losing the value of the system call we want to restart. Instead, let's return -1 from do_syscall_trace_enter() to indicate that the syscall was rejected, so we don't clobber the value in case of -ERESTARTSYS or whatever. This commit fixes the user_notification_signal seccomp selftest on riscv to no longer hang. That test expects the system call to be re-issued after the signal, and it wasn't due to the above bug. Now that it is, everything works normally. Note that in the ptrace (tracer) case, the tracer can set the register values to whatever they want, so we still need to keep the code that handles out-of-bounds syscalls. However, we can drop the comment. We can also drop syscall_set_nr(), since it is no longer used anywhere, and the code that re-loads the value in a7 because of it. Reported in: https://lore.kernel.org/bpf/CAEn-LTp=ss0Dfv6J00=rCAy+N78U2AmhqJNjfqjr2FDpPYjxEQ@mail.gmail.com/ Reported-by: David Abdurachmanov <[email protected]> Signed-off-by: Tycho Andersen <[email protected]> Reviewed-by: Kees Cook <[email protected]> Signed-off-by: Palmer Dabbelt <[email protected]>
1 parent 0a91330 commit af33d24

File tree

3 files changed

+8
-21
lines changed

3 files changed

+8
-21
lines changed

arch/riscv/include/asm/syscall.h

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -28,13 +28,6 @@ static inline int syscall_get_nr(struct task_struct *task,
2828
return regs->a7;
2929
}
3030

31-
static inline void syscall_set_nr(struct task_struct *task,
32-
struct pt_regs *regs,
33-
int sysno)
34-
{
35-
regs->a7 = sysno;
36-
}
37-
3831
static inline void syscall_rollback(struct task_struct *task,
3932
struct pt_regs *regs)
4033
{

arch/riscv/kernel/entry.S

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -228,20 +228,13 @@ check_syscall_nr:
228228
/* Check to make sure we don't jump to a bogus syscall number. */
229229
li t0, __NR_syscalls
230230
la s0, sys_ni_syscall
231-
/*
232-
* The tracer can change syscall number to valid/invalid value.
233-
* We use syscall_set_nr helper in syscall_trace_enter thus we
234-
* cannot trust the current value in a7 and have to reload from
235-
* the current task pt_regs.
236-
*/
237-
REG_L a7, PT_A7(sp)
238231
/*
239232
* Syscall number held in a7.
240233
* If syscall number is above allowed value, redirect to ni_syscall.
241234
*/
242235
bge a7, t0, 1f
243236
/*
244-
* Check if syscall is rejected by tracer or seccomp, i.e., a7 == -1.
237+
* Check if syscall is rejected by tracer, i.e., a7 == -1.
245238
* If yes, we pretend it was executed.
246239
*/
247240
li t1, -1
@@ -334,6 +327,7 @@ work_resched:
334327
handle_syscall_trace_enter:
335328
move a0, sp
336329
call do_syscall_trace_enter
330+
move t0, a0
337331
REG_L a0, PT_A0(sp)
338332
REG_L a1, PT_A1(sp)
339333
REG_L a2, PT_A2(sp)
@@ -342,6 +336,7 @@ handle_syscall_trace_enter:
342336
REG_L a5, PT_A5(sp)
343337
REG_L a6, PT_A6(sp)
344338
REG_L a7, PT_A7(sp)
339+
bnez t0, ret_from_syscall_rejected
345340
j check_syscall_nr
346341
handle_syscall_trace_exit:
347342
move a0, sp

arch/riscv/kernel/ptrace.c

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -148,28 +148,27 @@ long arch_ptrace(struct task_struct *child, long request,
148148
* Allows PTRACE_SYSCALL to work. These are called from entry.S in
149149
* {handle,ret_from}_syscall.
150150
*/
151-
__visible void do_syscall_trace_enter(struct pt_regs *regs)
151+
__visible int do_syscall_trace_enter(struct pt_regs *regs)
152152
{
153153
if (test_thread_flag(TIF_SYSCALL_TRACE))
154154
if (tracehook_report_syscall_entry(regs))
155-
syscall_set_nr(current, regs, -1);
155+
return -1;
156156

157157
/*
158158
* Do the secure computing after ptrace; failures should be fast.
159159
* If this fails we might have return value in a0 from seccomp
160160
* (via SECCOMP_RET_ERRNO/TRACE).
161161
*/
162-
if (secure_computing() == -1) {
163-
syscall_set_nr(current, regs, -1);
164-
return;
165-
}
162+
if (secure_computing() == -1)
163+
return -1;
166164

167165
#ifdef CONFIG_HAVE_SYSCALL_TRACEPOINTS
168166
if (test_thread_flag(TIF_SYSCALL_TRACEPOINT))
169167
trace_sys_enter(regs, syscall_get_nr(current, regs));
170168
#endif
171169

172170
audit_syscall_entry(regs->a7, regs->a0, regs->a1, regs->a2, regs->a3);
171+
return 0;
173172
}
174173

175174
__visible void do_syscall_trace_exit(struct pt_regs *regs)

0 commit comments

Comments
 (0)