Re: [PATCH] seccomp: add ptrace commands for suspend/resume
From: Oleg Nesterov <hidden>
Date: 2015-06-02 18:29:31
Also in:
lkml
On 06/01, Tycho Andersen wrote:
quoted hunk ↗ jump to hunk
--- a/include/linux/seccomp.h +++ b/include/linux/seccomp.h@@ -25,6 +25,9 @@ struct seccomp_filter; struct seccomp { int mode; struct seccomp_filter *filter; +#ifdef CONFIG_CHECKPOINT_RESTORE + bool suspended; +#endif
Then afaics you need to change copy_seccomp() to clear ->suspended. At least if the child is not traced.
quoted hunk ↗ jump to hunk
@@ -691,6 +697,11 @@ u32 seccomp_phase1(struct seccomp_data *sd) int this_syscall = sd ? sd->nr : syscall_get_nr(current, task_pt_regs(current)); +#ifdef CONFIG_CHECKPOINT_RESTORE + if (unlikely(current->seccomp.suspended)) + return SECCOMP_PHASE1_OK; +#endif +
I am wondering if PTRACE_SUSPEND_SECCOMP can just clear/set TIF_SECCOMP. Of course, it is not that resume_seccomp() can simply do set_tsk_thread_flag, it should be more careful. And prctl_set_seccomp() paths will need some changes. Probably not, this would be more complex. So perhaps it would be better to add PTRACE_O_SUSPEND_SECCOMP? This also solves the problem with the killed tracer. Except TIF_NOTSC... But why do we bother to play with TIF_NOTSC, could you explain?
+int suspend_seccomp(struct task_struct *task)
+{
+ int ret = -EACCES;
+
+ spin_lock_irq(&task->sighand->siglock);
+
+ if (!capable(CAP_SYS_ADMIN))
+ goto out;I am puzzled ;) Why do we need ->siglock? And even if we need it, why we can't check CAP_SYS_ADMIN lockless? And I am not sure I understand why do we need the additional security check, but I leave this to you and Andy. If you have the rights to trace this task, then you can do anything the tracee could do without the filtering.
+
+ task->seccomp.suspended = true;
+
+#ifdef TIF_NOTSC
+ if (task->seccomp.mode == SECCOMP_MODE_STRICT)
+ clear_tsk_thread_flag(task, TIF_NOTSC);
+#endif
+
+ ret = 0;
+out:
+ spin_unlock_irq(&task->sighand->siglock);
+
+ return ret;
+}
+
+int resume_seccomp(struct task_struct *task)
+{
+ int ret = -EACCES;
+
+ spin_lock_irq(&task->sighand->siglock);
+
+ if (!capable(CAP_SYS_ADMIN))
+ goto out;
+
+ task->seccomp.suspended = false;
+
+#ifdef TIF_NOTSC
+ if (task->seccomp.mode == SECCOMP_MODE_STRICT)
+ set_tsk_thread_flag(task, TIF_NOTSC);
+#endif
+
+ ret = 0;
+out:
+ spin_unlock_irq(&task->sighand->siglock);
+
+ return ret;
+}
+#endif /* CONFIG_CHECKPOINT_RESTORE */Well, I do not think we need 2 helpers, just one which takes a boolean will look better, imo. Oleg.