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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20181030150254.GB3385@redhat.com>
Date:   Tue, 30 Oct 2018 16:02:54 +0100
From:   Oleg Nesterov <oleg@...hat.com>
To:     Tycho Andersen <tycho@...ho.ws>
Cc:     Kees Cook <keescook@...omium.org>,
        Andy Lutomirski <luto@...capital.net>,
        "Eric W . Biederman" <ebiederm@...ssion.com>,
        "Serge E . Hallyn" <serge@...lyn.com>,
        Christian Brauner <christian@...uner.io>,
        Tyler Hicks <tyhicks@...onical.com>,
        Akihiro Suda <suda.akihiro@....ntt.co.jp>,
        Aleksa Sarai <asarai@...e.de>, linux-kernel@...r.kernel.org,
        containers@...ts.linux-foundation.org, linux-api@...r.kernel.org
Subject: Re: [PATCH v8 1/2] seccomp: add a return code to trap to userspace

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.