Re: [PATCH v8 1/2] seccomp: add a return code to trap to userspace

[Tycho Andersen]

On Tue, Oct 30, 2018 at 04:02:54PM +0100, Oleg Nesterov wrote:
> On 10/29, Tycho Andersen wrote:
> >
> > +static long seccomp_notify_recv(struct seccomp_filter *filter,
> > +				void __user *buf)
> > +{
> > +	struct seccomp_knotif *knotif = NULL, *cur;
> > +	struct seccomp_notif unotif;
> > +	ssize_t ret;
> > +
> > +	memset(&unotif, 0, sizeof(unotif));
> > +
> > +	ret = down_interruptible(&filter->notif->request);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	mutex_lock(&filter->notify_lock);
> > +	list_for_each_entry(cur, &filter->notif->notifications, list) {
> > +		if (cur->state == SECCOMP_NOTIFY_INIT) {
> > +			knotif = cur;
> > +			break;
> > +		}
> > +	}
> > +
> > +	/*
> > +	 * If we didn't find a notification, it could be that the task was
> > +	 * interrupted by a fatal signal between the time we were woken and
> > +	 * when we were able to acquire the rw lock.
> > +	 *
> > +	 * This is the place where we handle the extra high semaphore count
> > +	 * mentioned in seccomp_do_user_notification().
> > +	 */
> > +	if (!knotif) {
> > +		ret = -ENOENT;
> > +		goto out;
> > +	}
> > +
> > +	unotif.id = knotif->id;
> > +	unotif.pid = task_pid_vnr(knotif->task);
> > +	if (knotif->signaled)
> > +		unotif.flags |= SECCOMP_NOTIF_FLAG_SIGNALED;
> > +	unotif.data = *(knotif->data);
> 
> Tycho, I forgot everything about seccomp, most probably I am wrong but let me
> ask anyway.
> 
> __seccomp_filter(SECCOMP_RET_TRACE) does
> 
> 		/*
> 		 * Recheck the syscall, since it may have changed. This
> 		 * intentionally uses a NULL struct seccomp_data to force
> 		 * a reload of all registers. This does not goto skip since
> 		 * a skip would have already been reported.
> 		 */
> 		if (__seccomp_filter(this_syscall, NULL, true))
> 			return -1;
> 
> and the next seccomp_run_filters() can return SECCOMP_RET_USER_NOTIF, right?
> seccomp_do_user_notification() doesn't check recheck_after_trace and it simply
> does n.data = sd.
> 
> Doesn't this mean that "unotif.data = *(knotif->data)" can hit NULL ?
> 
> seccomp_run_filters() does populate_seccomp_data() in this case, but this
> won't affect "seccomp_data *sd" passed to seccomp_do_user_notification().

Oof, yes, you're right. Seems like there are no other users of sd in
__seccomp_filter(). Seems to me like we can just do the
populate_seccomp_data() one level higher in __seccomp_filter()?

Tycho