| Message ID | 20200706201720.3482959-5-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add seccomp notifier ioctl that enables adding fds | expand |
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 <vamshi.k.sthambamkadi@gmail.com> for > catching a missed fput() in an earlier version of this patch. > > Fixes: 8649c322f75c ("pid: Implement pidfd_getfd syscall") > Reviewed-by: Sargun Dhillon <sargun@sargun.me> > Signed-off-by: Kees Cook <keescook@chromium.org> > --- Thanks! Acked-by: Christian Brauner <christian.brauner@ubuntu.com> 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: diff --git a/kernel/pid.c b/kernel/pid.c index f1496b757162..c26bba822be3 100644 --- a/kernel/pid.c +++ b/kernel/pid.c @@ -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); + } return ret; } 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
On Tue, Jul 07, 2020 at 02:22:20PM +0200, Christian Brauner wrote: > 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 <vamshi.k.sthambamkadi@gmail.com> for > > catching a missed fput() in an earlier version of this patch. > > > > Fixes: 8649c322f75c ("pid: Implement pidfd_getfd syscall") > > Reviewed-by: Sargun Dhillon <sargun@sargun.me> > > Signed-off-by: Kees Cook <keescook@chromium.org> > > --- > > Thanks! > Acked-by: Christian Brauner <christian.brauner@ubuntu.com> > > 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: > > diff --git a/kernel/pid.c b/kernel/pid.c > index f1496b757162..c26bba822be3 100644 > --- a/kernel/pid.c > +++ b/kernel/pid.c > @@ -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); > + } > > return ret; > } > > 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... > > Thanks! > Christian
On Tue, Jul 07, 2020 at 02:22:20PM +0200, Christian Brauner wrote: > 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: > > diff --git a/kernel/pid.c b/kernel/pid.c > index f1496b757162..c26bba822be3 100644 > --- a/kernel/pid.c > +++ b/kernel/pid.c > @@ -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); > + } > > return ret; > } > > 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...
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: > > 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: > > > > diff --git a/kernel/pid.c b/kernel/pid.c > > index f1496b757162..c26bba822be3 100644 > > --- a/kernel/pid.c > > +++ b/kernel/pid.c > > @@ -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); > > + } > > > > return ret; > > } > > > > 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
diff --git a/kernel/pid.c b/kernel/pid.c index f1496b757162..a31c102f4c87 100644 --- a/kernel/pid.c +++ b/kernel/pid.c @@ -635,17 +635,8 @@ static int pidfd_getfd(struct pid *pid, int fd) if (IS_ERR(file)) return PTR_ERR(file); - ret = security_file_receive(file); - if (ret) { - fput(file); - return ret; - } - - ret = get_unused_fd_flags(O_CLOEXEC); - if (ret < 0) - fput(file); - else - fd_install(ret, file); + ret = receive_fd(file, O_CLOEXEC); + fput(file); return ret; }
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 <vamshi.k.sthambamkadi@gmail.com> for catching a missed fput() in an earlier version of this patch. Fixes: 8649c322f75c ("pid: Implement pidfd_getfd syscall") Reviewed-by: Sargun Dhillon <sargun@sargun.me> Signed-off-by: Kees Cook <keescook@chromium.org> --- kernel/pid.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)