Re: [PATCH v8 3/4] fs: move S_ISGID stripping into the vfs

From: Christian Brauner <brauner@kernel.org>
To: Miklos Szeredi <miklos@szeredi.hu>
Cc: Amir Goldstein <amir73il@gmail.com>,
	Yang Xu <xuyang2018.jy@fujitsu.com>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	ceph-devel <ceph-devel@vger.kernel.org>,
	Al Viro <viro@zeniv.linux.org.uk>,
	Dave Chinner <david@fromorbit.com>,
	"Darrick J. Wong" <djwong@kernel.org>,
	Matthew Wilcox <willy@infradead.org>,
	Jeff Layton <jlayton@kernel.org>
Subject: Re: [PATCH v8 3/4] fs: move S_ISGID stripping into the vfs
Date: Tue, 26 Apr 2022 16:53:49 +0200	[thread overview]
Message-ID: <20220426145349.zxmahoq2app2lhip@wittgenstein> (raw)
In-Reply-To: <CAJfpegu5uJiHgHmLcuSJ6+cQfOPB2aOBovHr4W5j_LU+reJsCw@mail.gmail.com>

On Tue, Apr 26, 2022 at 01:52:11PM +0200, Miklos Szeredi wrote:
> On Tue, 26 Apr 2022 at 13:21, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Tue, Apr 26, 2022 at 1:38 PM Christian Brauner <brauner@kernel.org> wrote:
> 
> > > One thing that I just remembered and which I think I haven't mentioned
> > > so far is that moving S_ISGID stripping from filesystem callpaths into
> > > the vfs callpaths means that we're hoisting this logic out of vfs_*()
> > > helpers implicitly.
> > >
> > > So filesystems that call vfs_*() helpers directly can't rely on S_ISGID
> > > stripping being done in vfs_*() helpers anymore unless they pass the
> > > mode on from a prior run through the vfs.
> > >
> > > This mostly affects overlayfs which calls vfs_*() functions directly. So
> > > a typical overlayfs callstack would be (roughly - I'm omw to lunch):
> > >
> > > sys_mknod()
> > > -> do_mknodat(mode) // calls vfs_prepare_mode()
> > >    -> .mknod = ovl_mknod(mode)
> > >       -> ovl_create(mode)
> > >          -> vfs_mknod(mode)
> > >
> > > I think we are safe as overlayfs passes on the mode on from its own run
> > > through the vfs and then via vfs_*() to the underlying filesystem but it
> > > is worth point that out.
> > >
> > > Ccing Amir just for confirmation.
> >
> > Looks fine to me, but CC Miklos ...
> 
> Looks fine to me as well.  Overlayfs should share the mode (including
> the suid and sgid bits), owner, group and ACL's with the underlying
> filesystem, so clearing sgid based on overlay parent directory should
> result in the same mode as if it was done based on the parent
> directory on the underlying layer.

Ah yes, good point.

> 
> AFAIU this logic is not affected by userns or mnt_userns, but
> Christian would be best to confirm that.

It does depend on it as S_ISGID stripping requires knowledge about
whether the caller has CAP_FSETID and is capable over the parent
directory or if they are in the group the file is owned by.

I think ultimately it might just come down to moving vfs_prepare_mode()
into vfs_*() helpers and not into the do_*at() helpers.

That would be cleaner anyway as right now we have this weird disconnect
between vfs_tmpfile() and vfs_{create,mknod,mkdir}(). IOW, vfs_tmpfile()
doesn't even have an associated do_*() wrapper where we could call
vfs_prepare_mode() from.

So ultimately it might be nicer if we do it in vfs_*() helpers anyway.

The less pretty thing about it will be that the security_path_*() hooks
also want a mode.

Right now these hooks receive the mode as it's passed in from userspace
minus umask but before S_ISGID stripping happens.

Whereas I think they should really see what the filesystem sees and
currently it's a bug that they see something else.

I need to think about this a bit.