* [PATCH v3 1/3] samples, selftests/seccomp: Zero out seccomp_notif @ 2019-12-29 6:24 Sargun Dhillon 2019-12-29 6:24 ` [PATCH v3 2/3] seccomp: Check that seccomp_notif is zeroed out by the user Sargun Dhillon ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Sargun Dhillon @ 2019-12-29 6:24 UTC (permalink / raw) To: linux-kernel, linux-api Cc: Jann Horn, Christian Brauner, Kees Cook, Aleksa Sarai, Tycho Andersen, Sargun Dhillon The seccomp_notif structure should be zeroed out prior to calling the SECCOMP_IOCTL_NOTIF_RECV ioctl. Previously, the kernel did not check whether these structures were zeroed out or not, so these worked. This patch zeroes out the seccomp_notif data structure prior to calling the ioctl. Signed-off-by: Sargun Dhillon <sargun@sargun.me> Reviewed-by: Tycho Andersen <tycho@tycho.ws> Cc: Kees Cook <keescook@chromium.org> Cc: Christian Brauner <christian.brauner@ubuntu.com> --- samples/seccomp/user-trap.c | 2 +- tools/testing/selftests/seccomp/seccomp_bpf.c | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/samples/seccomp/user-trap.c b/samples/seccomp/user-trap.c index 6d0125ca8af7..3e31ec0cf4a5 100644 --- a/samples/seccomp/user-trap.c +++ b/samples/seccomp/user-trap.c @@ -298,7 +298,6 @@ int main(void) req = malloc(sizes.seccomp_notif); if (!req) goto out_close; - memset(req, 0, sizeof(*req)); resp = malloc(sizes.seccomp_notif_resp); if (!resp) @@ -306,6 +305,7 @@ int main(void) memset(resp, 0, sizeof(*resp)); while (1) { + memset(req, 0, sizeof(*req)); if (ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, req)) { perror("ioctl recv"); goto out_resp; diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c index 6944b898bb53..f53f14971bff 100644 --- a/tools/testing/selftests/seccomp/seccomp_bpf.c +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c @@ -3278,6 +3278,7 @@ TEST(user_notification_signal) close(sk_pair[1]); + memset(&req, 0, sizeof(req)); EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0); EXPECT_EQ(kill(pid, SIGUSR1), 0); @@ -3296,6 +3297,7 @@ TEST(user_notification_signal) EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), -1); EXPECT_EQ(errno, ENOENT); + memset(&req, 0, sizeof(req)); EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0); resp.id = req.id; -- 2.20.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v3 2/3] seccomp: Check that seccomp_notif is zeroed out by the user 2019-12-29 6:24 [PATCH v3 1/3] samples, selftests/seccomp: Zero out seccomp_notif Sargun Dhillon @ 2019-12-29 6:24 ` Sargun Dhillon 2019-12-30 18:29 ` Kees Cook 2019-12-29 6:24 ` [PATCH v3 3/3] selftests/seccomp: Test kernel catches garbage on SECCOMP_IOCTL_NOTIF_RECV Sargun Dhillon 2019-12-29 16:11 ` [PATCH v3 1/3] samples, selftests/seccomp: Zero out seccomp_notif Christian Brauner 2 siblings, 1 reply; 11+ messages in thread From: Sargun Dhillon @ 2019-12-29 6:24 UTC (permalink / raw) To: linux-kernel, linux-api Cc: Jann Horn, Christian Brauner, Kees Cook, Aleksa Sarai, Tycho Andersen, Sargun Dhillon This patch is a small change in enforcement of the uapi for SECCOMP_IOCTL_NOTIF_RECV ioctl. Specifically, the datastructure which is passed (seccomp_notif) must be zeroed out. Previously any of its members could be set to nonsense values, and we would ignore it. This ensures all fields are set to their zero value. Signed-off-by: Sargun Dhillon <sargun@sargun.me> Cc: Kees Cook <keescook@chromium.org> Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com> Reviewed-by: Aleksa Sarai <cyphar@cyphar.com> Acked-by: Tycho Andersen <tycho@tycho.ws> --- kernel/seccomp.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/kernel/seccomp.c b/kernel/seccomp.c index 12d2227e5786..b6ea3dcb57bf 100644 --- a/kernel/seccomp.c +++ b/kernel/seccomp.c @@ -1026,6 +1026,13 @@ static long seccomp_notify_recv(struct seccomp_filter *filter, struct seccomp_notif unotif; ssize_t ret; + /* Verify that we're not given garbage to keep struct extensible. */ + ret = check_zeroed_user(buf, sizeof(unotif)); + if (ret < 0) + return ret; + if (!ret) + return -EINVAL; + memset(&unotif, 0, sizeof(unotif)); ret = down_interruptible(&filter->notif->request); -- 2.20.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/3] seccomp: Check that seccomp_notif is zeroed out by the user 2019-12-29 6:24 ` [PATCH v3 2/3] seccomp: Check that seccomp_notif is zeroed out by the user Sargun Dhillon @ 2019-12-30 18:29 ` Kees Cook 0 siblings, 0 replies; 11+ messages in thread From: Kees Cook @ 2019-12-30 18:29 UTC (permalink / raw) To: Sargun Dhillon Cc: linux-kernel, linux-api, Jann Horn, Christian Brauner, Aleksa Sarai, Tycho Andersen On Sat, Dec 28, 2019 at 10:24:50PM -0800, Sargun Dhillon wrote: > This patch is a small change in enforcement of the uapi for > SECCOMP_IOCTL_NOTIF_RECV ioctl. Specifically, the datastructure which > is passed (seccomp_notif) must be zeroed out. Previously any of its > members could be set to nonsense values, and we would ignore it. > > This ensures all fields are set to their zero value. > > Signed-off-by: Sargun Dhillon <sargun@sargun.me> > Cc: Kees Cook <keescook@chromium.org> > Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com> > Reviewed-by: Aleksa Sarai <cyphar@cyphar.com> > Acked-by: Tycho Andersen <tycho@tycho.ws> Applied! -- Kees Cook ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 3/3] selftests/seccomp: Test kernel catches garbage on SECCOMP_IOCTL_NOTIF_RECV 2019-12-29 6:24 [PATCH v3 1/3] samples, selftests/seccomp: Zero out seccomp_notif Sargun Dhillon 2019-12-29 6:24 ` [PATCH v3 2/3] seccomp: Check that seccomp_notif is zeroed out by the user Sargun Dhillon @ 2019-12-29 6:24 ` Sargun Dhillon 2019-12-29 17:14 ` Christian Brauner 2019-12-29 16:11 ` [PATCH v3 1/3] samples, selftests/seccomp: Zero out seccomp_notif Christian Brauner 2 siblings, 1 reply; 11+ messages in thread From: Sargun Dhillon @ 2019-12-29 6:24 UTC (permalink / raw) To: linux-kernel, linux-api Cc: Jann Horn, Christian Brauner, Kees Cook, Aleksa Sarai, Tycho Andersen, Sargun Dhillon Add a self-test to make sure that the kernel returns EINVAL, if any of the fields in seccomp_notif are set to non-null. Signed-off-by: Sargun Dhillon <sargun@sargun.me> Suggested-by: Christian Brauner <christian.brauner@ubuntu.com> Cc: Kees Cook <keescook@chromium.org> --- tools/testing/selftests/seccomp/seccomp_bpf.c | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c index f53f14971bff..379391a7fa41 100644 --- a/tools/testing/selftests/seccomp/seccomp_bpf.c +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c @@ -3601,6 +3601,29 @@ TEST(user_notification_continue) } } +TEST(user_notification_garbage) +{ + /* + * intentionally set pid to a garbage value to make sure the kernel + * catches it + */ + struct seccomp_notif req = { + .pid = 1, + }; + int ret, listener; + + 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_trap_syscall(__NR_dup, SECCOMP_FILTER_FLAG_NEW_LISTENER); + ASSERT_GE(listener, 0); + + EXPECT_EQ(-1, ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req)); + EXPECT_EQ(EINVAL, errno); +} + /* * TODO: * - add microbenchmarks -- 2.20.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3 3/3] selftests/seccomp: Test kernel catches garbage on SECCOMP_IOCTL_NOTIF_RECV 2019-12-29 6:24 ` [PATCH v3 3/3] selftests/seccomp: Test kernel catches garbage on SECCOMP_IOCTL_NOTIF_RECV Sargun Dhillon @ 2019-12-29 17:14 ` Christian Brauner 2019-12-29 19:06 ` Sargun Dhillon 0 siblings, 1 reply; 11+ messages in thread From: Christian Brauner @ 2019-12-29 17:14 UTC (permalink / raw) To: Sargun Dhillon Cc: linux-kernel, linux-api, Jann Horn, Kees Cook, Aleksa Sarai, Tycho Andersen On Sat, Dec 28, 2019 at 10:24:51PM -0800, Sargun Dhillon wrote: > Add a self-test to make sure that the kernel returns EINVAL, if any > of the fields in seccomp_notif are set to non-null. > > Signed-off-by: Sargun Dhillon <sargun@sargun.me> > Suggested-by: Christian Brauner <christian.brauner@ubuntu.com> > Cc: Kees Cook <keescook@chromium.org> > --- > tools/testing/selftests/seccomp/seccomp_bpf.c | 23 +++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c > index f53f14971bff..379391a7fa41 100644 > --- a/tools/testing/selftests/seccomp/seccomp_bpf.c > +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c > @@ -3601,6 +3601,29 @@ TEST(user_notification_continue) > } > } > > +TEST(user_notification_garbage) > +{ > + /* > + * intentionally set pid to a garbage value to make sure the kernel > + * catches it > + */ > + struct seccomp_notif req = { > + .pid = 1, > + }; > + int ret, listener; > + > + 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_trap_syscall(__NR_dup, SECCOMP_FILTER_FLAG_NEW_LISTENER); > + ASSERT_GE(listener, 0); > + > + EXPECT_EQ(-1, ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req)); > + EXPECT_EQ(EINVAL, errno); Does that even work if no dup() syscall has been made and trapped? This looks like it would give you ENOENT... If you want a simple solution just do: diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c index 6944b898bb53..4c73ae8679ea 100644 --- a/tools/testing/selftests/seccomp/seccomp_bpf.c +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c @@ -3095,7 +3095,7 @@ TEST(user_notification_basic) pid_t pid; long ret; int status, listener; - struct seccomp_notif req = {}; + struct seccomp_notif req; struct seccomp_notif_resp resp = {}; struct pollfd pollfd; @@ -3158,6 +3158,13 @@ TEST(user_notification_basic) EXPECT_GT(poll(&pollfd, 1, -1), 0); EXPECT_EQ(pollfd.revents, POLLIN); + /* Test that we can't pass garbage to the kernel. */ + memset(&req, 0, sizeof(req)); + req.pid = -1; + EXPECT_EQ(-1, ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req)); + EXPECT_EQ(EINVAL, errno); + + req.pid = 0; EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0); pollfd.fd = listener If you want a complete separate test then you can do: TEST(user_notification_garbage_recv) { pid_t pid; long ret; int status, listener; struct seccomp_notif req; struct seccomp_notif_resp resp = {}; struct pollfd pollfd; 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_trap_syscall(__NR_getppid, SECCOMP_FILTER_FLAG_NEW_LISTENER); ASSERT_GE(listener, 0); pid = fork(); ASSERT_GE(pid, 0); if (pid == 0) { ret = syscall(__NR_getppid); exit(ret != USER_NOTIF_MAGIC); } pollfd.fd = listener; pollfd.events = POLLIN | POLLOUT; EXPECT_GT(poll(&pollfd, 1, -1), 0); EXPECT_EQ(pollfd.revents, POLLIN); memset(&req, 0, sizeof(req)); req.pid = -1; EXPECT_EQ(-1, ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req)); EXPECT_EQ(EINVAL, errno); req.pid = 0; EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0); pollfd.fd = listener; pollfd.events = POLLIN | POLLOUT; EXPECT_GT(poll(&pollfd, 1, -1), 0); EXPECT_EQ(pollfd.revents, POLLOUT); EXPECT_EQ(req.data.nr, __NR_getppid); memset(&resp, 0, sizeof(resp)); resp.id = req.id; resp.error = 0; resp.val = USER_NOTIF_MAGIC; EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), 0); EXPECT_EQ(waitpid(pid, &status, 0), pid); EXPECT_EQ(true, WIFEXITED(status)); EXPECT_EQ(0, WEXITSTATUS(status)); } Christian ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3 3/3] selftests/seccomp: Test kernel catches garbage on SECCOMP_IOCTL_NOTIF_RECV 2019-12-29 17:14 ` Christian Brauner @ 2019-12-29 19:06 ` Sargun Dhillon 2019-12-29 19:43 ` Christian Brauner 0 siblings, 1 reply; 11+ messages in thread From: Sargun Dhillon @ 2019-12-29 19:06 UTC (permalink / raw) To: Christian Brauner Cc: LKML, Linux API, Jann Horn, Kees Cook, Aleksa Sarai, Tycho Andersen On Sun, Dec 29, 2019 at 12:14 PM Christian Brauner <christian.brauner@ubuntu.com> wrote: > > On Sat, Dec 28, 2019 at 10:24:51PM -0800, Sargun Dhillon wrote: > > Add a self-test to make sure that the kernel returns EINVAL, if any > > of the fields in seccomp_notif are set to non-null. > > > > Signed-off-by: Sargun Dhillon <sargun@sargun.me> > > Suggested-by: Christian Brauner <christian.brauner@ubuntu.com> > > Cc: Kees Cook <keescook@chromium.org> > > --- > > tools/testing/selftests/seccomp/seccomp_bpf.c | 23 +++++++++++++++++++ > > 1 file changed, 23 insertions(+) > > > > diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c > > index f53f14971bff..379391a7fa41 100644 > > --- a/tools/testing/selftests/seccomp/seccomp_bpf.c > > +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c > > @@ -3601,6 +3601,29 @@ TEST(user_notification_continue) > > } > > } > > > > +TEST(user_notification_garbage) > > +{ > > + /* > > + * intentionally set pid to a garbage value to make sure the kernel > > + * catches it > > + */ > > + struct seccomp_notif req = { > > + .pid = 1, > > + }; > > + int ret, listener; > > + > > + 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_trap_syscall(__NR_dup, SECCOMP_FILTER_FLAG_NEW_LISTENER); > > + ASSERT_GE(listener, 0); > > + > > + EXPECT_EQ(-1, ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req)); > > + EXPECT_EQ(EINVAL, errno); > > Does that even work if no dup() syscall has been made and trapped? Yes, the first check that occurs is the check which checks if seccom_notif has been zeroed out. This happens before any of the other work. > This looks like it would give you ENOENT... This ioctl is a blocking ioctl. It'll block until there is a wakeup. In this case, the wakeup will never come, but that doesn't mean we get an ENOENT. > > If you want a simple solution just do: > > diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c > index 6944b898bb53..4c73ae8679ea 100644 > --- a/tools/testing/selftests/seccomp/seccomp_bpf.c > +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c > @@ -3095,7 +3095,7 @@ TEST(user_notification_basic) > pid_t pid; > long ret; > int status, listener; > - struct seccomp_notif req = {}; > + struct seccomp_notif req; > struct seccomp_notif_resp resp = {}; > struct pollfd pollfd; > > @@ -3158,6 +3158,13 @@ TEST(user_notification_basic) > EXPECT_GT(poll(&pollfd, 1, -1), 0); > EXPECT_EQ(pollfd.revents, POLLIN); > > + /* Test that we can't pass garbage to the kernel. */ > + memset(&req, 0, sizeof(req)); > + req.pid = -1; > + EXPECT_EQ(-1, ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req)); > + EXPECT_EQ(EINVAL, errno); > + > + req.pid = 0; > EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0); > > pollfd.fd = listener > > > If you want a complete separate test then you can do: I can do this, but given that the seccomp_notif datastructure should always be copied and checked before doing the actual evaluation of the syscall, this test should pass even if the trap is not triggered. The basic test should check if the inverse holds. If the kernel is broken the self-test harness will stall, and the alarm timeout will kick in. > > TEST(user_notification_garbage_recv) > { > pid_t pid; > long ret; > int status, listener; > struct seccomp_notif req; > struct seccomp_notif_resp resp = {}; > struct pollfd pollfd; > > 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_trap_syscall(__NR_getppid, > SECCOMP_FILTER_FLAG_NEW_LISTENER); > ASSERT_GE(listener, 0); > > pid = fork(); > ASSERT_GE(pid, 0); > > if (pid == 0) { > ret = syscall(__NR_getppid); > exit(ret != USER_NOTIF_MAGIC); > } > > pollfd.fd = listener; > pollfd.events = POLLIN | POLLOUT; > > EXPECT_GT(poll(&pollfd, 1, -1), 0); > EXPECT_EQ(pollfd.revents, POLLIN); > > memset(&req, 0, sizeof(req)); > req.pid = -1; > EXPECT_EQ(-1, ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req)); > EXPECT_EQ(EINVAL, errno); > > req.pid = 0; > EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0); > > pollfd.fd = listener; > pollfd.events = POLLIN | POLLOUT; > > EXPECT_GT(poll(&pollfd, 1, -1), 0); > EXPECT_EQ(pollfd.revents, POLLOUT); > > EXPECT_EQ(req.data.nr, __NR_getppid); > > memset(&resp, 0, sizeof(resp)); > resp.id = req.id; > resp.error = 0; > resp.val = USER_NOTIF_MAGIC; > EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), 0); > > EXPECT_EQ(waitpid(pid, &status, 0), pid); > EXPECT_EQ(true, WIFEXITED(status)); > EXPECT_EQ(0, WEXITSTATUS(status)); > } > > Christian ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 3/3] selftests/seccomp: Test kernel catches garbage on SECCOMP_IOCTL_NOTIF_RECV 2019-12-29 19:06 ` Sargun Dhillon @ 2019-12-29 19:43 ` Christian Brauner 2019-12-29 23:42 ` Sargun Dhillon 0 siblings, 1 reply; 11+ messages in thread From: Christian Brauner @ 2019-12-29 19:43 UTC (permalink / raw) To: Sargun Dhillon Cc: LKML, Linux API, Jann Horn, Kees Cook, Aleksa Sarai, Tycho Andersen On Sun, Dec 29, 2019 at 11:06:25AM -0800, Sargun Dhillon wrote: > On Sun, Dec 29, 2019 at 12:14 PM Christian Brauner > <christian.brauner@ubuntu.com> wrote: > > > > On Sat, Dec 28, 2019 at 10:24:51PM -0800, Sargun Dhillon wrote: > > > Add a self-test to make sure that the kernel returns EINVAL, if any > > > of the fields in seccomp_notif are set to non-null. > > > > > > Signed-off-by: Sargun Dhillon <sargun@sargun.me> > > > Suggested-by: Christian Brauner <christian.brauner@ubuntu.com> > > > Cc: Kees Cook <keescook@chromium.org> > > > --- > > > tools/testing/selftests/seccomp/seccomp_bpf.c | 23 +++++++++++++++++++ > > > 1 file changed, 23 insertions(+) > > > > > > diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c > > > index f53f14971bff..379391a7fa41 100644 > > > --- a/tools/testing/selftests/seccomp/seccomp_bpf.c > > > +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c > > > @@ -3601,6 +3601,29 @@ TEST(user_notification_continue) > > > } > > > } > > > > > > +TEST(user_notification_garbage) > > > +{ > > > + /* > > > + * intentionally set pid to a garbage value to make sure the kernel > > > + * catches it > > > + */ > > > + struct seccomp_notif req = { > > > + .pid = 1, > > > + }; > > > + int ret, listener; > > > + > > > + 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_trap_syscall(__NR_dup, SECCOMP_FILTER_FLAG_NEW_LISTENER); > > > + ASSERT_GE(listener, 0); > > > + > > > + EXPECT_EQ(-1, ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req)); > > > + EXPECT_EQ(EINVAL, errno); > > > > Does that even work if no dup() syscall has been made and trapped? > Yes, the first check that occurs is the check which checks if > seccom_notif has been > zeroed out. This happens before any of the other work. Ah, then sure I don't mind doing it this way. Though plumbing it directly into TEST(user_notification_basic) like I did below seems cleaner to me. > > > This looks like it would give you ENOENT... > This ioctl is a blocking ioctl. It'll block until there is a wakeup. > In this case, the wakeup > will never come, but that doesn't mean we get an ENOENT. Yeah, but that wold mean the test will hang weirdly if it bypasses the check. Sure it'll timeout but meh. I think I would prefer to have this done as part of the basic test where we know that there is an event but _shrug_. Christian ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 3/3] selftests/seccomp: Test kernel catches garbage on SECCOMP_IOCTL_NOTIF_RECV 2019-12-29 19:43 ` Christian Brauner @ 2019-12-29 23:42 ` Sargun Dhillon 2019-12-30 18:29 ` Kees Cook 0 siblings, 1 reply; 11+ messages in thread From: Sargun Dhillon @ 2019-12-29 23:42 UTC (permalink / raw) To: Christian Brauner Cc: LKML, Linux API, Jann Horn, Kees Cook, Aleksa Sarai, Tycho Andersen On Sun, Dec 29, 2019 at 11:43 AM Christian Brauner <christian.brauner@ubuntu.com> wrote: > > On Sun, Dec 29, 2019 at 11:06:25AM -0800, Sargun Dhillon wrote: > > On Sun, Dec 29, 2019 at 12:14 PM Christian Brauner > > <christian.brauner@ubuntu.com> wrote: > > > Does that even work if no dup() syscall has been made and trapped? > > Yes, the first check that occurs is the check which checks if > > seccom_notif has been > > zeroed out. This happens before any of the other work. > > Ah, then sure I don't mind doing it this way. Though plumbing it > directly into TEST(user_notification_basic) like I did below seems > cleaner to me. > > > > > > This looks like it would give you ENOENT... > > This ioctl is a blocking ioctl. It'll block until there is a wakeup. > > In this case, the wakeup > > will never come, but that doesn't mean we get an ENOENT. > > Yeah, but that wold mean the test will hang weirdly if it bypasses the > check. Sure it'll timeout but meh. I think I would prefer to have this > done as part of the basic test where we know that there is an event but > _shrug_. > > Christian My one worry about this is that the behaviour should be if the input (seccomp_notif) is invalid, it should immediately bail out, whether or not there is an event waiting. If we add it to basic_test, then it would hide the erroneous behaviour if bailout isn't immediate. I'm not sure if that's a worry or not. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 3/3] selftests/seccomp: Test kernel catches garbage on SECCOMP_IOCTL_NOTIF_RECV 2019-12-29 23:42 ` Sargun Dhillon @ 2019-12-30 18:29 ` Kees Cook 0 siblings, 0 replies; 11+ messages in thread From: Kees Cook @ 2019-12-30 18:29 UTC (permalink / raw) To: Sargun Dhillon Cc: Christian Brauner, LKML, Linux API, Jann Horn, Aleksa Sarai, Tycho Andersen On Sun, Dec 29, 2019 at 03:42:17PM -0800, Sargun Dhillon wrote: > On Sun, Dec 29, 2019 at 11:43 AM Christian Brauner > <christian.brauner@ubuntu.com> wrote: > > > > On Sun, Dec 29, 2019 at 11:06:25AM -0800, Sargun Dhillon wrote: > > > On Sun, Dec 29, 2019 at 12:14 PM Christian Brauner > > > <christian.brauner@ubuntu.com> wrote: > > > > Does that even work if no dup() syscall has been made and trapped? > > > Yes, the first check that occurs is the check which checks if > > > seccom_notif has been > > > zeroed out. This happens before any of the other work. > > > > Ah, then sure I don't mind doing it this way. Though plumbing it > > directly into TEST(user_notification_basic) like I did below seems > > cleaner to me. > > > > > > > > > This looks like it would give you ENOENT... > > > This ioctl is a blocking ioctl. It'll block until there is a wakeup. > > > In this case, the wakeup > > > will never come, but that doesn't mean we get an ENOENT. > > > > Yeah, but that wold mean the test will hang weirdly if it bypasses the > > check. Sure it'll timeout but meh. I think I would prefer to have this > > done as part of the basic test where we know that there is an event but > > _shrug_. I'd like to design the tests to not _depend_ on the timeout to catch bad behaviors. The timeout is there for us to not break a test runner when we forget weird corner cases. > My one worry about this is that the behaviour should be if the input > (seccomp_notif) is invalid, it should immediately bail out, whether > or not there is an event waiting. If we add it to basic_test, then > it would hide the erroneous behaviour if bailout isn't immediate. I'm not following this paragraph. The ioctl's zero check is immediate, so this test should fail the EXPECT (but continue to the next "correct" ioctl) -- it's not an ASSERT (which would stop the test). I think Christian's test might be a cleaner approach, unless I'm still missing something here? -- Kees Cook ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/3] samples, selftests/seccomp: Zero out seccomp_notif 2019-12-29 6:24 [PATCH v3 1/3] samples, selftests/seccomp: Zero out seccomp_notif Sargun Dhillon 2019-12-29 6:24 ` [PATCH v3 2/3] seccomp: Check that seccomp_notif is zeroed out by the user Sargun Dhillon 2019-12-29 6:24 ` [PATCH v3 3/3] selftests/seccomp: Test kernel catches garbage on SECCOMP_IOCTL_NOTIF_RECV Sargun Dhillon @ 2019-12-29 16:11 ` Christian Brauner 2019-12-30 18:29 ` Kees Cook 2 siblings, 1 reply; 11+ messages in thread From: Christian Brauner @ 2019-12-29 16:11 UTC (permalink / raw) To: Sargun Dhillon Cc: linux-kernel, linux-api, Jann Horn, Kees Cook, Aleksa Sarai, Tycho Andersen On Sat, Dec 28, 2019 at 10:24:49PM -0800, Sargun Dhillon wrote: > The seccomp_notif structure should be zeroed out prior to calling the > SECCOMP_IOCTL_NOTIF_RECV ioctl. Previously, the kernel did not check > whether these structures were zeroed out or not, so these worked. > > This patch zeroes out the seccomp_notif data structure prior to calling > the ioctl. > > Signed-off-by: Sargun Dhillon <sargun@sargun.me> > Reviewed-by: Tycho Andersen <tycho@tycho.ws> > Cc: Kees Cook <keescook@chromium.org> > Cc: Christian Brauner <christian.brauner@ubuntu.com> Thanks! Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/3] samples, selftests/seccomp: Zero out seccomp_notif 2019-12-29 16:11 ` [PATCH v3 1/3] samples, selftests/seccomp: Zero out seccomp_notif Christian Brauner @ 2019-12-30 18:29 ` Kees Cook 0 siblings, 0 replies; 11+ messages in thread From: Kees Cook @ 2019-12-30 18:29 UTC (permalink / raw) To: Christian Brauner Cc: Sargun Dhillon, linux-kernel, linux-api, Jann Horn, Aleksa Sarai, Tycho Andersen On Sun, Dec 29, 2019 at 05:11:28PM +0100, Christian Brauner wrote: > On Sat, Dec 28, 2019 at 10:24:49PM -0800, Sargun Dhillon wrote: > > The seccomp_notif structure should be zeroed out prior to calling the > > SECCOMP_IOCTL_NOTIF_RECV ioctl. Previously, the kernel did not check > > whether these structures were zeroed out or not, so these worked. > > > > This patch zeroes out the seccomp_notif data structure prior to calling > > the ioctl. > > > > Signed-off-by: Sargun Dhillon <sargun@sargun.me> > > Reviewed-by: Tycho Andersen <tycho@tycho.ws> > > Cc: Kees Cook <keescook@chromium.org> > > Cc: Christian Brauner <christian.brauner@ubuntu.com> > > Thanks! > Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com> Thanks for this series and the discussion! :) I've applied this to my tree for Linus. -- Kees Cook ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-12-30 18:29 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-29 6:24 [PATCH v3 1/3] samples, selftests/seccomp: Zero out seccomp_notif Sargun Dhillon 2019-12-29 6:24 ` [PATCH v3 2/3] seccomp: Check that seccomp_notif is zeroed out by the user Sargun Dhillon 2019-12-30 18:29 ` Kees Cook 2019-12-29 6:24 ` [PATCH v3 3/3] selftests/seccomp: Test kernel catches garbage on SECCOMP_IOCTL_NOTIF_RECV Sargun Dhillon 2019-12-29 17:14 ` Christian Brauner 2019-12-29 19:06 ` Sargun Dhillon 2019-12-29 19:43 ` Christian Brauner 2019-12-29 23:42 ` Sargun Dhillon 2019-12-30 18:29 ` Kees Cook 2019-12-29 16:11 ` [PATCH v3 1/3] samples, selftests/seccomp: Zero out seccomp_notif Christian Brauner 2019-12-30 18:29 ` Kees Cook
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).