mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Kees Cook <kees@kernel.org>
To: Hui Peng <benquike@gmail.com>
Cc: Andy Lutomirski <luto@amacapital.net>,
	Will Drewry <wad@chromium.org>, Shuah Khan <shuah@kernel.org>,
	Bradley Morgan <brads@mainlining.org>,
	Lorenzo Stoakes <ljs@kernel.org>,
	linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
	stable@vger.kernel.org
Subject: Re: [PATCH v4] seccomp: restore knotif->state when SECCOMP_ADDFD_FLAG_SEND is interrupted
Date: Mon, 21 Sep 2026 02:19:38 -0700	[thread overview]
Message-ID: <202609210158.F3D96F157@keescook> (raw)
In-Reply-To: <20260920185910.3307638-1-benquike@gmail.com>

On Sun, Sep 20, 2026 at 06:59:10PM +0000, Hui Peng wrote:
> Fix this by restoring knotif->state back to SECCOMP_NOTIFY_SENT if
> SECCOMP_ADDFD_FLAG_SEND was set and kaddfd was not consumed before the
> interrupted wait. Also add a seccomp_bpf selftest
> (user_notification_addfd_send_interrupted) covering this race.

Yeah, this fix is the same restoration logic as seccomp_notify_recv()
uses (although that may actually need "if (state == SECCOMP_NOTIFY_SENT) ..."
added). It does make it clear there is a missing state in the state
machine. SECCOMP_NOTIFY_REPLIES means both "A reply is reserved" and
"A reply has been delivered". But the hidden state is maintained:
"REPLIED + queued ADDFD+SEND" == in flight, and "REPLIED + no queued
addfd" == delivered.

Thank you for the selftest addition, the sched trick is nice!

> Tested in QEMU against Linux 7.3.0-rc3 by exercising kernel/seccomp.c
> and verifying the fix with KASAN enabled.

I've verified this now too. It's kind of a ugly problem because unlucky
timing makes it look like the returned fd is fd 0. :(

> [...]
> +	/*
> +	 * Demote the tracee to SCHED_IDLE and promote the supervisor to
> +	 * SCHED_FIFO(99) on the same CPU.
> +	 */
> +	sched_setscheduler(pid, SCHED_IDLE, &sp_tracee_idle);

Unchecked return value?

> +	if (sched_setscheduler(0, SCHED_FIFO, &sp_supervisor_fifo) != 0) {
> +		close(listener);
> +		close(memfd);
> +		kill(pid, SIGKILL);
> +		waitpid(pid, NULL, 0);
> +		SKIP(return, "SCHED_FIFO requires CAP_SYS_NICE");
> +	}

Instead of this you may want to look at FIXTURE_TEARDOWN to clean up
(though it's not strictly needed since the harness is run in a
subprocess so all these go away on test exit). The one thing that might
be worth doing is making sure sched_setscheduler(0, SCHED_OTHER, ...)
happens ASAP or the test could block everything on a single CPU
machine/VM/CI.

> [...]
> +	sig_pid = fork();
> +	ASSERT_GE(sig_pid, 0);
> +	if (sig_pid == 0) {
> +		sched_setscheduler(0, SCHED_FIFO, &sp_sig_helper_fifo);

Missed return value check here too.

> +		kill(parent_pid, SIGUSR1);
> +		_exit(0);
> +	}
> +
> +	EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_ADDFD, &addfd), -1);
> +	EXPECT_EQ(errno, EINTR);
> +	EXPECT_EQ(waitpid(sig_pid, &status, 0), sig_pid);
> +
> +	/*
> +	 * Restore normal scheduling and sleep briefly so the woken tracee
> +	 * runs in do_user_notif(). With knotif->state restored to
> +	 * SECCOMP_NOTIFY_SENT, the tracee must loop back to sleep waiting for
> +	 * the notification reply rather than returning 0 from __NR_getppid.
> +	 */
> +	sched_setscheduler(0, SCHED_OTHER, &sp_tracee_idle);
> +	sched_setscheduler(pid, SCHED_OTHER, &sp_tracee_idle);

Need to check these too...

> +	nanosleep(&delay, NULL);
> +
> +	/*
> +	 * Retry SECCOMP_IOCTL_NOTIF_ADDFD. Because knotif->state is
> +	 * SECCOMP_NOTIFY_SENT, the retry succeeds (returns 42) instead of
> +	 * failing with -EINPROGRESS, installs FD 42 into the tracee, and wakes
> +	 * the tracee to complete the syscall with return value 42.
> +	 */
> +	EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_ADDFD, &addfd), 42);
> +
> +	EXPECT_EQ(waitpid(pid, &status, 0), pid);
> +	EXPECT_EQ(true, WIFEXITED(status));
> +	EXPECT_EQ(0, WEXITSTATUS(status));
> +
> +	close(listener);
> +	close(memfd);
> +}
> +
>  #ifndef SECCOMP_USER_NOTIF_FD_SYNC_WAKE_UP
>  #define SECCOMP_USER_NOTIF_FD_SYNC_WAKE_UP (1UL << 0)
>  #define SECCOMP_IOCTL_NOTIF_SET_FLAGS  SECCOMP_IOW(4, __u64)
> -- 
> 2.49.0

Thank you for the test, it really helps see the shape of the issue.

-Kees

-- 
Kees Cook

  reply	other threads:[~2026-09-21  9:19 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-09-19 20:35 [PATCH] " Hui Peng
2026-09-19 22:22 ` Bradley Morgan
2026-09-20  3:50 ` Kees Cook
2026-09-20  4:16   ` [PATCH v2] " Hui Peng
2026-09-20  7:20   ` [PATCH v3] " Hui Peng
2026-09-20 11:56     ` Bradley Morgan
2026-09-20 18:59       ` [PATCH v4] " Hui Peng
2026-09-21  9:19         ` Kees Cook [this message]
2026-09-20 10:10   ` [PATCH] " Lorenzo Stoakes (ARM)
2026-09-20 11:55     ` Bradley Morgan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=202609210158.F3D96F157@keescook \
    --to=kees@kernel.org \
    --cc=benquike@gmail.com \
    --cc=brads@mainlining.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=ljs@kernel.org \
    --cc=luto@amacapital.net \
    --cc=shuah@kernel.org \
    --cc=stable@vger.kernel.org \
    --cc=wad@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®