Re: [PATCH v6 02/11] fs: add O_ALLOW_ENCODED open flag

From: Amir Goldstein <amir73il@gmail.com>
To: Omar Sandoval <osandov@osandov.com>
Cc: Jann Horn <jannh@google.com>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	Linux Btrfs <linux-btrfs@vger.kernel.org>,
	Al Viro <viro@zeniv.linux.org.uk>,
	Christoph Hellwig <hch@infradead.org>,
	Dave Chinner <david@fromorbit.com>,
	Aleksa Sarai <cyphar@cyphar.com>,
	Linux API <linux-api@vger.kernel.org>,
	Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH v6 02/11] fs: add O_ALLOW_ENCODED open flag
Date: Tue, 1 Dec 2020 10:15:58 +0200	[thread overview]
Message-ID: <CAOQ4uxhLG1b03nYEgefcAybvMem26mjG=6dcrD5djjYFSa-q1g@mail.gmail.com> (raw)
In-Reply-To: <X8VHcZs6paUvQGkk@relinquished.localdomain>

On Mon, Nov 30, 2020 at 9:26 PM Omar Sandoval <osandov@osandov.com> wrote:
>
> On Sat, Nov 21, 2020 at 12:41:23AM +0100, Jann Horn wrote:
> > On Thu, Nov 19, 2020 at 8:03 AM Amir Goldstein <amir73il@gmail.com> wrote:
> > > On Wed, Nov 18, 2020 at 9:18 PM Omar Sandoval <osandov@osandov.com> wrote:
> > > > The upcoming RWF_ENCODED operation introduces some security concerns:
> > > >
> > > > 1. Compressed writes will pass arbitrary data to decompression
> > > >    algorithms in the kernel.
> > > > 2. Compressed reads can leak truncated/hole punched data.
> > > >
> > > > Therefore, we need to require privilege for RWF_ENCODED. It's not
> > > > possible to do the permissions checks at the time of the read or write
> > > > because, e.g., io_uring submits IO from a worker thread. So, add an open
> > > > flag which requires CAP_SYS_ADMIN. It can also be set and cleared with
> > > > fcntl(). The flag is not cleared in any way on fork or exec. It must be
> > > > combined with O_CLOEXEC when opening to avoid accidental leaks (if
> > > > needed, it may be set without O_CLOEXEC by using fnctl()).
> > > >
> > > > Note that the usual issue that unknown open flags are ignored doesn't
> > > > really matter for O_ALLOW_ENCODED; if the kernel doesn't support
> > > > O_ALLOW_ENCODED, then it doesn't support RWF_ENCODED, either.
> > [...]
> > > > diff --git a/fs/open.c b/fs/open.c
> > > > index 9af548fb841b..f2863aaf78e7 100644
> > > > --- a/fs/open.c
> > > > +++ b/fs/open.c
> > > > @@ -1040,6 +1040,13 @@ inline int build_open_flags(const struct open_how *how, struct open_flags *op)
> > > >                 acc_mode = 0;
> > > >         }
> > > >
> > > > +       /*
> > > > +        * O_ALLOW_ENCODED must be combined with O_CLOEXEC to avoid accidentally
> > > > +        * leaking encoded I/O privileges.
> > > > +        */
> > > > +       if ((how->flags & (O_ALLOW_ENCODED | O_CLOEXEC)) == O_ALLOW_ENCODED)
> > > > +               return -EINVAL;
> > > > +
> > >
> > >
> > > dup() can also result in accidental leak.
> > > We could fail dup() of fd without O_CLOEXEC. Should we?
> > >
> > > If we should than what error code should it be? We could return EPERM,
> > > but since we do allow to clear O_CLOEXEC or set O_ALLOW_ENCODED
> > > after open, EPERM seems a tad harsh.
> > > EINVAL seems inappropriate because the error has nothing to do with
> > > input args of dup() and EBADF would also be confusing.
> >
> > This seems very arbitrary to me. Sure, leaking these file descriptors
> > wouldn't be great, but there are plenty of other types of file
> > descriptors that are probably more sensitive. (Writable file
> > descriptors to databases, to important configuration files, to
> > io_uring instances, and so on.) So I don't see why this specific
> > feature should impose such special rules on it.
>
> I agree with Jann. I'm okay with the O_CLOEXEC-on-open requirement if it
> makes people more comfortable, but I don't think we should be bending
> over backwards to block it anywhere else.

I'm fine with or without the O_CLOEXEC-on-open requirement.
Just pointing out the weirdness.

Thanks,
Amir.