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
On Fri, Apr 29, 2022 at 10:35:57PM +0000, Sargun Dhillon wrote:
> 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?
Oh yeah, duh. :P
> 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);
> }
> }
Yeah, though as you point out, that is likely overkill. :)
> > > + 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?
Yeah, I guess that's true. Cool, cool.
> Although, I might cleanup the user_notification_signal test to disable
> SA_RESTART like these tests.
Sounds good, though maybe that can be a separate patch?
--
Kees Cook