Re: [PATCH] tmpfs: verify {g,u}id mount options correctly

[Christian Brauner <brauner@kernel.org>]

On Tue, Aug 01, 2023 at 11:47:41AM -0500, Seth Forshee wrote:
> On Tue, Aug 01, 2023 at 06:17:04PM +0200, Christian Brauner wrote:
> > A while ago we received the following report:
> > 
> > "The other outstanding issue I noticed comes from the fact that
> > fsconfig syscalls may occur in a different userns than that which
> > called fsopen. That means that resolving the uid/gid via
> > current_user_ns() can save a kuid that isn't mapped in the associated
> > namespace when the filesystem is finally mounted. This means that it
> > is possible for an unprivileged user to create files owned by any
> > group in a tmpfs mount (since we can set the SUID bit on the tmpfs
> > directory), or a tmpfs that is owned by any user, including the root
> > group/user."
> > 
> > The contract for {g,u}id mount options and {g,u}id values in general set
> > from userspace has always been that they are translated according to the
> > caller's idmapping. In so far, tmpfs has been doing the correct thing.
> > But since tmpfs is mountable in unprivileged contexts it is also
> > necessary to verify that the resulting {k,g}uid is representable in the
> > namespace of the superblock to avoid such bugs as above.
> > 
> > The new mount api's cross-namespace delegation abilities are already
> > widely used. After having talked to a bunch of userspace this is the
> > most faithful solution with minimal regression risks. I know of one
> > users - systemd - that makes use of the new mount api in this way and
> > they don't set unresolable {g,u}ids. So the regression risk is minimal.
> > 
> > Link: https://lore.kernel.org/lkml/CALxfFW4BXhEwxR0Q5LSkg-8Vb4r2MONKCcUCVioehXQKr35eHg@mail.gmail.com
> > Fixes: f32356261d44 ("vfs: Convert ramfs, shmem, tmpfs, devtmpfs, rootfs to use the new mount API")
> > Reported-by: Seth Jenkins <sethjenkins@google.com>
> > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > ---
> > 
> > ---
> >  mm/shmem.c | 28 ++++++++++++++++++++++++----
> >  1 file changed, 24 insertions(+), 4 deletions(-)
> > 
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index 2f2e0e618072..1c0b2dafafe5 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -3636,6 +3636,8 @@ static int shmem_parse_one(struct fs_context *fc, struct fs_parameter *param)
> >  	unsigned long long size;
> >  	char *rest;
> >  	int opt;
> > +	kuid_t kuid;
> > +	kgid_t kgid;
> >  
> >  	opt = fs_parse(fc, shmem_fs_parameters, param, &result);
> >  	if (opt < 0)
> > @@ -3671,14 +3673,32 @@ static int shmem_parse_one(struct fs_context *fc, struct fs_parameter *param)
> >  		ctx->mode = result.uint_32 & 07777;
> >  		break;
> >  	case Opt_uid:
> > -		ctx->uid = make_kuid(current_user_ns(), result.uint_32);
> > -		if (!uid_valid(ctx->uid))
> > +		kuid = make_kuid(current_user_ns(), result.uint_32);
> > +		if (!uid_valid(kuid))
> >  			goto bad_value;
> > +
> > +		/*
> > +		 * The requested uid must be representable in the
> > +		 * filesystem's idmapping.
> > +		 */
> > +		if (!kuid_has_mapping(fc->user_ns, kuid))
> > +			goto bad_value;
> > +
> > +		ctx->uid = kuid;
> 
> This seems like the most sensible way to handle ids in mount options.
> Wouldn't some other filesystems (e.g. fuse) benefit from the same sort
> of handling though? Rather than having filesystems handle these checks
> themselves, what about adding k{uid,gid}_t members to the
> fs_parse_result union with fsparam_is_{uid,gid}() helpers which peform
> these checks?

Yes, I like that proposal. Let's see if that works.