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

Oleg Nesterov
2018-10-30T15:03:00Z

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().

Oleg.