From 771b894f2f3dfedc2ba5561731fffa0e39b1bbb6 Mon Sep 17 00:00:00 2001 From: Sargun Dhillon Date: Mon, 30 Dec 2019 12:35:03 -0800 Subject: samples/seccomp: Zero out members based on seccomp_notif_sizes The sizes by which seccomp_notif and seccomp_notif_resp are allocated are based on the SECCOMP_GET_NOTIF_SIZES ioctl. This allows for graceful extension of these datastructures. If userspace zeroes out the datastructure based on its version, and it is lagging behind the kernel's version, it will end up sending trailing garbage. On the other hand, if it is ahead of the kernel version, it will write extra zero space, and potentially cause corruption. Signed-off-by: Sargun Dhillon Suggested-by: Tycho Andersen Link: https://lore.kernel.org/r/20191230203503.4925-1-sargun@sargun.me Fixes: fec7b6690541 ("samples: add an example of seccomp user trap") Cc: stable@vger.kernel.org Signed-off-by: Kees Cook --- samples/seccomp/user-trap.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/samples/seccomp/user-trap.c b/samples/seccomp/user-trap.c index 6d0125ca8af7..20291ec6489f 100644 --- a/samples/seccomp/user-trap.c +++ b/samples/seccomp/user-trap.c @@ -298,14 +298,14 @@ 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) goto out_req; - memset(resp, 0, sizeof(*resp)); + memset(resp, 0, sizes.seccomp_notif_resp); while (1) { + memset(req, 0, sizes.seccomp_notif); if (ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, req)) { perror("ioctl recv"); goto out_resp; -- cgit v1.2.3-59-g8ed1b From 88c13f8bd71472fbab5338b01d99122908c77e53 Mon Sep 17 00:00:00 2001 From: Sargun Dhillon Date: Sat, 28 Dec 2019 22:24:49 -0800 Subject: selftests/seccomp: Zero out seccomp_notif 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 Reviewed-by: Tycho Andersen Reviewed-by: Christian Brauner Link: https://lore.kernel.org/r/20191229062451.9467-1-sargun@sargun.me Fixes: 6a21cc50f0c7 ("seccomp: add a return code to trap to userspace") Cc: stable@vger.kernel.org Signed-off-by: Kees Cook --- tools/testing/selftests/seccomp/seccomp_bpf.c | 2 ++ 1 file changed, 2 insertions(+) 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; -- cgit v1.2.3-59-g8ed1b From 2882d53c9c6f3b8311d225062522f03772cf0179 Mon Sep 17 00:00:00 2001 From: Sargun Dhillon Date: Sat, 28 Dec 2019 22:24:50 -0800 Subject: seccomp: Check that seccomp_notif is zeroed out by the user 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 Reviewed-by: Christian Brauner Reviewed-by: Aleksa Sarai Acked-by: Tycho Andersen Link: https://lore.kernel.org/r/20191229062451.9467-2-sargun@sargun.me Fixes: 6a21cc50f0c7 ("seccomp: add a return code to trap to userspace") Cc: stable@vger.kernel.org Signed-off-by: Kees Cook --- 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); -- cgit v1.2.3-59-g8ed1b From e4ab5ccc357b978999328fadae164e098c26fa40 Mon Sep 17 00:00:00 2001 From: Sargun Dhillon Date: Mon, 30 Dec 2019 12:38:11 -0800 Subject: selftests/seccomp: Catch garbage on SECCOMP_IOCTL_NOTIF_RECV This adds logic to the user_notification_basic test to set a member of struct seccomp_notif to an invalid value to ensure that the kernel returns EINVAL if any of the struct seccomp_notif members are set to invalid values. Signed-off-by: Sargun Dhillon Suggested-by: Christian Brauner Link: https://lore.kernel.org/r/20191230203811.4996-1-sargun@sargun.me Fixes: 6a21cc50f0c7 ("seccomp: add a return code to trap to userspace") Cc: stable@vger.kernel.org Signed-off-by: Kees Cook --- tools/testing/selftests/seccomp/seccomp_bpf.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c index f53f14971bff..ee1b727ede04 100644 --- a/tools/testing/selftests/seccomp/seccomp_bpf.c +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c @@ -3158,7 +3158,18 @@ TEST(user_notification_basic) EXPECT_GT(poll(&pollfd, 1, -1), 0); EXPECT_EQ(pollfd.revents, POLLIN); - EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0); + /* Test that we can't pass garbage to the kernel. */ + memset(&req, 0, sizeof(req)); + req.pid = -1; + errno = 0; + ret = ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req); + EXPECT_EQ(-1, ret); + EXPECT_EQ(EINVAL, errno); + + if (ret) { + req.pid = 0; + EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0); + } pollfd.fd = listener; pollfd.events = POLLIN | POLLOUT; -- cgit v1.2.3-59-g8ed1b