Re: [PATCH v9 3/4] seccomp: add a return code to trap to userspace

On Mon, Dec 03, 2018 at 08:52:11AM -0700, Tycho Andersen wrote:

On Sun, Dec 02, 2018 at 11:26:50PM -0600, Serge E. Hallyn wrote:

quoted

On Sun, Dec 02, 2018 at 08:28:26PM -0700, Tycho Andersen wrote:

quoted

+struct seccomp_knotif {
+	/* The struct pid of the task whose filter triggered the notification */
+	struct task_struct *task;
+
+	/* The "cookie" for this request; this is unique for this filter. */
+	u64 id;
+
+	/*
+	 * The seccomp data. This pointer is valid the entire time this
+	 * notification is active, since it comes from __seccomp_filter which

define 'active' - is a notification in state REPLIED still active?

Yes,

quoted

Actually while looking at that, I came to wondering - when are knotifs
freed?  Seems like only during seccomp_notify_release(), i.e. when the
tracing task stops polling for events?  Is that going to be a problem?
Or am I misreading this?

they're stack allocated in do_user_notification(). So "active" in this
sense really means "somewhere in do_user_notification()".

D'oh!  I see, thanks :)

quoted

quoted

+	if (ret == 0 && copy_to_user(buf, &unotif, sizeof(unotif))) {
+		ret = -EFAULT;
+
+		/*
+		 * Userspace screwed up. To make sure that we keep this
+		 * notification alive, let's reset it back to INIT. It

Is keeping the notification alive the right thing to do?

If userspace has messed up in something this touchy, it seems unlikely
that it'll to better if we give it a do-over...  I'm not sure whether
killing the whole thing (victim and tracer) is the right thing or not.

I suppose we could do that too. I just didn't want to get into a
situation where the notification is lost and the task is stuck because
userspace screwed up here.

Yeah, that's probably best - I'm just trying to see whether there is
any way that this could be abused.  My underactive imagination hasn't
come up with anything yet.

-serge