by David Laight

[permalink] [raw]

Subject: RE: [PATCH v4 03/11] fs: Add fd_install_received() wrapper for __fd_install_received()

From: Kees Cook
> Sent: 17 June 2020 20:58
> On Wed, Jun 17, 2020 at 03:35:20PM +0000, David Laight wrote:
> > From: Kees Cook
> > > Sent: 16 June 2020 04:25
> > >
> > > For both pidfd and seccomp, the __user pointer is not used. Update
> > > __fd_install_received() to make writing to ufd optional. (ufd
> > > itself cannot checked for NULL because this changes the SCM_RIGHTS
> > > interface behavior.) In these cases, the new fd needs to be returned
> > > on success. Update the existing callers to handle it. Add new wrapper
> > > fd_install_received() for pidfd and seccomp that does not use the ufd
> > > argument.
> > ...>
> > > static inline int fd_install_received_user(struct file *file, int __user *ufd,
> > > unsigned int o_flags)
> > > {
> > > - return __fd_install_received(file, ufd, o_flags);
> > > + return __fd_install_received(file, true, ufd, o_flags);
> > > +}
> >
> > Can you get rid of the 'return user' parameter by adding
> > if (!ufd) return -EFAULT;
> > to the above wrapper, then checking for NULL in the function?
> >
> > Or does that do the wrong horrid things in the fail path?
>
> Oh, hm. No, that shouldn't break the failure path, since everything gets
> unwound in __fd_install_received if the ufd write fails.
>
> Effectively this (I'll chop it up into the correct patches):

Yep, that's what i was thinking...

Personally I'm not sure that it matters whether the file is left
attached to a process fd when the copy_to_user() fails.
The kernel data structures are consistent either way.
So sane code relies on catching SIGSEGV, fixing thigs up,
and carrying on.
(IIRC the original /bin/sh code called sbrk() in its SIGSEGV
handler instead of doing the limit check in malloc()!)

The important error path is 'failing to get an fd number'.
In that case the caller needs to keep the 'file *' or close it.

I've not looked at the code, but I wonder if you need to pass
the 'file *' by reference so that you can consume it (write NULL)
and return an error.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

2020-06-18 22:58:40

by Sargun Dhillon

[permalink] [raw]

Subject: Re: [PATCH v4 00/11] Add seccomp notifier ioctl that enables adding fds

On Mon, Jun 15, 2020 at 08:25:13PM -0700, Kees Cook wrote:
> Hello!
>
> This is a bit of 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 seccomp tree, unless someone has objections.
> :) Please review and test!
>
> -Kees
>
> [1] https://lore.kernel.org/lkml/[email protected]/
> [2] https://lore.kernel.org/lkml/[email protected]/
>
> Kees Cook (9):
> net/scm: Regularize compat handling of scm_detach_fds()
> fs: Move __scm_install_fd() to __fd_install_received()
> fs: Add fd_install_received() wrapper for __fd_install_received()
> pidfd: Replace open-coded partial fd_install_received()
> fs: Expand __fd_install_received() to accept fd
> selftests/seccomp: Make kcmp() less required
> selftests/seccomp: Rename user_trap_syscall() to user_notif_syscall()
> seccomp: Switch addfd to Extensible Argument ioctl
> seccomp: Fix ioctl number for SECCOMP_IOCTL_NOTIF_ID_VALID
>
This looks much cleaner than the original patchset. Thanks.

Reviewed-by: Sargun Dhillon <[email protected]>

on the pidfd, change fs* changes.

> Sargun Dhillon (2):
> seccomp: Introduce addfd ioctl to seccomp user notifier
> selftests/seccomp: Test SECCOMP_IOCTL_NOTIF_ADDFD
>
> fs/file.c | 65 ++++
> include/linux/file.h | 16 +
> include/uapi/linux/seccomp.h | 25 +-
> kernel/pid.c | 11 +-
> kernel/seccomp.c | 181 ++++++++-
> net/compat.c | 55 ++-
> net/core/scm.c | 50 +--
> tools/testing/selftests/seccomp/seccomp_bpf.c | 350 +++++++++++++++---
> 8 files changed, 618 insertions(+), 135 deletions(-)
>
> --
> 2.25.1
>