| Message ID | 20200616032524.460144-11-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add seccomp notifier ioctl that enables adding fds | expand |
On Mon, Jun 15, 2020 at 08:25:23PM -0700, Kees Cook wrote: > This patch is based on discussions[1] with Sargun Dhillon, Christian > Brauner, and David Laight. Instead of building size into the addfd > structure, make it a function of the ioctl command (which is how sizes are > normally passed to ioctls). To support forward and backward compatibility, > just mask out the direction and size, and match everything. The size (and > any future direction) checks are done along with copy_struct_from_user() > logic. Also update the selftests to check size bounds. > > [1] https://lore.kernel.org/lkml/20200612104629.GA15814@ircssh-2.c.rugged-nimbus-611.internal > > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > include/uapi/linux/seccomp.h | 2 - > kernel/seccomp.c | 21 ++++++---- > tools/testing/selftests/seccomp/seccomp_bpf.c | 40 ++++++++++++++++--- > 3 files changed, 49 insertions(+), 14 deletions(-) > > diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h > index c347160378e5..473a61695ac3 100644 > --- a/include/uapi/linux/seccomp.h > +++ b/include/uapi/linux/seccomp.h > @@ -118,7 +118,6 @@ struct seccomp_notif_resp { > > /** > * struct seccomp_notif_addfd > - * @size: The size of the seccomp_notif_addfd structure > * @id: The ID of the seccomp notification > * @flags: SECCOMP_ADDFD_FLAG_* > * @srcfd: The local fd number > @@ -126,7 +125,6 @@ struct seccomp_notif_resp { > * @newfd_flags: The O_* flags the remote FD should have applied > */ > struct seccomp_notif_addfd { > - __u64 size; Huh? Won't this break builds? Tycho
On Tue, Jun 16, 2020 at 08:55:46AM -0600, Tycho Andersen wrote: > On Mon, Jun 15, 2020 at 08:25:23PM -0700, Kees Cook wrote: > > This patch is based on discussions[1] with Sargun Dhillon, Christian > > Brauner, and David Laight. Instead of building size into the addfd > > structure, make it a function of the ioctl command (which is how sizes are > > normally passed to ioctls). To support forward and backward compatibility, > > just mask out the direction and size, and match everything. The size (and > > any future direction) checks are done along with copy_struct_from_user() > > logic. Also update the selftests to check size bounds. > > > > [1] https://lore.kernel.org/lkml/20200612104629.GA15814@ircssh-2.c.rugged-nimbus-611.internal > > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > --- > > include/uapi/linux/seccomp.h | 2 - > > kernel/seccomp.c | 21 ++++++---- > > tools/testing/selftests/seccomp/seccomp_bpf.c | 40 ++++++++++++++++--- > > 3 files changed, 49 insertions(+), 14 deletions(-) > > > > diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h > > index c347160378e5..473a61695ac3 100644 > > --- a/include/uapi/linux/seccomp.h > > +++ b/include/uapi/linux/seccomp.h > > @@ -118,7 +118,6 @@ struct seccomp_notif_resp { > > > > /** > > * struct seccomp_notif_addfd > > - * @size: The size of the seccomp_notif_addfd structure > > * @id: The ID of the seccomp notification > > * @flags: SECCOMP_ADDFD_FLAG_* > > * @srcfd: The local fd number > > @@ -126,7 +125,6 @@ struct seccomp_notif_resp { > > * @newfd_flags: The O_* flags the remote FD should have applied > > */ > > struct seccomp_notif_addfd { > > - __u64 size; > > Huh? Won't this break builds? Only if they use addfd without this patch? :) Are you saying I should collapse this patch into the main addfd and test patches?
On Tue, Jun 16, 2020 at 09:05:29AM -0700, Kees Cook wrote: > On Tue, Jun 16, 2020 at 08:55:46AM -0600, Tycho Andersen wrote: > > On Mon, Jun 15, 2020 at 08:25:23PM -0700, Kees Cook wrote: > > > This patch is based on discussions[1] with Sargun Dhillon, Christian > > > Brauner, and David Laight. Instead of building size into the addfd > > > structure, make it a function of the ioctl command (which is how sizes are > > > normally passed to ioctls). To support forward and backward compatibility, > > > just mask out the direction and size, and match everything. The size (and > > > any future direction) checks are done along with copy_struct_from_user() > > > logic. Also update the selftests to check size bounds. > > > > > > [1] https://lore.kernel.org/lkml/20200612104629.GA15814@ircssh-2.c.rugged-nimbus-611.internal > > > > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > > --- > > > include/uapi/linux/seccomp.h | 2 - > > > kernel/seccomp.c | 21 ++++++---- > > > tools/testing/selftests/seccomp/seccomp_bpf.c | 40 ++++++++++++++++--- > > > 3 files changed, 49 insertions(+), 14 deletions(-) > > > > > > diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h > > > index c347160378e5..473a61695ac3 100644 > > > --- a/include/uapi/linux/seccomp.h > > > +++ b/include/uapi/linux/seccomp.h > > > @@ -118,7 +118,6 @@ struct seccomp_notif_resp { > > > > > > /** > > > * struct seccomp_notif_addfd > > > - * @size: The size of the seccomp_notif_addfd structure > > > * @id: The ID of the seccomp notification > > > * @flags: SECCOMP_ADDFD_FLAG_* > > > * @srcfd: The local fd number > > > @@ -126,7 +125,6 @@ struct seccomp_notif_resp { > > > * @newfd_flags: The O_* flags the remote FD should have applied > > > */ > > > struct seccomp_notif_addfd { > > > - __u64 size; > > > > Huh? Won't this break builds? > > Only if they use addfd without this patch? :) Are you saying I should > collapse this patch into the main addfd and test patches? Oh, derp, I see :) Yeah, maybe that would be good. Tycho
diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h index c347160378e5..473a61695ac3 100644 --- a/include/uapi/linux/seccomp.h +++ b/include/uapi/linux/seccomp.h @@ -118,7 +118,6 @@ struct seccomp_notif_resp { /** * struct seccomp_notif_addfd - * @size: The size of the seccomp_notif_addfd structure * @id: The ID of the seccomp notification * @flags: SECCOMP_ADDFD_FLAG_* * @srcfd: The local fd number @@ -126,7 +125,6 @@ struct seccomp_notif_resp { * @newfd_flags: The O_* flags the remote FD should have applied */ struct seccomp_notif_addfd { - __u64 size; __u64 id; __u32 flags; __u32 srcfd; diff --git a/kernel/seccomp.c b/kernel/seccomp.c index 3c913f3b8451..9660abf91135 100644 --- a/kernel/seccomp.c +++ b/kernel/seccomp.c @@ -1292,17 +1292,17 @@ static long seccomp_notify_id_valid(struct seccomp_filter *filter, } static long seccomp_notify_addfd(struct seccomp_filter *filter, - struct seccomp_notif_addfd __user *uaddfd) + struct seccomp_notif_addfd __user *uaddfd, + unsigned int size) { struct seccomp_notif_addfd addfd; struct seccomp_knotif *knotif; struct seccomp_kaddfd kaddfd; - u64 size; int ret; - ret = get_user(size, &uaddfd->size); - if (ret) - return ret; + /* 24 is original sizeof(struct seccomp_notif_addfd) */ + if (size < 24 || size >= PAGE_SIZE) + return -EINVAL; ret = copy_struct_from_user(&addfd, sizeof(addfd), uaddfd, size); if (ret) @@ -1391,6 +1391,7 @@ static long seccomp_notify_ioctl(struct file *file, unsigned int cmd, struct seccomp_filter *filter = file->private_data; void __user *buf = (void __user *)arg; + /* Fixed-size ioctls */ switch (cmd) { case SECCOMP_IOCTL_NOTIF_RECV: return seccomp_notify_recv(filter, buf); @@ -1398,11 +1399,17 @@ static long seccomp_notify_ioctl(struct file *file, unsigned int cmd, return seccomp_notify_send(filter, buf); case SECCOMP_IOCTL_NOTIF_ID_VALID: return seccomp_notify_id_valid(filter, buf); - case SECCOMP_IOCTL_NOTIF_ADDFD: - return seccomp_notify_addfd(filter, buf); + } + + /* Extensible Argument ioctls */ +#define EA_IOCTL(cmd) ((cmd) & ~(IOC_INOUT | IOCSIZE_MASK)) + switch (EA_IOCTL(cmd)) { + case EA_IOCTL(SECCOMP_IOCTL_NOTIF_ADDFD): + return seccomp_notify_addfd(filter, buf, _IOC_SIZE(cmd)); default: return -EINVAL; } +#undef EA_IOCTL } static __poll_t seccomp_notify_poll(struct file *file, diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c index 95b134933831..cf1480e498ea 100644 --- a/tools/testing/selftests/seccomp/seccomp_bpf.c +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c @@ -216,7 +216,6 @@ struct seccomp_notif_sizes { #define SECCOMP_ADDFD_FLAG_SETFD (1UL << 0) /* Specify remote fd */ struct seccomp_notif_addfd { - __u64 size; __u64 id; __u32 flags; __u32 srcfd; @@ -225,6 +224,22 @@ struct seccomp_notif_addfd { }; #endif +struct seccomp_notif_addfd_small { + __u64 id; + char weird[4]; +}; +#define SECCOMP_IOCTL_NOTIF_ADDFD_SMALL \ + SECCOMP_IOW(3, struct seccomp_notif_addfd_small) + +struct seccomp_notif_addfd_big { + union { + struct seccomp_notif_addfd addfd; + char buf[sizeof(struct seccomp_notif_addfd) + 8]; + }; +}; +#define SECCOMP_IOCTL_NOTIF_ADDFD_BIG \ + SECCOMP_IOWR(3, struct seccomp_notif_addfd_big) + #ifndef PTRACE_EVENTMSG_SYSCALL_ENTRY #define PTRACE_EVENTMSG_SYSCALL_ENTRY 1 #define PTRACE_EVENTMSG_SYSCALL_EXIT 2 @@ -3853,6 +3868,8 @@ TEST(user_notification_sendfd) long ret; int status, listener, memfd, fd; struct seccomp_notif_addfd addfd = {}; + struct seccomp_notif_addfd_small small = {}; + struct seccomp_notif_addfd_big big = {}; struct seccomp_notif req = {}; struct seccomp_notif_resp resp = {}; /* 100 ms */ @@ -3882,7 +3899,6 @@ TEST(user_notification_sendfd) ASSERT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0); - addfd.size = sizeof(addfd); addfd.srcfd = memfd; addfd.newfd = 0; addfd.id = req.id; @@ -3906,6 +3922,16 @@ TEST(user_notification_sendfd) EXPECT_EQ(errno, EINVAL); addfd.newfd = 0; + /* Verify small size cannot be set */ + EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_ADDFD_SMALL, &small), -1); + EXPECT_EQ(errno, EINVAL); + + /* Verify we can't send bits filled in unknown buffer area */ + memset(&big, 0xAA, sizeof(big)); + big.addfd = addfd; + EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_ADDFD_BIG, &big), -1); + EXPECT_EQ(errno, E2BIG); + /* Verify we can set an arbitrary remote fd */ fd = ioctl(listener, SECCOMP_IOCTL_NOTIF_ADDFD, &addfd); /* @@ -3921,10 +3947,15 @@ TEST(user_notification_sendfd) EXPECT_EQ(ret, 0); } + /* Verify we can set an arbitrary remote fd with large size */ + memset(&big, 0x0, sizeof(big)); + big.addfd = addfd; + fd = ioctl(listener, SECCOMP_IOCTL_NOTIF_ADDFD_BIG, &big); + EXPECT_EQ(fd, 6); + /* Verify we can set a specific remote fd */ addfd.newfd = 42; addfd.flags = SECCOMP_ADDFD_FLAG_SETFD; - fd = ioctl(listener, SECCOMP_IOCTL_NOTIF_ADDFD, &addfd); EXPECT_EQ(fd, 42); ret = filecmp(getpid(), pid, memfd, fd); @@ -3935,10 +3966,10 @@ TEST(user_notification_sendfd) EXPECT_EQ(ret, 0); } + /* Resume syscall */ resp.id = req.id; resp.error = 0; resp.val = USER_NOTIF_MAGIC; - EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), 0); /* @@ -4006,7 +4037,6 @@ TEST(user_notification_sendfd_rlimit) ASSERT_EQ(prlimit(pid, RLIMIT_NOFILE, &lim, NULL), 0); - addfd.size = sizeof(addfd); addfd.srcfd = memfd; addfd.newfd_flags = O_CLOEXEC; addfd.newfd = 0;
This patch is based on discussions[1] with Sargun Dhillon, Christian Brauner, and David Laight. Instead of building size into the addfd structure, make it a function of the ioctl command (which is how sizes are normally passed to ioctls). To support forward and backward compatibility, just mask out the direction and size, and match everything. The size (and any future direction) checks are done along with copy_struct_from_user() logic. Also update the selftests to check size bounds. [1] https://lore.kernel.org/lkml/20200612104629.GA15814@ircssh-2.c.rugged-nimbus-611.internal Signed-off-by: Kees Cook <keescook@chromium.org> --- include/uapi/linux/seccomp.h | 2 - kernel/seccomp.c | 21 ++++++---- tools/testing/selftests/seccomp/seccomp_bpf.c | 40 ++++++++++++++++--- 3 files changed, 49 insertions(+), 14 deletions(-)