Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 29 Apr 2022 22:35:57 +0000
From:   Sargun Dhillon <sargun@sargun.me>
To:     Kees Cook <keescook@chromium.org>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Linux Containers <containers@lists.linux-foundation.org>,
        Rodrigo Campos <rodrigo@kinvolk.io>,
        Christian Brauner <christian.brauner@ubuntu.com>,
        Giuseppe Scrivano <gscrivan@redhat.com>,
        Will Drewry <wad@chromium.org>,
        Andy Lutomirski <luto@amacapital.net>,
        Alban Crequy <alban@kinvolk.io>,
        Tycho Andersen <tycho@tycho.pizza>
Subject: Re: [PATCH v3 2/2] selftests/seccomp: Add test for wait killable
 notifier
Message-ID: <20220429223557.GB1267404@ircssh-3.c.rugged-nimbus-611.internal>
References: <20220429023113.74993-1-sargun@sargun.me>
 <20220429023113.74993-3-sargun@sargun.me>
 <202204291053.E04A367@keescook>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <202204291053.E04A367@keescook>
Precedence: bulk

On Fri, Apr 29, 2022 at 11:19:33AM -0700, Kees Cook wrote:
> On Thu, Apr 28, 2022 at 07:31:13PM -0700, Sargun Dhillon wrote:
> > +
> > +	ASSERT_EQ(socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, sk_pair), 0);
> > +
> > +	listener = user_notif_syscall(__NR_getppid,
> > +				      SECCOMP_FILTER_FLAG_NEW_LISTENER |
> > +				      SECCOMP_FILTER_FLAG_WAIT_KILLABLE_RECV);
> > +	ASSERT_GE(listener, 0);
> > +
> > +	pid = fork();
> > +	ASSERT_GE(pid, 0);
> > +
> > +	if (pid == 0) {
> > +		close(sk_pair[0]);
> > +		handled = sk_pair[1];
> > +
> > +		/* Setup the sigaction without SA_RESTART */
> > +		if (sigaction(SIGUSR1, &new_action, NULL)) {
> > +			perror("sigaction");
> > +			exit(1);
> > +		}
> > +
> > +		/* Make sure that the syscall is completed (no EINTR) */
> > +		ret = syscall(__NR_getppid);
> > +		exit(ret != USER_NOTIF_MAGIC);
> > +	}
> > +
> > +	while (get_proc_syscall(pid) != __NR_getppid &&
> > +	       get_proc_stat(pid) != 'S')
> > +		nanosleep(&delay, NULL);
> > +
> > +	EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
> > +	/* Kill the process to make sure it enters the wait_killable state */
> > +	EXPECT_EQ(kill(pid, SIGUSR1), 0);
> > +
> > +	/* TASK_KILLABLE is considered D (Disk Sleep) state */
> > +	while (get_proc_stat(pid) != 'D')
> > +		nanosleep(&delay, NULL);
> 
> Should a NOWAIT waitpid() happen in this loop to make sure this doesn't
> spin forever?
> 
> i.e. running these tests on a kernel that doesn't have the support
> shouldn't hang -- yes it'll time out eventually but that's annoying. ;)
> 
Wouldn't this bail already because user_notif_syscall would assert out
since the kernel would reject the unknown flag?

I might make this a little helper function, something like:
static void wait_for_state(struct __test_metadata *_metadata, pid_t pid, char wait_for) {
	/* 100 ms */
	struct timespec delay = { .tv_nsec = 100000000 };
	int status;

	while (get_proc_stat(pid) != wait_for) {
		ASSERT_EQ(waitpid(pid, &status, WNOHANG), 0) {
			if (WIFEXITED(status))
				TH_LOG("Process %d exited with error code %d", pid, WEXITSTATUS(status));
			else if (WIFSIGNALED(status))
				TH_LOG("Process %d exited due to signal %d", pid, WTERMSIG(status));
			else
				TH_LOG("Process %d exited due to unknown reason", pid);
		}
		nanosleep(&delay, NULL);
	}
}
	
}

> > +
> > +	resp.id = req.id;
> > +	resp.val = USER_NOTIF_MAGIC;
> > +	EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), 0);
> > +
> > +	/*
> > +	 * Make sure that the signal handler does get called once we're back in
> > +	 * userspace.
> > +	 */
> > +	EXPECT_EQ(read(sk_pair[0], &c, 1), 1);
> > +	EXPECT_EQ(waitpid(pid, &status, 0), pid);
> > +	EXPECT_EQ(true, WIFEXITED(status));
> > +	EXPECT_EQ(0, WEXITSTATUS(status));
> > +}
> > +
> > +TEST(user_notification_wait_killable_fatal)
> > +{
> > +	struct seccomp_notif req = {};
> > +	int listener, status;
> > +	pid_t pid;
> > +	long ret;
> > +	/* 100 ms */
> > +	struct timespec delay = { .tv_nsec = 100000000 };
> > +
> > +	ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
> > +	ASSERT_EQ(0, ret) {
> > +		TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!");
> > +	}
> > +
> > +	listener = user_notif_syscall(__NR_getppid,
> > +				      SECCOMP_FILTER_FLAG_NEW_LISTENER |
> > +				      SECCOMP_FILTER_FLAG_WAIT_KILLABLE_RECV);
> > +	ASSERT_GE(listener, 0);
> > +
> > +	pid = fork();
> > +	ASSERT_GE(pid, 0);
> > +
> > +	if (pid == 0) {
> > +		/* This should never complete */
> > +		syscall(__NR_getppid);
> > +		exit(1);
> > +	}
> > +
> > +	while (get_proc_stat(pid) != 'S')
> > +		nanosleep(&delay, NULL);
> > +
> > +	EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
> > +	/* Kill the process with a fatal signal */
> > +	EXPECT_EQ(kill(pid, SIGTERM), 0);
> > +
> > +	EXPECT_EQ(waitpid(pid, &status, 0), pid);
> > +	EXPECT_EQ(true, WIFSIGNALED(status));
> > +	EXPECT_EQ(SIGTERM, WTERMSIG(status));
> > +}
> 
> Should there be a test validating the inverse of this, as in _without_
> SECCOMP_FILTER_FLAG_WAIT_KILLABLE_RECV, how should the above tests
> behave?
Don't we roughly get that from the user_notification_kill_in_middle
and user_notification_signal?

Although, I might cleanup the user_notification_signal test to disable
SA_RESTART like these tests.

> 
> Otherwise, looks good! Yay tests!
> 
> -- 
> Kees Cook