Hello!
v6:
- fix missing fput()
- API name change: s/fd_install_received/receive_fd/
v5: https://lore.kernel.org/lkml/20200617220327.3731559-1-keescook@chromium.org/
This continues the thread-merge between [1] and [2]. tl;dr: add a way for
a seccomp user_notif process manager to inject files into the managed
process in order to handle emulation of various fd-returning syscalls
across security boundaries. Containers folks and Chrome are in need
of the feature, and investigating this solution uncovered (and fixed)
implementation issues with existing file sending routines.
I intend to carry this in the for-next/seccomp tree, unless someone
has objections. :) Please review and test!
-Kees
[1] https://lore.kernel.org/lkml/20200603011044.7972-1-sargun@sargun.me/
[2] https://lore.kernel.org/lkml/20200610045214.1175600-1-keescook@chromium.org/
Kees Cook (5):
net/scm: Regularize compat handling of scm_detach_fds()
fs: Move __scm_install_fd() to __receive_fd()
fs: Add receive_fd() wrapper for __receive_fd()
pidfd: Replace open-coded partial receive_fd()
fs: Expand __receive_fd() to accept existing fd
Sargun Dhillon (2):
seccomp: Introduce addfd ioctl to seccomp user notifier
selftests/seccomp: Test SECCOMP_IOCTL_NOTIF_ADDFD
fs/file.c | 67 +++++
include/linux/file.h | 19 ++
include/linux/net.h | 9 +
include/uapi/linux/seccomp.h | 22 ++
kernel/pid.c | 13 +-
kernel/seccomp.c | 172 ++++++++++++-
net/compat.c | 55 ++---
net/core/scm.c | 50 +---
tools/testing/selftests/seccomp/seccomp_bpf.c | 229 ++++++++++++++++++
9 files changed, 554 insertions(+), 82 deletions(-)
--
2.25.1
Expand __receive_fd() with support for replace_fd() for the coming seccomp
"addfd" ioctl(). Add new wrapper receive_fd_replace() for the new behavior
and update existing wrappers to retain old behavior.
Thanks to Colin Ian King [off-list ref] for pointing out an
uninitialized variable exposure in an earlier version of this patch.
Reviewed-by: Sargun Dhillon <redacted>
Signed-off-by: Kees Cook <redacted>
---
fs/file.c | 24 ++++++++++++++++++------
include/linux/file.h | 10 +++++++---
2 files changed, 25 insertions(+), 9 deletions(-)
@@ -960,18 +961,30 @@ int __receive_fd(struct file *file, int __user *ufd, unsigned int o_flags)if(error)returnerror;-new_fd=get_unused_fd_flags(o_flags);-if(new_fd<0)-returnnew_fd;+if(fd<0){+new_fd=get_unused_fd_flags(o_flags);+if(new_fd<0)+returnnew_fd;+}else+new_fd=fd;if(ufd){error=put_user(new_fd,ufd);if(error){-put_unused_fd(new_fd);+if(fd<0)+put_unused_fd(new_fd);returnerror;}}+if(fd<0)+fd_install(new_fd,get_file(file));+else{+error=replace_fd(new_fd,file,o_flags);+if(error)+returnerror;+}+/**Bumptheusagecountandinstallthefile.Theresultingvalueof*"error"isignoredheresinceweonlyneedtotakeactionwhen
@@ -982,7 +995,6 @@ int __receive_fd(struct file *file, int __user *ufd, unsigned int o_flags)sock_update_netprioidx(&sock->sk->sk_cgrp_data);sock_update_classid(&sock->sk->sk_cgrp_data);}-fd_install(new_fd,get_file(file));returnnew_fd;}
The sock counting (sock_update_netprioidx() and sock_update_classid()) was
missing from pidfd's implementation of received fd installation. Replace
the open-coded version with a call to the new receive_fd()
helper.
Thanks to Vamshi K Sthambamkadi [off-list ref] for
catching a missed fput() in an earlier version of this patch.
Fixes: 8649c322f75c ("pid: Implement pidfd_getfd syscall")
Reviewed-by: Sargun Dhillon <redacted>
Signed-off-by: Kees Cook <redacted>
---
kernel/pid.c | 13 ++-----------
1 file changed, 2 insertions(+), 11 deletions(-)
From: Sargun Dhillon <redacted>
This adds a seccomp notifier ioctl which allows for the listener to
"add" file descriptors to a process which originated a seccomp user
notification. This allows calls like mount, and mknod to be "implemented",
as the return value, and the arguments are data in memory. On the other
hand, calls like connect can be "implemented" using pidfd_getfd.
Unfortunately, there are calls which return file descriptors, like
open, which are vulnerable to ToCToU attacks, and require that the more
privileged supervisor can inspect the argument, and perform the syscall
on behalf of the process generating the notification. This allows the
file descriptor generated from that open call to be returned to the
calling process.
In addition, there is functionality to allow for replacement of specific
file descriptors, following dup2-like semantics.
The ioctl handling is based on the discussions[1] of how Extensible
Arguments should interact with ioctls. 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.
As a note, the seccomp_notif_addfd structure is laid out based on 8-byte
alignment without requiring packing as there have been packing issues
with uapi highlighted before[1][2]. Although we could overload the
newfd field and use -1 to indicate that it is not to be used, doing
so requires changing the size of the fd field, and introduces struct
packing complexity.
[1]: https://lore.kernel.org/lkml/87o8w9bcaf.fsf@mid.deneb.enyo.de/
[2]: https://lore.kernel.org/lkml/a328b91d-fd8f-4f27-b3c2-91a9c45f18c0@rasmusvillemoes.dk/
[3]: https://lore.kernel.org/lkml/20200612104629.GA15814@ircssh-2.c.rugged-nimbus-611.internal
Suggested-by: Matt Denton <redacted>
Link: https://lore.kernel.org/r/20200603011044.7972-4-sargun@sargun.me
Signed-off-by: Sargun Dhillon <redacted>
Co-developed-by: Kees Cook <redacted>
Signed-off-by: Kees Cook <redacted>
---
include/uapi/linux/seccomp.h | 22 +++++
kernel/seccomp.c | 172 ++++++++++++++++++++++++++++++++++-
2 files changed, 193 insertions(+), 1 deletion(-)
@@ -87,10 +87,42 @@ struct seccomp_knotif {longval;u32flags;-/* Signals when this has entered SECCOMP_NOTIFY_REPLIED */+/*+*Signalswhenthishaschangedstates,suchasthelistener+*dying,anewseccompaddfdmessage,orchangingtoREPLIED+*/structcompletionready;structlist_headlist;++/* outstanding addfd requests */+structlist_headaddfd;+};++/**+*structseccomp_kaddfd-containerforseccomp_addfdioctlmessages+*+*@file:Areferencetothefiletoinstallintheothertask+*@fd:Thefdnumbertoinstallitat.Ifthefdnumberis-1,itmeansthe+*installingprocessshouldallocatethefdasnormal.+*@flags:Theflagsforthenewfiledescriptor.Atthemoment,onlyO_CLOEXEC+*isallowed.+*@ret:Thereturnvalueoftheinstallingprocess.Itissettothefdnum+*uponsuccess(>=0).+*@completion:Indicatesthattheinstallingprocesshascompletedfd+*installation,orgoneaway(eitherduetosuccessful+*reply,orsignal)+*+*/+structseccomp_kaddfd{+structfile*file;+intfd;+unsignedintflags;++/* To only be set on reply */+intret;+structcompletioncompletion;+structlist_headlist;};/**
@@ -801,6 +844,7 @@ static int seccomp_do_user_notification(int this_syscall,u32flags=0;longret=0;structseccomp_knotifn={};+structseccomp_kaddfd*addfd,*tmp;mutex_lock(&match->notify_lock);err=-ENOSYS;
@@ -813,6 +857,7 @@ static int seccomp_do_user_notification(int this_syscall,n.id=seccomp_next_notify_id(match);init_completion(&n.ready);list_add(&n.list,&match->notif->notifications);+INIT_LIST_HEAD(&n.addfd);up(&match->notif->request);wake_up_poll(&match->wqh,EPOLLIN|EPOLLRDNORM);
@@ -821,14 +866,31 @@ static int seccomp_do_user_notification(int this_syscall,/**Thisiswherewewaitforareplyfromuserspace.*/+wait:err=wait_for_completion_interruptible(&n.ready);mutex_lock(&match->notify_lock);if(err==0){+/* Check if we were woken up by a addfd message */+addfd=list_first_entry_or_null(&n.addfd,+structseccomp_kaddfd,list);+if(addfd&&n.state!=SECCOMP_NOTIFY_REPLIED){+seccomp_handle_addfd(addfd);+mutex_unlock(&match->notify_lock);+gotowait;+}ret=n.val;err=n.error;flags=n.flags;}+/* If there were any pending addfd calls, clear them out */+list_for_each_entry_safe(addfd,tmp,&n.addfd,list){+/* The process went away before we got a chance to handle it */+addfd->ret=-ESRCH;+list_del_init(&addfd->list);+complete(&addfd->completion);+}+/**Notethatit'spossiblethelistenerdiedinbetweenthetimewhen*wewerenotifiedofarespons(orasignal)andwhenwewereableto
@@ -1233,12 +1300,107 @@ static long seccomp_notify_id_valid(struct seccomp_filter *filter,returnret;}+staticlongseccomp_notify_addfd(structseccomp_filter*filter,+structseccomp_notif_addfd__user*uaddfd,+unsignedintsize)+{+structseccomp_notif_addfdaddfd;+structseccomp_knotif*knotif;+structseccomp_kaddfdkaddfd;+intret;++/* 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)+returnret;++if(addfd.newfd_flags&~O_CLOEXEC)+return-EINVAL;++if(addfd.flags&~SECCOMP_ADDFD_FLAG_SETFD)+return-EINVAL;++if(addfd.newfd&&!(addfd.flags&SECCOMP_ADDFD_FLAG_SETFD))+return-EINVAL;++kaddfd.file=fget(addfd.srcfd);+if(!kaddfd.file)+return-EBADF;++kaddfd.flags=addfd.newfd_flags;+kaddfd.fd=(addfd.flags&SECCOMP_ADDFD_FLAG_SETFD)?+addfd.newfd:-1;+init_completion(&kaddfd.completion);++ret=mutex_lock_interruptible(&filter->notify_lock);+if(ret<0)+gotoout;++knotif=find_notification(filter,addfd.id);+if(!knotif){+ret=-ENOENT;+gotoout_unlock;+}++/*+*WedonotwanttoallowforFDinjectiontooccurbeforethe+*notificationhasbeenpickedupbyauserspacehandler,orafter+*thenotificationhasbeenrepliedto.+*/+if(knotif->state!=SECCOMP_NOTIFY_SENT){+ret=-EINPROGRESS;+gotoout_unlock;+}++list_add(&kaddfd.list,&knotif->addfd);+complete(&knotif->ready);+mutex_unlock(&filter->notify_lock);++/* Now we wait for it to be processed or be interrupted */+ret=wait_for_completion_interruptible(&kaddfd.completion);+if(ret==0){+/*+*Wehadasuccessfulcompletion.Theothersidehasalready+*removedusfromtheaddfdqueue,and+*wait_for_completion_interruptiblehasamemorybarrierupon+*successthatletsusreadthisvaluedirectlywithout+*locking.+*/+ret=kaddfd.ret;+gotoout;+}++mutex_lock(&filter->notify_lock);+/*+*Eventhoughwewerewokenupbyasignalandnotasuccessful+*completion,acompletionmayhavehappenedinthemeantime.+*+*Weneedtocheckagainiftheaddfdrequesthasbeenhandled,+*andifnot,wewillremoveitfromthequeue.+*/+if(list_empty(&kaddfd.list))+ret=kaddfd.ret;+else+list_del(&kaddfd.list);++out_unlock:+mutex_unlock(&filter->notify_lock);+out:+fput(kaddfd.file);++returnret;+}+staticlongseccomp_notify_ioctl(structfile*file,unsignedintcmd,unsignedlongarg){structseccomp_filter*filter=file->private_data;void__user*buf=(void__user*)arg;+/* Fixed-size ioctls */switch(cmd){caseSECCOMP_IOCTL_NOTIF_RECV:returnseccomp_notify_recv(filter,buf);
From: Sargun Dhillon <redacted>
Test whether we can add file descriptors in response to notifications.
This injects the file descriptors via notifications, and then uses kcmp
to determine whether or not it has been successful.
It also includes some basic sanity checking for arguments.
Signed-off-by: Sargun Dhillon <redacted>
Link: https://lore.kernel.org/r/20200603011044.7972-5-sargun@sargun.me
Co-developed-by: Kees Cook <redacted>
Signed-off-by: Kees Cook <redacted>
---
tools/testing/selftests/seccomp/seccomp_bpf.c | 229 ++++++++++++++++++
1 file changed, 229 insertions(+)
@@ -204,6 +207,39 @@ struct seccomp_notif_sizes {};#endif+#ifndef SECCOMP_IOCTL_NOTIF_ADDFD+/* On success, the return value is the remote process's added fd number */+#define SECCOMP_IOCTL_NOTIF_ADDFD SECCOMP_IOW(3, \+structseccomp_notif_addfd)++/* valid flags for seccomp_notif_addfd */+#define SECCOMP_ADDFD_FLAG_SETFD (1UL << 0) /* Specify remote fd */++structseccomp_notif_addfd{+__u64id;+__u32flags;+__u32srcfd;+__u32newfd;+__u32newfd_flags;+};+#endif++structseccomp_notif_addfd_small{+__u64id;+charweird[4];+};+#define SECCOMP_IOCTL_NOTIF_ADDFD_SMALL \+SECCOMP_IOW(3,structseccomp_notif_addfd_small)++structseccomp_notif_addfd_big{+union{+structseccomp_notif_addfdaddfd;+charbuf[sizeof(structseccomp_notif_addfd)+8];+};+};+#define SECCOMP_IOCTL_NOTIF_ADDFD_BIG \+SECCOMP_IOWR(3,structseccomp_notif_addfd_big)+#ifndef PTRACE_EVENTMSG_SYSCALL_ENTRY#define PTRACE_EVENTMSG_SYSCALL_ENTRY 1#define PTRACE_EVENTMSG_SYSCALL_EXIT 2
@@ -3738,6 +3774,199 @@ TEST(user_notification_filter_empty_threaded)EXPECT_GT((pollfd.revents&POLLHUP)?:0,0);}+TEST(user_notification_addfd)+{+pid_tpid;+longret;+intstatus,listener,memfd,fd;+structseccomp_notif_addfdaddfd={};+structseccomp_notif_addfd_smallsmall={};+structseccomp_notif_addfd_bigbig={};+structseccomp_notifreq={};+structseccomp_notif_respresp={};+/* 100 ms */+structtimespecdelay={.tv_nsec=100000000};++memfd=memfd_create("test",0);+ASSERT_GE(memfd,0);++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!");+}++/* Check that the basic notification machinery works */+listener=user_notif_syscall(__NR_getppid,+SECCOMP_FILTER_FLAG_NEW_LISTENER);+ASSERT_GE(listener,0);++pid=fork();+ASSERT_GE(pid,0);++if(pid==0){+if(syscall(__NR_getppid)!=USER_NOTIF_MAGIC)+exit(1);+exit(syscall(__NR_getppid)!=USER_NOTIF_MAGIC);+}++ASSERT_EQ(ioctl(listener,SECCOMP_IOCTL_NOTIF_RECV,&req),0);++addfd.srcfd=memfd;+addfd.newfd=0;+addfd.id=req.id;+addfd.flags=0x0;++/* Verify bad newfd_flags cannot be set */+addfd.newfd_flags=~O_CLOEXEC;+EXPECT_EQ(ioctl(listener,SECCOMP_IOCTL_NOTIF_ADDFD,&addfd),-1);+EXPECT_EQ(errno,EINVAL);+addfd.newfd_flags=O_CLOEXEC;++/* Verify bad flags cannot be set */+addfd.flags=0xff;+EXPECT_EQ(ioctl(listener,SECCOMP_IOCTL_NOTIF_ADDFD,&addfd),-1);+EXPECT_EQ(errno,EINVAL);+addfd.flags=0;++/* Verify that remote_fd cannot be set without setting flags */+addfd.newfd=1;+EXPECT_EQ(ioctl(listener,SECCOMP_IOCTL_NOTIF_ADDFD,&addfd),-1);+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);+/*+*Thechildhasfds0(stdin),1(stdout),2(stderr),3(memfd),+*4(listener),sothenewlyallocatedfdshouldbe5.+*/+EXPECT_EQ(fd,5);+EXPECT_EQ(filecmp(getpid(),pid,memfd,fd),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);+EXPECT_EQ(filecmp(getpid(),pid,memfd,fd),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);++/*+*ThissetstheIDoftheADDFDtothelastrequestplus1.The+*notificationIDincrements1pernotification.+*/+addfd.id=req.id+1;++/* This spins until the underlying notification is generated */+while(ioctl(listener,SECCOMP_IOCTL_NOTIF_ADDFD,&addfd)!=-1&&+errno!=-EINPROGRESS)+nanosleep(&delay,NULL);++memset(&req,0,sizeof(req));+ASSERT_EQ(ioctl(listener,SECCOMP_IOCTL_NOTIF_RECV,&req),0);+ASSERT_EQ(addfd.id,req.id);++resp.id=req.id;+resp.error=0;+resp.val=USER_NOTIF_MAGIC;+EXPECT_EQ(ioctl(listener,SECCOMP_IOCTL_NOTIF_SEND,&resp),0);++/* Wait for child to finish. */+EXPECT_EQ(waitpid(pid,&status,0),pid);+EXPECT_EQ(true,WIFEXITED(status));+EXPECT_EQ(0,WEXITSTATUS(status));++close(memfd);+}++TEST(user_notification_addfd_rlimit)+{+pid_tpid;+longret;+intstatus,listener,memfd;+structseccomp_notif_addfdaddfd={};+structseccomp_notifreq={};+structseccomp_notif_respresp={};+conststructrlimitlim={+.rlim_cur=0,+.rlim_max=0,+};++memfd=memfd_create("test",0);+ASSERT_GE(memfd,0);++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!");+}++/* Check that the basic notification machinery works */+listener=user_notif_syscall(__NR_getppid,+SECCOMP_FILTER_FLAG_NEW_LISTENER);+ASSERT_GE(listener,0);++pid=fork();+ASSERT_GE(pid,0);++if(pid==0)+exit(syscall(__NR_getppid)!=USER_NOTIF_MAGIC);+++ASSERT_EQ(ioctl(listener,SECCOMP_IOCTL_NOTIF_RECV,&req),0);++ASSERT_EQ(prlimit(pid,RLIMIT_NOFILE,&lim,NULL),0);++addfd.srcfd=memfd;+addfd.newfd_flags=O_CLOEXEC;+addfd.newfd=0;+addfd.id=req.id;+addfd.flags=0;++/* Should probably spot check /proc/sys/fs/file-nr */+EXPECT_EQ(ioctl(listener,SECCOMP_IOCTL_NOTIF_ADDFD,&addfd),-1);+EXPECT_EQ(errno,EMFILE);++addfd.newfd=100;+addfd.flags=SECCOMP_ADDFD_FLAG_SETFD;+EXPECT_EQ(ioctl(listener,SECCOMP_IOCTL_NOTIF_ADDFD,&addfd),-1);+EXPECT_EQ(errno,EBADF);++resp.id=req.id;+resp.error=0;+resp.val=USER_NOTIF_MAGIC;++EXPECT_EQ(ioctl(listener,SECCOMP_IOCTL_NOTIF_SEND,&resp),0);++/* Wait for child to finish. */+EXPECT_EQ(waitpid(pid,&status,0),pid);+EXPECT_EQ(true,WIFEXITED(status));+EXPECT_EQ(0,WEXITSTATUS(status));++close(memfd);+}+/**TODO:*-expandNNPtesting
In preparation for users of the "install a received file" logic outside
of net/ (pidfd and seccomp), relocate and rename __scm_install_fd() from
net/core/scm.c to __receive_fd() in fs/file.c, and provide a wrapper
named receive_fd_user(), as future patches will change the interface
to __receive_fd().
Reviewed-by: Sargun Dhillon <redacted>
Signed-off-by: Kees Cook <redacted>
---
fs/file.c | 48 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/file.h | 8 ++++++++
include/linux/net.h | 9 +++++++++
include/net/scm.h | 1 -
net/compat.c | 2 +-
net/core/scm.c | 32 +----------------------------
6 files changed, 67 insertions(+), 33 deletions(-)
For both pidfd and seccomp, the __user pointer is not used. Update
__receive_fd() to make writing to ufd optional via a NULL check. However,
for the receive_fd_user() wrapper, ufd is NULL checked so an -EFAULT
can be returned to avoid changing the SCM_RIGHTS interface behavior. Add
new wrapper receive_fd() for pidfd and seccomp that does not use the ufd
argument. For the new helper, the allocated fd needs to be returned on
success. Update the existing callers to handle it.
Reviewed-by: Sargun Dhillon <redacted>
Signed-off-by: Kees Cook <redacted>
---
fs/file.c | 23 +++++++++++++++--------
include/linux/file.h | 7 +++++++
net/compat.c | 2 +-
net/core/scm.c | 2 +-
4 files changed, 24 insertions(+), 10 deletions(-)
@@ -963,20 +964,26 @@ int __receive_fd(struct file *file, int __user *ufd, unsigned int o_flags)if(new_fd<0)returnnew_fd;-error=put_user(new_fd,ufd);-if(error){-put_unused_fd(new_fd);-returnerror;+if(ufd){+error=put_user(new_fd,ufd);+if(error){+put_unused_fd(new_fd);+returnerror;+}}-/* Bump the usage count and install the file. */+/*+*Bumptheusagecountandinstallthefile.Theresultingvalueof+*"error"isignoredheresinceweonlyneedtotakeactionwhen+*thefileisasocketandtesting"sock"forNULLissufficient.+*/sock=sock_from_file(file,&error);if(sock){sock_update_netprioidx(&sock->sk->sk_cgrp_data);sock_update_classid(&sock->sk->sk_cgrp_data);}fd_install(new_fd,get_file(file));-return0;+returnnew_fd;}staticintksys_dup3(unsignedintoldfd,unsignedintnewfd,intflags)
Duplicate the cleanups from commit 2618d530dd8b ("net/scm: cleanup
scm_detach_fds") into the compat code.
Move the check added in commit 1f466e1f15cf ("net: cleanly handle kernel
vs user buffers for ->msg_control") to before the compat call, even
though it should be impossible for an in-kernel call to also be compat.
Correct the int "flags" argument to unsigned int to match fd_install()
and similar APIs.
Regularize any remaining differences, including a whitespace issue,
a checkpatch warning, and add the check from commit 6900317f5eff ("net,
scm: fix PaX detected msg_controllen overflow in scm_detach_fds") which
fixed an overflow unique to 64-bit. To avoid confusion when comparing
the compat handler to the native handler, just include the same check
in the compat handler.
Fixes: 48a87cc26c13 ("net: netprio: fd passed in SCM_RIGHTS datagram not set correctly")
Fixes: d84295067fc7 ("net: net_cls: fd passed in SCM_RIGHTS datagram not set correctly")
Signed-off-by: Kees Cook <redacted>
---
include/net/scm.h | 1 +
net/compat.c | 55 +++++++++++++++++++++--------------------------
net/core/scm.c | 18 ++++++++--------
3 files changed, 35 insertions(+), 39 deletions(-)
@@ -281,39 +281,31 @@ int put_cmsg_compat(struct msghdr *kmsg, int level, int type, int len, void *datreturn0;}-voidscm_detach_fds_compat(structmsghdr*kmsg,structscm_cookie*scm)+staticintscm_max_fds_compat(structmsghdr*msg){-structcompat_cmsghdr__user*cm=(structcompat_cmsghdr__user*)kmsg->msg_control;-intfdmax=(kmsg->msg_controllen-sizeof(structcompat_cmsghdr))/sizeof(int);-intfdnum=scm->fp->count;-structfile**fp=scm->fp->fp;-int__user*cmfptr;-interr=0,i;+if(msg->msg_controllen<=sizeof(structcompat_cmsghdr))+return0;+return(msg->msg_controllen-sizeof(structcompat_cmsghdr))/sizeof(int);+}-if(fdnum<fdmax)-fdmax=fdnum;+voidscm_detach_fds_compat(structmsghdr*msg,structscm_cookie*scm)+{+structcompat_cmsghdr__user*cm=+(structcompat_cmsghdr__user*)msg->msg_control;+unsignedinto_flags=(msg->msg_flags&MSG_CMSG_CLOEXEC)?O_CLOEXEC:0;+intfdmax=min_t(int,scm_max_fds_compat(msg),scm->fp->count);+int__user*cmsg_data=CMSG_USER_DATA(cm);+interr=0,i;-for(i=0,cmfptr=(int__user*)CMSG_COMPAT_DATA(cm);i<fdmax;i++,cmfptr++){-intnew_fd;-err=security_file_receive(fp[i]);+for(i=0;i<fdmax;i++){+err=__scm_install_fd(scm->fp->fp[i],cmsg_data+i,o_flags);if(err)break;-err=get_unused_fd_flags(MSG_CMSG_CLOEXEC&kmsg->msg_flags-?O_CLOEXEC:0);-if(err<0)-break;-new_fd=err;-err=put_user(new_fd,cmfptr);-if(err){-put_unused_fd(new_fd);-break;-}-/* Bump the usage count and install the file. */-fd_install(new_fd,get_file(fp[i]));}if(i>0){intcmlen=CMSG_COMPAT_LEN(i*sizeof(int));+err=put_user(SOL_SOCKET,&cm->cmsg_level);if(!err)err=put_user(SCM_RIGHTS,&cm->cmsg_type);
@@ -319,29 +319,29 @@ static int scm_max_fds(struct msghdr *msg)voidscm_detach_fds(structmsghdr*msg,structscm_cookie*scm){-structcmsghdr__user*cm-=(__forcestructcmsghdr__user*)msg->msg_control;-into_flags=(msg->msg_flags&MSG_CMSG_CLOEXEC)?O_CLOEXEC:0;+structcmsghdr__user*cm=+(__forcestructcmsghdr__user*)msg->msg_control;+unsignedinto_flags=(msg->msg_flags&MSG_CMSG_CLOEXEC)?O_CLOEXEC:0;intfdmax=min_t(int,scm_max_fds(msg),scm->fp->count);int__user*cmsg_data=CMSG_USER_DATA(cm);interr=0,i;+/* no use for FD passing from kernel space callers */+if(WARN_ON_ONCE(!msg->msg_control_is_user))+return;+if(msg->msg_flags&MSG_CMSG_COMPAT){scm_detach_fds_compat(msg,scm);return;}-/* no use for FD passing from kernel space callers */-if(WARN_ON_ONCE(!msg->msg_control_is_user))-return;-for(i=0;i<fdmax;i++){err=__scm_install_fd(scm->fp->fp[i],cmsg_data+i,o_flags);if(err)break;}-if(i>0){+if(i>0){intcmlen=CMSG_LEN(i*sizeof(int));err=put_user(SOL_SOCKET,&cm->cmsg_level);
From: Christoph Hellwig <hch@lst.de> Date: 2020-07-07 06:49:27
On Mon, Jul 06, 2020 at 01:17:15PM -0700, Kees Cook wrote:
In preparation for users of the "install a received file" logic outside
of net/ (pidfd and seccomp), relocate and rename __scm_install_fd() from
net/core/scm.c to __receive_fd() in fs/file.c, and provide a wrapper
named receive_fd_user(), as future patches will change the interface
to __receive_fd().
From: Christoph Hellwig <hch@lst.de> Date: 2020-07-07 06:49:43
On Mon, Jul 06, 2020 at 01:17:16PM -0700, Kees Cook wrote:
For both pidfd and seccomp, the __user pointer is not used. Update
__receive_fd() to make writing to ufd optional via a NULL check. However,
for the receive_fd_user() wrapper, ufd is NULL checked so an -EFAULT
can be returned to avoid changing the SCM_RIGHTS interface behavior. Add
new wrapper receive_fd() for pidfd and seccomp that does not use the ufd
argument. For the new helper, the allocated fd needs to be returned on
success. Update the existing callers to handle it.
From: Christian Brauner <hidden> Date: 2020-07-07 11:41:12
On Mon, Jul 06, 2020 at 01:17:14PM -0700, Kees Cook wrote:
Duplicate the cleanups from commit 2618d530dd8b ("net/scm: cleanup
scm_detach_fds") into the compat code.
Move the check added in commit 1f466e1f15cf ("net: cleanly handle kernel
vs user buffers for ->msg_control") to before the compat call, even
though it should be impossible for an in-kernel call to also be compat.
Correct the int "flags" argument to unsigned int to match fd_install()
and similar APIs.
Regularize any remaining differences, including a whitespace issue,
a checkpatch warning, and add the check from commit 6900317f5eff ("net,
scm: fix PaX detected msg_controllen overflow in scm_detach_fds") which
fixed an overflow unique to 64-bit. To avoid confusion when comparing
the compat handler to the native handler, just include the same check
in the compat handler.
Fixes: 48a87cc26c13 ("net: netprio: fd passed in SCM_RIGHTS datagram not set correctly")
Fixes: d84295067fc7 ("net: net_cls: fd passed in SCM_RIGHTS datagram not set correctly")
Signed-off-by: Kees Cook <redacted>
---
Thanks. Just a comment below.
Acked-by: Christian Brauner <redacted>
@@ -281,39 +281,31 @@ int put_cmsg_compat(struct msghdr *kmsg, int level, int type, int len, void *datreturn0;}-voidscm_detach_fds_compat(structmsghdr*kmsg,structscm_cookie*scm)+staticintscm_max_fds_compat(structmsghdr*msg){-structcompat_cmsghdr__user*cm=(structcompat_cmsghdr__user*)kmsg->msg_control;-intfdmax=(kmsg->msg_controllen-sizeof(structcompat_cmsghdr))/sizeof(int);-intfdnum=scm->fp->count;-structfile**fp=scm->fp->fp;-int__user*cmfptr;-interr=0,i;+if(msg->msg_controllen<=sizeof(structcompat_cmsghdr))+return0;+return(msg->msg_controllen-sizeof(structcompat_cmsghdr))/sizeof(int);+}-if(fdnum<fdmax)-fdmax=fdnum;+voidscm_detach_fds_compat(structmsghdr*msg,structscm_cookie*scm)+{+structcompat_cmsghdr__user*cm=+(structcompat_cmsghdr__user*)msg->msg_control;+unsignedinto_flags=(msg->msg_flags&MSG_CMSG_CLOEXEC)?O_CLOEXEC:0;+intfdmax=min_t(int,scm_max_fds_compat(msg),scm->fp->count);
Just a note that SCM_RIGHTS fd-sending is limited to 253 (SCM_MAX_FD)
fds so min_t should never ouput > SCM_MAX_FD here afaict.
quoted hunk
+ int __user *cmsg_data = CMSG_USER_DATA(cm);
+ int err = 0, i;
- for (i = 0, cmfptr = (int __user *) CMSG_COMPAT_DATA(cm); i < fdmax; i++, cmfptr++) {
- int new_fd;
- err = security_file_receive(fp[i]);
+ for (i = 0; i < fdmax; i++) {
+ err = __scm_install_fd(scm->fp->fp[i], cmsg_data + i, o_flags);
if (err)
break;
- err = get_unused_fd_flags(MSG_CMSG_CLOEXEC & kmsg->msg_flags
- ? O_CLOEXEC : 0);
- if (err < 0)
- break;
- new_fd = err;
- err = put_user(new_fd, cmfptr);
- if (err) {
- put_unused_fd(new_fd);
- break;
- }
- /* Bump the usage count and install the file. */
- fd_install(new_fd, get_file(fp[i]));
}
if (i > 0) {
int cmlen = CMSG_COMPAT_LEN(i * sizeof(int));
+
err = put_user(SOL_SOCKET, &cm->cmsg_level);
if (!err)
err = put_user(SCM_RIGHTS, &cm->cmsg_type);
I think fdmax can't be < 0 after your changes? scm_max_fds() guarantees
that fdmax is always >= 0 and min_t() guarantees that fdmax <= scm->fp->count.
So the check should technically be :)
if (i < scm->fp->count || scm->fp->count && fdmax == 0)
quoted hunk
+ msg->msg_flags |= MSG_CTRUNC;
/*
- * All of the files that fit in the message have had their
- * usage counts incremented, so we just free the list.
+ * All of the files that fit in the message have had their usage counts
+ * incremented, so we just free the list.
*/
__scm_destroy(scm);
}
@@ -319,29 +319,29 @@ static int scm_max_fds(struct msghdr *msg)voidscm_detach_fds(structmsghdr*msg,structscm_cookie*scm){-structcmsghdr__user*cm-=(__forcestructcmsghdr__user*)msg->msg_control;-into_flags=(msg->msg_flags&MSG_CMSG_CLOEXEC)?O_CLOEXEC:0;+structcmsghdr__user*cm=+(__forcestructcmsghdr__user*)msg->msg_control;+unsignedinto_flags=(msg->msg_flags&MSG_CMSG_CLOEXEC)?O_CLOEXEC:0;intfdmax=min_t(int,scm_max_fds(msg),scm->fp->count);int__user*cmsg_data=CMSG_USER_DATA(cm);interr=0,i;+/* no use for FD passing from kernel space callers */+if(WARN_ON_ONCE(!msg->msg_control_is_user))+return;+if(msg->msg_flags&MSG_CMSG_COMPAT){scm_detach_fds_compat(msg,scm);return;}-/* no use for FD passing from kernel space callers */-if(WARN_ON_ONCE(!msg->msg_control_is_user))-return;-for(i=0;i<fdmax;i++){err=__scm_install_fd(scm->fp->fp[i],cmsg_data+i,o_flags);if(err)break;}-if(i>0){+if(i>0){intcmlen=CMSG_LEN(i*sizeof(int));err=put_user(SOL_SOCKET,&cm->cmsg_level);
From: Christian Brauner <hidden> Date: 2020-07-07 11:46:14
On Mon, Jul 06, 2020 at 01:17:15PM -0700, Kees Cook wrote:
In preparation for users of the "install a received file" logic outside
of net/ (pidfd and seccomp), relocate and rename __scm_install_fd() from
net/core/scm.c to __receive_fd() in fs/file.c, and provide a wrapper
named receive_fd_user(), as future patches will change the interface
to __receive_fd().
Reviewed-by: Sargun Dhillon <redacted>
Signed-off-by: Kees Cook <redacted>
---
From: Christian Brauner <hidden> Date: 2020-07-07 11:49:34
On Mon, Jul 06, 2020 at 01:17:16PM -0700, Kees Cook wrote:
For both pidfd and seccomp, the __user pointer is not used. Update
__receive_fd() to make writing to ufd optional via a NULL check. However,
for the receive_fd_user() wrapper, ufd is NULL checked so an -EFAULT
can be returned to avoid changing the SCM_RIGHTS interface behavior. Add
new wrapper receive_fd() for pidfd and seccomp that does not use the ufd
argument. For the new helper, the allocated fd needs to be returned on
success. Update the existing callers to handle it.
Reviewed-by: Sargun Dhillon <redacted>
Signed-off-by: Kees Cook <redacted>
---
Hm, I'm not sure why 2/7 and 3/7 aren't just one patch but ok. :)
Acked-by: Christian Brauner <redacted>
From: Christian Brauner <hidden> Date: 2020-07-07 12:22:31
On Mon, Jul 06, 2020 at 01:17:17PM -0700, Kees Cook wrote:
The sock counting (sock_update_netprioidx() and sock_update_classid()) was
missing from pidfd's implementation of received fd installation. Replace
the open-coded version with a call to the new receive_fd()
helper.
Thanks to Vamshi K Sthambamkadi [off-list ref] for
catching a missed fput() in an earlier version of this patch.
Fixes: 8649c322f75c ("pid: Implement pidfd_getfd syscall")
Reviewed-by: Sargun Dhillon <redacted>
Signed-off-by: Kees Cook <redacted>
---
Thanks!
Acked-by: Christian Brauner <redacted>
Christoph, Kees,
So while the patch is correct it leaves 5.6 and 5.7 with a bug in the
pidfd_getfd() implementation and that just doesn't seem right. I'm
wondering whether we should introduce:
void sock_update(struct file *file)
{
struct socket *sock;
int error;
sock = sock_from_file(file, &error);
if (sock) {
sock_update_netprioidx(&sock->sk->sk_cgrp_data);
sock_update_classid(&sock->sk->sk_cgrp_data);
}
}
and switch pidfd_getfd() over to:
@@ -642,10 +642,12 @@ static int pidfd_getfd(struct pid *pid, int fd)}ret=get_unused_fd_flags(O_CLOEXEC);-if(ret<0)+if(ret<0){fput(file);-else+}else{+sock_update(file);fd_install(ret,file);+}returnret;}
first thing in the series and then all of the other patches on top of it
so that we can Cc stable for this and that can get it backported to 5.6,
5.7, and 5.8.
Alternatively, I can make this a separate bugfix patch series which I'll
send upstream soonish. Or we have specific patches just for 5.6, 5.7,
and 5.8. Thoughts?
Thanks!
Christian
From: Christian Brauner <hidden> Date: 2020-07-07 12:39:04
On Mon, Jul 06, 2020 at 01:17:18PM -0700, Kees Cook wrote:
Expand __receive_fd() with support for replace_fd() for the coming seccomp
"addfd" ioctl(). Add new wrapper receive_fd_replace() for the new behavior
and update existing wrappers to retain old behavior.
Thanks to Colin Ian King [off-list ref] for pointing out an
uninitialized variable exposure in an earlier version of this patch.
Reviewed-by: Sargun Dhillon <redacted>
Signed-off-by: Kees Cook <redacted>
---
Thanks!
(One tiny-nit below.)
Acked-by: Christian Brauner <redacted>
@@ -960,18 +961,30 @@ int __receive_fd(struct file *file, int __user *ufd, unsigned int o_flags)if(error)returnerror;-new_fd=get_unused_fd_flags(o_flags);-if(new_fd<0)-returnnew_fd;+if(fd<0){+new_fd=get_unused_fd_flags(o_flags);+if(new_fd<0)+returnnew_fd;+}else+new_fd=fd;
This is nitpicky but coding style technically wants us to use braces
around both branches if one of them requires them. ;)
quoted hunk
if (ufd) {
error = put_user(new_fd, ufd);
if (error) {
- put_unused_fd(new_fd);
+ if (fd < 0)
+ put_unused_fd(new_fd);
return error;
}
}
+ if (fd < 0)
+ fd_install(new_fd, get_file(file));
+ else {
+ error = replace_fd(new_fd, file, o_flags);
+ if (error)
+ return error;
+ }
+
/*
* Bump the usage count and install the file. The resulting value of
* "error" is ignored here since we only need to take action when
@@ -982,7 +995,6 @@ int __receive_fd(struct file *file, int __user *ufd, unsigned int o_flags) sock_update_netprioidx(&sock->sk->sk_cgrp_data); sock_update_classid(&sock->sk->sk_cgrp_data); }- fd_install(new_fd, get_file(file)); return new_fd; }
From: Christian Brauner <hidden> Date: 2020-07-07 13:31:04
On Mon, Jul 06, 2020 at 01:17:19PM -0700, Kees Cook wrote:
From: Sargun Dhillon <redacted>
This adds a seccomp notifier ioctl which allows for the listener to
"add" file descriptors to a process which originated a seccomp user
notification. This allows calls like mount, and mknod to be "implemented",
This either seems like the wrong description as we're intercepting
mount() and mknod() already and as far as I can tell, addfd is not
needed in any way for them. addfd is targeted at system calls that
themselves would install a file descriptor in the intercepted task, e.g.
open() and accept().
as the return value, and the arguments are data in memory. On the other
hand, calls like connect can be "implemented" using pidfd_getfd.
Unfortunately, there are calls which return file descriptors, like
open, which are vulnerable to ToCToU attacks, and require that the more
Hm, the race is not specific to open though but any syscall and
specifically its bad for those syscalls with pointer arguments where the
supervisor copies out the memory, inspects it, and bases its decision on
them.
privileged supervisor can inspect the argument, and perform the syscall
on behalf of the process generating the notification. This allows the
file descriptor generated from that open call to be returned to the
calling process.
Hm, maybe change that description to sm like:
The seccomp notifier which was introduced a while back allows for
syscall supervision. It is often used in settings where a supervisising
task emulates syscalls for a supervised task in userspace either to
further restrict it's syscall capabilities or to circumvent kernel
enforced restrictions the supervisor deems safe to lift.
While the seccomp notifier allows for the interception of any syscall
only a certain set of syscalls can be correctly emulated. Over the last
cycles we have done work to reduce the set of syscalls which can't be
emulated with the addition of pidfd_getfd(). With this syscall we are
now able to e.g. intercept syscalls that require the supervisor to
operate on file descriptors of the supervisee such as connect().
However, syscalls that cause new file descriptors to be installed in the
supervisor can currently not be correctly emulated since there is no way
for the supervisor to inject file descriptors into the supervisee. The
new addfd ioctl removes this restriction by allowing the supervisor to
install file descriptors into the intercepted task. By implementing this
feature via seccomp the supervisor effectively instructs the supervisee
to install a set of file descriptors into its own file descriptor table.
With the new addfd extension it is possible to e.g. intercept syscalls
such as open() or accept(). The addfd ioctl allows allows to replace
existing file descriptors in the supervisee. One use-case would be to
replace stdout and stderr of a supervisee with a log file descriptor to
record the supervisees output.
quoted hunk
In addition, there is functionality to allow for replacement of specific
file descriptors, following dup2-like semantics.
The ioctl handling is based on the discussions[1] of how Extensible
Arguments should interact with ioctls. 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.
As a note, the seccomp_notif_addfd structure is laid out based on 8-byte
alignment without requiring packing as there have been packing issues
with uapi highlighted before[1][2]. Although we could overload the
newfd field and use -1 to indicate that it is not to be used, doing
so requires changing the size of the fd field, and introduces struct
packing complexity.
[1]: https://lore.kernel.org/lkml/87o8w9bcaf.fsf@mid.deneb.enyo.de/
[2]: https://lore.kernel.org/lkml/a328b91d-fd8f-4f27-b3c2-91a9c45f18c0@rasmusvillemoes.dk/
[3]: https://lore.kernel.org/lkml/20200612104629.GA15814@ircssh-2.c.rugged-nimbus-611.internal
Suggested-by: Matt Denton <redacted>
Link: https://lore.kernel.org/r/20200603011044.7972-4-sargun@sargun.me
Signed-off-by: Sargun Dhillon <redacted>
Co-developed-by: Kees Cook <redacted>
Signed-off-by: Kees Cook <redacted>
---
include/uapi/linux/seccomp.h | 22 +++++
kernel/seccomp.c | 172 ++++++++++++++++++++++++++++++++++-
2 files changed, 193 insertions(+), 1 deletion(-)
@@ -87,10 +87,42 @@ struct seccomp_knotif {longval;u32flags;-/* Signals when this has entered SECCOMP_NOTIFY_REPLIED */+/*+*Signalswhenthishaschangedstates,suchasthelistener+*dying,anewseccompaddfdmessage,orchangingtoREPLIED+*/structcompletionready;structlist_headlist;++/* outstanding addfd requests */+structlist_headaddfd;+};++/**+*structseccomp_kaddfd-containerforseccomp_addfdioctlmessages+*+*@file:Areferencetothefiletoinstallintheothertask+*@fd:Thefdnumbertoinstallitat.Ifthefdnumberis-1,itmeansthe+*installingprocessshouldallocatethefdasnormal.+*@flags:Theflagsforthenewfiledescriptor.Atthemoment,onlyO_CLOEXEC+*isallowed.+*@ret:Thereturnvalueoftheinstallingprocess.Itissettothefdnum+*uponsuccess(>=0).+*@completion:Indicatesthattheinstallingprocesshascompletedfd+*installation,orgoneaway(eitherduetosuccessful+*reply,orsignal)+*+*/+structseccomp_kaddfd{+structfile*file;+intfd;+unsignedintflags;++/* To only be set on reply */+intret;+structcompletioncompletion;+structlist_headlist;};/**
@@ -801,6 +844,7 @@ static int seccomp_do_user_notification(int this_syscall,u32flags=0;longret=0;structseccomp_knotifn={};+structseccomp_kaddfd*addfd,*tmp;mutex_lock(&match->notify_lock);err=-ENOSYS;
@@ -813,6 +857,7 @@ static int seccomp_do_user_notification(int this_syscall,n.id=seccomp_next_notify_id(match);init_completion(&n.ready);list_add(&n.list,&match->notif->notifications);+INIT_LIST_HEAD(&n.addfd);up(&match->notif->request);wake_up_poll(&match->wqh,EPOLLIN|EPOLLRDNORM);
@@ -821,14 +866,31 @@ static int seccomp_do_user_notification(int this_syscall,/**Thisiswherewewaitforareplyfromuserspace.*/+wait:err=wait_for_completion_interruptible(&n.ready);mutex_lock(&match->notify_lock);if(err==0){+/* Check if we were woken up by a addfd message */+addfd=list_first_entry_or_null(&n.addfd,+structseccomp_kaddfd,list);+if(addfd&&n.state!=SECCOMP_NOTIFY_REPLIED){+seccomp_handle_addfd(addfd);+mutex_unlock(&match->notify_lock);+gotowait;+}ret=n.val;err=n.error;flags=n.flags;}+/* If there were any pending addfd calls, clear them out */+list_for_each_entry_safe(addfd,tmp,&n.addfd,list){+/* The process went away before we got a chance to handle it */+addfd->ret=-ESRCH;+list_del_init(&addfd->list);+complete(&addfd->completion);+}+/**Notethatit'spossiblethelistenerdiedinbetweenthetimewhen*wewerenotifiedofarespons(orasignal)andwhenwewereableto
@@ -1233,12 +1300,107 @@ static long seccomp_notify_id_valid(struct seccomp_filter *filter,returnret;}+staticlongseccomp_notify_addfd(structseccomp_filter*filter,+structseccomp_notif_addfd__user*uaddfd,+unsignedintsize)+{+structseccomp_notif_addfdaddfd;+structseccomp_knotif*knotif;+structseccomp_kaddfdkaddfd;+intret;++/* 24 is original sizeof(struct seccomp_notif_addfd) */+if(size<24||size>=PAGE_SIZE)+return-EINVAL;
Hm, so maybe add the following:
#define SECCOMP_NOTIFY_ADDFD_VER0 24
#define SECCOMP_NOTIFY_ADDFD_LATEST SECCOMP_NOTIFY_ADDFD_VER0
and then place:
BUILD_BUG_ON(sizeof(struct seccomp_notify_addfd) < SECCOMP_NOTIFY_ADDFD_VER0);
BUILD_BUG_ON(sizeof(struct open_how) != SECCOMP_NOTIFY_ADDFD_LATEST);
somewhere which is what we do for clone3(), openat2() and others to
catch build-time nonsense.
include/uapi/linux/perf_event.h:#define PERF_ATTR_SIZE_VER0 64 /* sizeof first published struct */
include/uapi/linux/sched.h:#define CLONE_ARGS_SIZE_VER0 64 /* sizeof first published struct */
include/uapi/linux/sched/types.h:#define SCHED_ATTR_SIZE_VER0 48 /* sizeof first published struct */
include/linux/fcntl.h:#define OPEN_HOW_SIZE_VER0 24 /* sizeof first published struct */
include/linux/fcntl.h:#define OPEN_HOW_SIZE_LATEST OPEN_HOW_SIZE_VER0
quoted hunk
+
+ ret = copy_struct_from_user(&addfd, sizeof(addfd), uaddfd, size);
+ if (ret)
+ return ret;
+
+ if (addfd.newfd_flags & ~O_CLOEXEC)
+ return -EINVAL;
+
+ if (addfd.flags & ~SECCOMP_ADDFD_FLAG_SETFD)
+ return -EINVAL;
+
+ if (addfd.newfd && !(addfd.flags & SECCOMP_ADDFD_FLAG_SETFD))
+ return -EINVAL;
+
+ kaddfd.file = fget(addfd.srcfd);
+ if (!kaddfd.file)
+ return -EBADF;
+
+ kaddfd.flags = addfd.newfd_flags;
+ kaddfd.fd = (addfd.flags & SECCOMP_ADDFD_FLAG_SETFD) ?
+ addfd.newfd : -1;
+ init_completion(&kaddfd.completion);
+
+ ret = mutex_lock_interruptible(&filter->notify_lock);
+ if (ret < 0)
+ goto out;
+
+ knotif = find_notification(filter, addfd.id);
+ if (!knotif) {
+ ret = -ENOENT;
+ goto out_unlock;
+ }
+
+ /*
+ * We do not want to allow for FD injection to occur before the
+ * notification has been picked up by a userspace handler, or after
+ * the notification has been replied to.
+ */
+ if (knotif->state != SECCOMP_NOTIFY_SENT) {
+ ret = -EINPROGRESS;
+ goto out_unlock;
+ }
+
+ list_add(&kaddfd.list, &knotif->addfd);
+ complete(&knotif->ready);
+ mutex_unlock(&filter->notify_lock);
+
+ /* Now we wait for it to be processed or be interrupted */
+ ret = wait_for_completion_interruptible(&kaddfd.completion);
+ if (ret == 0) {
+ /*
+ * We had a successful completion. The other side has already
+ * removed us from the addfd queue, and
+ * wait_for_completion_interruptible has a memory barrier upon
+ * success that lets us read this value directly without
+ * locking.
+ */
+ ret = kaddfd.ret;
+ goto out;
+ }
+
+ mutex_lock(&filter->notify_lock);
+ /*
+ * Even though we were woken up by a signal and not a successful
+ * completion, a completion may have happened in the mean time.
+ *
+ * We need to check again if the addfd request has been handled,
+ * and if not, we will remove it from the queue.
+ */
+ if (list_empty(&kaddfd.list))
+ ret = kaddfd.ret;
+ else
+ list_del(&kaddfd.list);
+
+out_unlock:
+ mutex_unlock(&filter->notify_lock);
+out:
+ fput(kaddfd.file);
+
+ return ret;
+}
+
static long seccomp_notify_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
{
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);
@@ -1247,9 +1409,17 @@ static long seccomp_notify_ioctl(struct file *file, unsigned int cmd, case SECCOMP_IOCTL_NOTIF_ID_VALID_WRONG_DIR: case SECCOMP_IOCTL_NOTIF_ID_VALID: return seccomp_notify_id_valid(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
On Tue, Jul 07, 2020 at 01:41:03PM +0200, Christian Brauner wrote:
On Mon, Jul 06, 2020 at 01:17:14PM -0700, Kees Cook wrote:
quoted
Duplicate the cleanups from commit 2618d530dd8b ("net/scm: cleanup
scm_detach_fds") into the compat code.
Move the check added in commit 1f466e1f15cf ("net: cleanly handle kernel
vs user buffers for ->msg_control") to before the compat call, even
though it should be impossible for an in-kernel call to also be compat.
Correct the int "flags" argument to unsigned int to match fd_install()
and similar APIs.
Regularize any remaining differences, including a whitespace issue,
a checkpatch warning, and add the check from commit 6900317f5eff ("net,
scm: fix PaX detected msg_controllen overflow in scm_detach_fds") which
fixed an overflow unique to 64-bit. To avoid confusion when comparing
the compat handler to the native handler, just include the same check
in the compat handler.
Fixes: 48a87cc26c13 ("net: netprio: fd passed in SCM_RIGHTS datagram not set correctly")
Fixes: d84295067fc7 ("net: net_cls: fd passed in SCM_RIGHTS datagram not set correctly")
Signed-off-by: Kees Cook <redacted>
---
Thanks. Just a comment below.
Acked-by: Christian Brauner <redacted>
@@ -281,39 +281,31 @@ int put_cmsg_compat(struct msghdr *kmsg, int level, int type, int len, void *datreturn0;}-voidscm_detach_fds_compat(structmsghdr*kmsg,structscm_cookie*scm)+staticintscm_max_fds_compat(structmsghdr*msg){-structcompat_cmsghdr__user*cm=(structcompat_cmsghdr__user*)kmsg->msg_control;-intfdmax=(kmsg->msg_controllen-sizeof(structcompat_cmsghdr))/sizeof(int);-intfdnum=scm->fp->count;-structfile**fp=scm->fp->fp;-int__user*cmfptr;-interr=0,i;+if(msg->msg_controllen<=sizeof(structcompat_cmsghdr))+return0;+return(msg->msg_controllen-sizeof(structcompat_cmsghdr))/sizeof(int);+}-if(fdnum<fdmax)-fdmax=fdnum;+voidscm_detach_fds_compat(structmsghdr*msg,structscm_cookie*scm)+{+structcompat_cmsghdr__user*cm=+(structcompat_cmsghdr__user*)msg->msg_control;+unsignedinto_flags=(msg->msg_flags&MSG_CMSG_CLOEXEC)?O_CLOEXEC:0;+intfdmax=min_t(int,scm_max_fds_compat(msg),scm->fp->count);
Just a note that SCM_RIGHTS fd-sending is limited to 253 (SCM_MAX_FD)
fds so min_t should never ouput > SCM_MAX_FD here afaict.
quoted
+ int __user *cmsg_data = CMSG_USER_DATA(cm);
+ int err = 0, i;
- for (i = 0, cmfptr = (int __user *) CMSG_COMPAT_DATA(cm); i < fdmax; i++, cmfptr++) {
- int new_fd;
- err = security_file_receive(fp[i]);
+ for (i = 0; i < fdmax; i++) {
+ err = __scm_install_fd(scm->fp->fp[i], cmsg_data + i, o_flags);
if (err)
break;
- err = get_unused_fd_flags(MSG_CMSG_CLOEXEC & kmsg->msg_flags
- ? O_CLOEXEC : 0);
- if (err < 0)
- break;
- new_fd = err;
- err = put_user(new_fd, cmfptr);
- if (err) {
- put_unused_fd(new_fd);
- break;
- }
- /* Bump the usage count and install the file. */
- fd_install(new_fd, get_file(fp[i]));
}
if (i > 0) {
int cmlen = CMSG_COMPAT_LEN(i * sizeof(int));
+
err = put_user(SOL_SOCKET, &cm->cmsg_level);
if (!err)
err = put_user(SCM_RIGHTS, &cm->cmsg_type);
I think fdmax can't be < 0 after your changes? scm_max_fds() guarantees
that fdmax is always >= 0 and min_t() guarantees that fdmax <= scm->fp->count.
So the check should technically be :)
You left our your suggestion! :) But, I think you mean "== 0" ?
The check actually comes from the refactoring from commit 2618d530dd8b
("net/scm: cleanup scm_detach_fds") which I mostly copy/pasted into
compat. However, fdmax is an int, and scm->fp->count is signed so it's
possible fdmax is < 0 (but I don't think count can actually ever be <
0), but I don't want to refactor all the types just to fix this boundary
condition. :)
--
Kees Cook
On Tue, Jul 07, 2020 at 01:49:23PM +0200, Christian Brauner wrote:
On Mon, Jul 06, 2020 at 01:17:16PM -0700, Kees Cook wrote:
quoted
For both pidfd and seccomp, the __user pointer is not used. Update
__receive_fd() to make writing to ufd optional via a NULL check. However,
for the receive_fd_user() wrapper, ufd is NULL checked so an -EFAULT
can be returned to avoid changing the SCM_RIGHTS interface behavior. Add
new wrapper receive_fd() for pidfd and seccomp that does not use the ufd
argument. For the new helper, the allocated fd needs to be returned on
success. Update the existing callers to handle it.
Reviewed-by: Sargun Dhillon <redacted>
Signed-off-by: Kees Cook <redacted>
---
Hm, I'm not sure why 2/7 and 3/7 aren't just one patch but ok. :)
I wanted to do a "clean" move from one source to another without any
behavioral changes first.
On Tue, Jul 07, 2020 at 02:22:20PM +0200, Christian Brauner wrote:
quoted hunk
On Mon, Jul 06, 2020 at 01:17:17PM -0700, Kees Cook wrote:
quoted
The sock counting (sock_update_netprioidx() and sock_update_classid()) was
missing from pidfd's implementation of received fd installation. Replace
the open-coded version with a call to the new receive_fd()
helper.
Thanks to Vamshi K Sthambamkadi [off-list ref] for
catching a missed fput() in an earlier version of this patch.
Fixes: 8649c322f75c ("pid: Implement pidfd_getfd syscall")
Reviewed-by: Sargun Dhillon <redacted>
Signed-off-by: Kees Cook <redacted>
---
Thanks!
Acked-by: Christian Brauner <redacted>
Christoph, Kees,
So while the patch is correct it leaves 5.6 and 5.7 with a bug in the
pidfd_getfd() implementation and that just doesn't seem right. I'm
wondering whether we should introduce:
void sock_update(struct file *file)
{
struct socket *sock;
int error;
sock = sock_from_file(file, &error);
if (sock) {
sock_update_netprioidx(&sock->sk->sk_cgrp_data);
sock_update_classid(&sock->sk->sk_cgrp_data);
}
}
and switch pidfd_getfd() over to:
@@ -642,10 +642,12 @@ static int pidfd_getfd(struct pid *pid, int fd)}ret=get_unused_fd_flags(O_CLOEXEC);-if(ret<0)+if(ret<0){fput(file);-else+}else{+sock_update(file);fd_install(ret,file);+}returnret;}
first thing in the series and then all of the other patches on top of it
so that we can Cc stable for this and that can get it backported to 5.6,
5.7, and 5.8.
Alternatively, I can make this a separate bugfix patch series which I'll
send upstream soonish. Or we have specific patches just for 5.6, 5.7,
and 5.8. Thoughts?
I was thinking of just tossing the entire series (hch's and mine) at
-stable since it's relatively narrow. I'll look at what's needed for
backports...
On Tue, Jul 07, 2020 at 02:38:54PM +0200, Christian Brauner wrote:
On Mon, Jul 06, 2020 at 01:17:18PM -0700, Kees Cook wrote:
quoted
Expand __receive_fd() with support for replace_fd() for the coming seccomp
"addfd" ioctl(). Add new wrapper receive_fd_replace() for the new behavior
and update existing wrappers to retain old behavior.
Thanks to Colin Ian King [off-list ref] for pointing out an
uninitialized variable exposure in an earlier version of this patch.
Reviewed-by: Sargun Dhillon <redacted>
Signed-off-by: Kees Cook <redacted>
---
Thanks!
(One tiny-nit below.)
Acked-by: Christian Brauner <redacted>
@@ -960,18 +961,30 @@ int __receive_fd(struct file *file, int __user *ufd, unsigned int o_flags)if(error)returnerror;-new_fd=get_unused_fd_flags(o_flags);-if(new_fd<0)-returnnew_fd;+if(fd<0){+new_fd=get_unused_fd_flags(o_flags);+if(new_fd<0)+returnnew_fd;+}else+new_fd=fd;
This is nitpicky but coding style technically wants us to use braces
around both branches if one of them requires them. ;)
On Tue, Jul 07, 2020 at 03:30:49PM +0200, Christian Brauner wrote:
Hm, maybe change that description to sm like:
[...]
Cool, yeah. Thanks! I've tweaked it a little more
quoted
+ /* 24 is original sizeof(struct seccomp_notif_addfd) */
+ if (size < 24 || size >= PAGE_SIZE)
+ return -EINVAL;
Hm, so maybe add the following:
#define SECCOMP_NOTIFY_ADDFD_VER0 24
#define SECCOMP_NOTIFY_ADDFD_LATEST SECCOMP_NOTIFY_ADDFD_VER0
and then place:
BUILD_BUG_ON(sizeof(struct seccomp_notify_addfd) < SECCOMP_NOTIFY_ADDFD_VER0);
BUILD_BUG_ON(sizeof(struct open_how) != SECCOMP_NOTIFY_ADDFD_LATEST);
Yes, good idea (BTW, did the EA syscall docs land?)
I've made these SECCOMP_NOTIFY_ADDFD_SIZE_* to match your examples below
(i.e. I added "SIZE" to what you suggested above).
somewhere which is what we do for clone3(), openat2() and others to
catch build-time nonsense.
include/uapi/linux/perf_event.h:#define PERF_ATTR_SIZE_VER0 64 /* sizeof first published struct */
include/uapi/linux/sched.h:#define CLONE_ARGS_SIZE_VER0 64 /* sizeof first published struct */
include/uapi/linux/sched/types.h:#define SCHED_ATTR_SIZE_VER0 48 /* sizeof first published struct */
include/linux/fcntl.h:#define OPEN_HOW_SIZE_VER0 24 /* sizeof first published struct */
include/linux/fcntl.h:#define OPEN_HOW_SIZE_LATEST OPEN_HOW_SIZE_VER0
The ..._SIZE_VER0 and ...LATEST stuff doesn't seem useful to export via
UAPI. Above, 2 of the 3 export to uapi. Is there a specific rationale
for which should and which shouldn't?
quoted
+#undef EA_IOCTL
Why is this undefed? :)
It was defined "in" a function, so I like to mimic function visibility.
But you're right; there's no reason to undef it.
--
Kees Cook
From: Sargun Dhillon <redacted>
The current SECCOMP_RET_USER_NOTIF API allows for syscall supervision over
an fd. It is often used in settings where a supervising task emulates
syscalls on behalf of a supervised task in userspace, either to further
restrict the supervisee's syscall abilities or to circumvent kernel
enforced restrictions the supervisor deems safe to lift (e.g. actually
performing a mount(2) for an unprivileged container).
While SECCOMP_RET_USER_NOTIF allows for the interception of any syscall,
only a certain subset of syscalls could be correctly emulated. Over the
last few development cycles, the set of syscalls which can't be emulated
has been reduced due to the addition of pidfd_getfd(2). With this we are
now able to, for example, intercept syscalls that require the supervisor
to operate on file descriptors of the supervisee such as connect(2).
However, syscalls that cause new file descriptors to be installed can not
currently be correctly emulated since there is no way for the supervisor
to inject file descriptors into the supervisee. This patch adds a
new addfd ioctl to remove this restriction by allowing the supervisor to
install file descriptors into the intercepted task. By implementing this
feature via seccomp the supervisor effectively instructs the supervisee
to install a set of file descriptors into its own file descriptor table
during the intercepted syscall. This way it is possible to intercept
syscalls such as open() or accept(), and install (or replace, like
dup2(2)) the supervisor's resulting fd into the supervisee. One
replacement use-case would be to redirect the stdout and stderr of a
supervisee into log file descriptors opened by the supervisor.
The ioctl handling is based on the discussions[1] of how Extensible
Arguments should interact with ioctls. 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.
As a note, the seccomp_notif_addfd structure is laid out based on 8-byte
alignment without requiring packing as there have been packing issues
with uapi highlighted before[2][3]. Although we could overload the
newfd field and use -1 to indicate that it is not to be used, doing
so requires changing the size of the fd field, and introduces struct
packing complexity.
[1]: https://lore.kernel.org/lkml/87o8w9bcaf.fsf@mid.deneb.enyo.de/
[2]: https://lore.kernel.org/lkml/a328b91d-fd8f-4f27-b3c2-91a9c45f18c0@rasmusvillemoes.dk/
[3]: https://lore.kernel.org/lkml/20200612104629.GA15814@ircssh-2.c.rugged-nimbus-611.internal
Cc: Christoph Hellwig <hch@lst.de>
Cc: Christian Brauner <redacted>
Cc: Tycho Andersen <redacted>
Cc: Jann Horn <jannh@google.com>
Cc: Robert Sesek <redacted>
Cc: Chris Palmer <redacted>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-api@vger.kernel.org
Suggested-by: Matt Denton <redacted>
Link: https://lore.kernel.org/r/20200603011044.7972-4-sargun@sargun.me
Signed-off-by: Sargun Dhillon <redacted>
Co-developed-by: Kees Cook <redacted>
Signed-off-by: Kees Cook <redacted>
---
v6.1:
- clarify commit log (christian)
- add ..._SIZE_{VER0,LATEST} and BUILD_BUG_ON()s (christian)
- remove undef (christian)
- fix embedded URL reference numbers
v6: https://lore.kernel.org/lkml/20200707133049.nfxc6vz6vcs26m3b@wittgenstein
---
include/linux/seccomp.h | 4 +
include/uapi/linux/seccomp.h | 22 +++++
kernel/seccomp.c | 173 ++++++++++++++++++++++++++++++++++-
3 files changed, 198 insertions(+), 1 deletion(-)
@@ -87,10 +87,42 @@ struct seccomp_knotif {longval;u32flags;-/* Signals when this has entered SECCOMP_NOTIFY_REPLIED */+/*+*Signalswhenthishaschangedstates,suchasthelistener+*dying,anewseccompaddfdmessage,orchangingtoREPLIED+*/structcompletionready;structlist_headlist;++/* outstanding addfd requests */+structlist_headaddfd;+};++/**+*structseccomp_kaddfd-containerforseccomp_addfdioctlmessages+*+*@file:Areferencetothefiletoinstallintheothertask+*@fd:Thefdnumbertoinstallitat.Ifthefdnumberis-1,itmeansthe+*installingprocessshouldallocatethefdasnormal.+*@flags:Theflagsforthenewfiledescriptor.Atthemoment,onlyO_CLOEXEC+*isallowed.+*@ret:Thereturnvalueoftheinstallingprocess.Itissettothefdnum+*uponsuccess(>=0).+*@completion:Indicatesthattheinstallingprocesshascompletedfd+*installation,orgoneaway(eitherduetosuccessful+*reply,orsignal)+*+*/+structseccomp_kaddfd{+structfile*file;+intfd;+unsignedintflags;++/* To only be set on reply */+intret;+structcompletioncompletion;+structlist_headlist;};/**
@@ -801,6 +844,7 @@ static int seccomp_do_user_notification(int this_syscall,u32flags=0;longret=0;structseccomp_knotifn={};+structseccomp_kaddfd*addfd,*tmp;mutex_lock(&match->notify_lock);err=-ENOSYS;
@@ -813,6 +857,7 @@ static int seccomp_do_user_notification(int this_syscall,n.id=seccomp_next_notify_id(match);init_completion(&n.ready);list_add(&n.list,&match->notif->notifications);+INIT_LIST_HEAD(&n.addfd);up(&match->notif->request);wake_up_poll(&match->wqh,EPOLLIN|EPOLLRDNORM);
@@ -821,14 +866,31 @@ static int seccomp_do_user_notification(int this_syscall,/**Thisiswherewewaitforareplyfromuserspace.*/+wait:err=wait_for_completion_interruptible(&n.ready);mutex_lock(&match->notify_lock);if(err==0){+/* Check if we were woken up by a addfd message */+addfd=list_first_entry_or_null(&n.addfd,+structseccomp_kaddfd,list);+if(addfd&&n.state!=SECCOMP_NOTIFY_REPLIED){+seccomp_handle_addfd(addfd);+mutex_unlock(&match->notify_lock);+gotowait;+}ret=n.val;err=n.error;flags=n.flags;}+/* If there were any pending addfd calls, clear them out */+list_for_each_entry_safe(addfd,tmp,&n.addfd,list){+/* The process went away before we got a chance to handle it */+addfd->ret=-ESRCH;+list_del_init(&addfd->list);+complete(&addfd->completion);+}+/**Notethatit'spossiblethelistenerdiedinbetweenthetimewhen*wewerenotifiedofarespons(orasignal)andwhenwewereableto
@@ -1233,12 +1300,109 @@ static long seccomp_notify_id_valid(struct seccomp_filter *filter,returnret;}+staticlongseccomp_notify_addfd(structseccomp_filter*filter,+structseccomp_notif_addfd__user*uaddfd,+unsignedintsize)+{+structseccomp_notif_addfdaddfd;+structseccomp_knotif*knotif;+structseccomp_kaddfdkaddfd;+intret;++BUILD_BUG_ON(sizeof(structseccomp_notify_addfd)<SECCOMP_NOTIFY_ADDFD_SIZE_VER0);+BUILD_BUG_ON(sizeof(structseccomp_notify_addfd)!=SECCOMP_NOTIFY_ADDFD_SIZE_LATEST);++if(size<SECCOMP_NOTIFY_ADDFD_SIZE_VER0||size>=PAGE_SIZE)+return-EINVAL;++ret=copy_struct_from_user(&addfd,sizeof(addfd),uaddfd,size);+if(ret)+returnret;++if(addfd.newfd_flags&~O_CLOEXEC)+return-EINVAL;++if(addfd.flags&~SECCOMP_ADDFD_FLAG_SETFD)+return-EINVAL;++if(addfd.newfd&&!(addfd.flags&SECCOMP_ADDFD_FLAG_SETFD))+return-EINVAL;++kaddfd.file=fget(addfd.srcfd);+if(!kaddfd.file)+return-EBADF;++kaddfd.flags=addfd.newfd_flags;+kaddfd.fd=(addfd.flags&SECCOMP_ADDFD_FLAG_SETFD)?+addfd.newfd:-1;+init_completion(&kaddfd.completion);++ret=mutex_lock_interruptible(&filter->notify_lock);+if(ret<0)+gotoout;++knotif=find_notification(filter,addfd.id);+if(!knotif){+ret=-ENOENT;+gotoout_unlock;+}++/*+*WedonotwanttoallowforFDinjectiontooccurbeforethe+*notificationhasbeenpickedupbyauserspacehandler,orafter+*thenotificationhasbeenrepliedto.+*/+if(knotif->state!=SECCOMP_NOTIFY_SENT){+ret=-EINPROGRESS;+gotoout_unlock;+}++list_add(&kaddfd.list,&knotif->addfd);+complete(&knotif->ready);+mutex_unlock(&filter->notify_lock);++/* Now we wait for it to be processed or be interrupted */+ret=wait_for_completion_interruptible(&kaddfd.completion);+if(ret==0){+/*+*Wehadasuccessfulcompletion.Theothersidehasalready+*removedusfromtheaddfdqueue,and+*wait_for_completion_interruptiblehasamemorybarrierupon+*successthatletsusreadthisvaluedirectlywithout+*locking.+*/+ret=kaddfd.ret;+gotoout;+}++mutex_lock(&filter->notify_lock);+/*+*Eventhoughwewerewokenupbyasignalandnotasuccessful+*completion,acompletionmayhavehappenedinthemeantime.+*+*Weneedtocheckagainiftheaddfdrequesthasbeenhandled,+*andifnot,wewillremoveitfromthequeue.+*/+if(list_empty(&kaddfd.list))+ret=kaddfd.ret;+else+list_del(&kaddfd.list);++out_unlock:+mutex_unlock(&filter->notify_lock);+out:+fput(kaddfd.file);++returnret;+}+staticlongseccomp_notify_ioctl(structfile*file,unsignedintcmd,unsignedlongarg){structseccomp_filter*filter=file->private_data;void__user*buf=(void__user*)arg;+/* Fixed-size ioctls */switch(cmd){caseSECCOMP_IOCTL_NOTIF_RECV:returnseccomp_notify_recv(filter,buf);
On Tue, Jul 07, 2020 at 02:22:20PM +0200, Christian Brauner wrote:
quoted hunk
So while the patch is correct it leaves 5.6 and 5.7 with a bug in the
pidfd_getfd() implementation and that just doesn't seem right. I'm
wondering whether we should introduce:
void sock_update(struct file *file)
{
struct socket *sock;
int error;
sock = sock_from_file(file, &error);
if (sock) {
sock_update_netprioidx(&sock->sk->sk_cgrp_data);
sock_update_classid(&sock->sk->sk_cgrp_data);
}
}
and switch pidfd_getfd() over to:
@@ -642,10 +642,12 @@ static int pidfd_getfd(struct pid *pid, int fd)}ret=get_unused_fd_flags(O_CLOEXEC);-if(ret<0)+if(ret<0){fput(file);-else+}else{+sock_update(file);fd_install(ret,file);+}returnret;}
first thing in the series and then all of the other patches on top of it
so that we can Cc stable for this and that can get it backported to 5.6,
5.7, and 5.8.
Alternatively, I can make this a separate bugfix patch series which I'll
send upstream soonish. Or we have specific patches just for 5.6, 5.7,
and 5.8. Thoughts?
Okay, I looked at hch's clean-ups again and I'm reminded why they
don't make great -stable material. :) The compat bug (also missing the
sock_update()) needs a similar fix (going back to 3.6...), so, yeah,
for ease of backport, probably an explicit sock_update() implementation
(with compat and native scm using it), and a second patch for pidfd.
Let me see what I looks best...
--
Kees Cook
From: Christian Brauner <hidden> Date: 2020-07-09 12:54:44
On Wed, Jul 08, 2020 at 11:35:39PM -0700, Kees Cook wrote:
On Tue, Jul 07, 2020 at 02:22:20PM +0200, Christian Brauner wrote:
quoted
So while the patch is correct it leaves 5.6 and 5.7 with a bug in the
pidfd_getfd() implementation and that just doesn't seem right. I'm
wondering whether we should introduce:
void sock_update(struct file *file)
{
struct socket *sock;
int error;
sock = sock_from_file(file, &error);
if (sock) {
sock_update_netprioidx(&sock->sk->sk_cgrp_data);
sock_update_classid(&sock->sk->sk_cgrp_data);
}
}
and switch pidfd_getfd() over to:
@@ -642,10 +642,12 @@ static int pidfd_getfd(struct pid *pid, int fd)}ret=get_unused_fd_flags(O_CLOEXEC);-if(ret<0)+if(ret<0){fput(file);-else+}else{+sock_update(file);fd_install(ret,file);+}returnret;}
first thing in the series and then all of the other patches on top of it
so that we can Cc stable for this and that can get it backported to 5.6,
5.7, and 5.8.
Alternatively, I can make this a separate bugfix patch series which I'll
send upstream soonish. Or we have specific patches just for 5.6, 5.7,
and 5.8. Thoughts?
Okay, I looked at hch's clean-ups again and I'm reminded why they
don't make great -stable material. :) The compat bug (also missing the
sock_update()) needs a similar fix (going back to 3.6...), so, yeah,
for ease of backport, probably an explicit sock_update() implementation
(with compat and native scm using it), and a second patch for pidfd.
Let me see what I looks best...
Yeah, it'd be quite some code. I've written some patches for this before
I sent this mail, just so you know. We likely need a 5.6 and 5.7 patch
and a 5.8 patch after Christoph's changes. The 5.8 fixes I'd like to get
in during this merge window. So either I can do this or you can send me
the patches for this?
Christian
From: Christian Brauner <hidden> Date: 2020-07-09 13:08:22
On Wed, Jul 08, 2020 at 11:12:02PM -0700, Kees Cook wrote:
On Tue, Jul 07, 2020 at 03:30:49PM +0200, Christian Brauner wrote:
quoted
Hm, maybe change that description to sm like:
[...]
Cool, yeah. Thanks! I've tweaked it a little more
quoted
quoted
+ /* 24 is original sizeof(struct seccomp_notif_addfd) */
+ if (size < 24 || size >= PAGE_SIZE)
+ return -EINVAL;
Hm, so maybe add the following:
#define SECCOMP_NOTIFY_ADDFD_VER0 24
#define SECCOMP_NOTIFY_ADDFD_LATEST SECCOMP_NOTIFY_ADDFD_VER0
and then place:
BUILD_BUG_ON(sizeof(struct seccomp_notify_addfd) < SECCOMP_NOTIFY_ADDFD_VER0);
BUILD_BUG_ON(sizeof(struct open_how) != SECCOMP_NOTIFY_ADDFD_LATEST);
Yes, good idea (BTW, did the EA syscall docs land?)
I'll be giving a kernel summit talk about extensible syscalls to come to
some agreement on a few things. After this we'll update the doc patch
we have now and merge it. :)
I've made these SECCOMP_NOTIFY_ADDFD_SIZE_* to match your examples below
(i.e. I added "SIZE" to what you suggested above).
Yup, sounds good!
quoted
somewhere which is what we do for clone3(), openat2() and others to
catch build-time nonsense.
include/uapi/linux/perf_event.h:#define PERF_ATTR_SIZE_VER0 64 /* sizeof first published struct */
include/uapi/linux/sched.h:#define CLONE_ARGS_SIZE_VER0 64 /* sizeof first published struct */
include/uapi/linux/sched/types.h:#define SCHED_ATTR_SIZE_VER0 48 /* sizeof first published struct */
include/linux/fcntl.h:#define OPEN_HOW_SIZE_VER0 24 /* sizeof first published struct */
include/linux/fcntl.h:#define OPEN_HOW_SIZE_LATEST OPEN_HOW_SIZE_VER0
The ..._SIZE_VER0 and ...LATEST stuff doesn't seem useful to export via
UAPI. Above, 2 of the 3 export to uapi. Is there a specific rationale
for which should and which shouldn't?
I think openat2() just didn't think it was useful. I find them helpful
because I often update codebase to the newest struct I know about:
struct clone_args {
__aligned_u64 flags;
__aligned_u64 pidfd;
__aligned_u64 child_tid;
__aligned_u64 parent_tid;
__aligned_u64 exit_signal;
__aligned_u64 stack;
__aligned_u64 stack_size;
__aligned_u64 tls;
/* CLONE_ARGS_SIZE_VER0 64 */
__aligned_u64 set_tid;
__aligned_u64 set_tid_size;
/* CLONE_ARGS_SIZE_VER1 80 */
__aligned_u64 cgroup;
/* CLONE_ARGS_SIZE_VER2 88 */
};
But bumping it means I can't use:
clone3(&clone_args, sizeof(clone));
everywhere in the codebase because I'm fscking over everyone on older
kernels now. :)
Soin various parts of the codebase I will just use:
clone3(&clone_args, CLONE_ARGS_SIZE_VER0);
because I don't care about any of the additional features and I don't
need the kernel to copy any of the other stuff. Then in other parts of
the codebase I want to set_tid so I use:
clone3(&clone_args, CLONE_ARGS_SIZE_VER1);
This way I can also set "templates", i.e.
struct clone_args clone_template1 = {
.flags |= CLONE_CLEAR_SIGHAND,
.exit_signal = SIGCHLD,
.set_tid = 1000,
.set_tid_size = 1,
};
and then use the same struct for:
clone3(&clone_template1, CLONE_ARGS_SIZE_VER0);
clone3(&clone_template1, CLONE_ARGS_SIZE_VER1);
Whereas sizeof(clone_template1) would always give me
CLONE_ARGS_SIZE_VER2.
Christian