From: Sargun Dhillon <sargun@sargun.me> To: Kees Cook <keescook@chromium.org> Cc: Christian Brauner <christian.brauner@ubuntu.com>, containers@lists.linux-foundation.org, Giuseppe Scrivano <gscrivan@redhat.com>, Robert Sesek <rsesek@google.com>, Chris Palmer <palmer@google.com>, Jann Horn <jannh@google.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Daniel Wagner <daniel.wagner@bmw-carit.de>, linux-kernel@vger.kernel.org, Matt Denton <mpdenton@google.com>, John Fastabend <john.r.fastabend@intel.com>, linux-fsdevel@vger.kernel.org, Tejun Heo <tj@kernel.org>, Al Viro <viro@zeniv.linux.org.uk>, cgroups@vger.kernel.org, stable@vger.kernel.org, "David S . Miller" <davem@davemloft.net> Subject: Re: [PATCH v3 1/4] fs, net: Standardize on file_receive helper to move fds across processes Date: Thu, 11 Jun 2020 04:41:02 +0000 [thread overview] Message-ID: <20200611044101.GA5909@ircssh-2.c.rugged-nimbus-611.internal> (raw) In-Reply-To: <202006101953.899EFB53@keescook> On Wed, Jun 10, 2020 at 07:59:55PM -0700, Kees Cook wrote: > > Yeah, that seems reasonable. Here's the diff for that part: > > diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h > index 7b6028b399d8..98bf19b4e086 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 datastructure > * @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..00cbdad6c480 100644 > --- a/kernel/seccomp.c > +++ b/kernel/seccomp.c > @@ -1297,14 +1297,9 @@ static long seccomp_notify_addfd(struct seccomp_filter *filter, > 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; > - > - ret = copy_struct_from_user(&addfd, sizeof(addfd), uaddfd, size); > + ret = copy_from_user(&addfd, uaddfd, sizeof(addfd)); > if (ret) > return ret; > > Looks good to me. If we ever change the size of this struct, we can do the work then to copy_struct_from_user. > > > > ---- > > +#define SECCOMP_IOCTL_NOTIF_ADDFD SECCOMP_IOR(3, \ > > + struct seccomp_notif_addfd) > > > > Lastly, what I believe to be a small mistake, it should be SECCOMP_IOW, based on > > the documentation in ioctl.h -- "_IOW means userland is writing and kernel is > > reading." > > Oooooh. Yeah; good catch. Uhm, that means SECCOMP_IOCTL_NOTIF_ID_VALID > is wrong too, yes? Tycho, Christian, how disruptive would this be to > fix? (Perhaps support both and deprecate the IOR version at some point > in the future?) I think at a minimum we should change the uapi, and accept both (for now). Maybe a pr_warn_once telling people not to use the old one. I can do the patch, if you want. > > Diff for just addfd's change: > > diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h > index 7b6028b399d8..98bf19b4e086 100644 > --- a/include/uapi/linux/seccomp.h > +++ b/include/uapi/linux/seccomp.h > @@ -146,7 +144,7 @@ struct seccomp_notif_addfd { > struct seccomp_notif_resp) > #define SECCOMP_IOCTL_NOTIF_ID_VALID SECCOMP_IOR(2, __u64) > /* On success, the return value is the remote process's added fd number */ > -#define SECCOMP_IOCTL_NOTIF_ADDFD SECCOMP_IOR(3, \ > +#define SECCOMP_IOCTL_NOTIF_ADDFD SECCOMP_IOW(3, \ > struct seccomp_notif_addfd) > > #endif /* _UAPI_LINUX_SECCOMP_H */ > > -- > Kees Cook Looks good. Thank you.
WARNING: multiple messages have this Message-ID (diff)
From: Sargun Dhillon <sargun-GaZTRHToo+CzQB+pC5nmwQ@public.gmane.org> To: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> Cc: Christian Brauner <christian.brauner-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>, containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, Giuseppe Scrivano <gscrivan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, Robert Sesek <rsesek-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>, Chris Palmer <palmer-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>, Jann Horn <jannh-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>, Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>, Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Matt Denton <mpdenton-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>, John Fastabend <john.r.fastabend-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, Al Viro <viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn@public.gmane.org>, cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, "David S . Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> Subject: Re: [PATCH v3 1/4] fs, net: Standardize on file_receive helper to move fds across processes Date: Thu, 11 Jun 2020 04:41:02 +0000 [thread overview] Message-ID: <20200611044101.GA5909@ircssh-2.c.rugged-nimbus-611.internal> (raw) In-Reply-To: <202006101953.899EFB53@keescook> On Wed, Jun 10, 2020 at 07:59:55PM -0700, Kees Cook wrote: > > Yeah, that seems reasonable. Here's the diff for that part: > > diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h > index 7b6028b399d8..98bf19b4e086 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 datastructure > * @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..00cbdad6c480 100644 > --- a/kernel/seccomp.c > +++ b/kernel/seccomp.c > @@ -1297,14 +1297,9 @@ static long seccomp_notify_addfd(struct seccomp_filter *filter, > 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; > - > - ret = copy_struct_from_user(&addfd, sizeof(addfd), uaddfd, size); > + ret = copy_from_user(&addfd, uaddfd, sizeof(addfd)); > if (ret) > return ret; > > Looks good to me. If we ever change the size of this struct, we can do the work then to copy_struct_from_user. > > > > ---- > > +#define SECCOMP_IOCTL_NOTIF_ADDFD SECCOMP_IOR(3, \ > > + struct seccomp_notif_addfd) > > > > Lastly, what I believe to be a small mistake, it should be SECCOMP_IOW, based on > > the documentation in ioctl.h -- "_IOW means userland is writing and kernel is > > reading." > > Oooooh. Yeah; good catch. Uhm, that means SECCOMP_IOCTL_NOTIF_ID_VALID > is wrong too, yes? Tycho, Christian, how disruptive would this be to > fix? (Perhaps support both and deprecate the IOR version at some point > in the future?) I think at a minimum we should change the uapi, and accept both (for now). Maybe a pr_warn_once telling people not to use the old one. I can do the patch, if you want. > > Diff for just addfd's change: > > diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h > index 7b6028b399d8..98bf19b4e086 100644 > --- a/include/uapi/linux/seccomp.h > +++ b/include/uapi/linux/seccomp.h > @@ -146,7 +144,7 @@ struct seccomp_notif_addfd { > struct seccomp_notif_resp) > #define SECCOMP_IOCTL_NOTIF_ID_VALID SECCOMP_IOR(2, __u64) > /* On success, the return value is the remote process's added fd number */ > -#define SECCOMP_IOCTL_NOTIF_ADDFD SECCOMP_IOR(3, \ > +#define SECCOMP_IOCTL_NOTIF_ADDFD SECCOMP_IOW(3, \ > struct seccomp_notif_addfd) > > #endif /* _UAPI_LINUX_SECCOMP_H */ > > -- > Kees Cook Looks good. Thank you.
next prev parent reply other threads:[~2020-06-11 4:41 UTC|newest] Thread overview: 66+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-06-03 1:10 [PATCH v3 0/4] Add seccomp notifier ioctl that enables adding fds Sargun Dhillon 2020-06-03 1:10 ` [PATCH v3 1/4] fs, net: Standardize on file_receive helper to move fds across processes Sargun Dhillon 2020-06-03 1:10 ` Sargun Dhillon 2020-06-04 1:24 ` Christian Brauner 2020-06-04 1:24 ` Christian Brauner 2020-06-04 2:22 ` Kees Cook 2020-06-04 5:20 ` Sargun Dhillon 2020-06-04 12:52 ` Christian Brauner 2020-06-04 13:28 ` David Laight 2020-06-04 13:28 ` David Laight 2020-06-05 7:54 ` Sargun Dhillon 2020-06-09 19:43 ` Kees Cook 2020-06-09 20:03 ` Christian Brauner 2020-06-09 20:03 ` Christian Brauner 2020-06-09 20:55 ` Kees Cook 2020-06-09 21:27 ` Christian Brauner 2020-06-10 5:27 ` Kees Cook 2020-06-10 5:27 ` Kees Cook 2020-06-10 8:12 ` Sargun Dhillon 2020-06-10 8:48 ` David Laight 2020-06-10 8:48 ` David Laight 2020-06-11 3:02 ` Kees Cook 2020-06-11 3:02 ` Kees Cook 2020-06-11 7:51 ` David Laight 2020-06-11 7:51 ` David Laight 2020-06-10 17:10 ` Kees Cook 2020-06-10 17:10 ` Kees Cook 2020-06-11 2:59 ` Kees Cook 2020-06-11 2:59 ` Kees Cook 2020-06-11 4:41 ` Sargun Dhillon [this message] 2020-06-11 4:41 ` Sargun Dhillon 2020-06-11 9:19 ` Christian Brauner 2020-06-11 10:39 ` Sargun Dhillon 2020-06-11 23:23 ` Kees Cook 2020-06-11 10:01 ` Christian Brauner 2020-06-11 10:01 ` Christian Brauner 2020-06-11 11:06 ` Sargun Dhillon 2020-06-11 14:42 ` Christian Brauner 2020-06-11 14:42 ` Christian Brauner 2020-06-11 14:56 ` David Laight 2020-06-11 23:49 ` Kees Cook 2020-06-11 23:49 ` Kees Cook 2020-06-12 6:58 ` Kees Cook 2020-06-12 6:58 ` Kees Cook 2020-06-12 8:36 ` David Laight 2020-06-12 8:36 ` David Laight 2020-06-12 10:46 ` Sargun Dhillon 2020-06-12 10:46 ` Sargun Dhillon 2020-06-12 15:13 ` Kees Cook 2020-06-12 15:13 ` Kees Cook 2020-06-12 15:55 ` David Laight 2020-06-12 18:28 ` Christian Brauner 2020-06-12 18:38 ` Kees Cook 2020-06-12 18:42 ` Christian Brauner 2020-06-15 8:27 ` David Laight 2020-06-10 9:30 ` Christian Brauner 2020-06-10 9:30 ` Christian Brauner 2020-06-04 3:39 ` Sargun Dhillon 2020-06-03 1:10 ` [PATCH v3 2/4] pid: Use file_receive helper to copy FDs Sargun Dhillon 2020-06-03 1:10 ` Sargun Dhillon 2020-06-03 1:10 ` [PATCH v3 3/4] seccomp: Introduce addfd ioctl to seccomp user notifier Sargun Dhillon 2020-06-03 1:10 ` [PATCH v3 4/4] selftests/seccomp: Test SECCOMP_IOCTL_NOTIF_ADDFD Sargun Dhillon 2020-06-03 21:25 ` [PATCH v3 0/4] Add seccomp notifier ioctl that enables adding fds Robert Sesek 2020-06-03 23:42 ` Kees Cook 2020-06-03 23:56 ` Sargun Dhillon 2020-06-04 2:44 ` Kees Cook
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200611044101.GA5909@ircssh-2.c.rugged-nimbus-611.internal \ --to=sargun@sargun.me \ --cc=cgroups@vger.kernel.org \ --cc=christian.brauner@ubuntu.com \ --cc=containers@lists.linux-foundation.org \ --cc=daniel.wagner@bmw-carit.de \ --cc=davem@davemloft.net \ --cc=gregkh@linuxfoundation.org \ --cc=gscrivan@redhat.com \ --cc=jannh@google.com \ --cc=john.r.fastabend@intel.com \ --cc=keescook@chromium.org \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mpdenton@google.com \ --cc=palmer@google.com \ --cc=rsesek@google.com \ --cc=stable@vger.kernel.org \ --cc=tj@kernel.org \ --cc=viro@zeniv.linux.org.uk \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.