On Wed, Aug 02, 2023 at 02:06:26PM +0200, Christian Brauner wrote: > 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@xxxxxxxxxxxxxx > > > Fixes: f32356261d44 ("vfs: Convert ramfs, shmem, tmpfs, devtmpfs, rootfs to use the new mount API") > > > Reported-by: Seth Jenkins <sethjenkins@xxxxxxxxxx> > > > Signed-off-by: Christian Brauner <brauner@xxxxxxxxxx> > > > --- > > > > > > --- > > > 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. After a little poking around, this is more complicated than I'd initially thought. The parameter helpers don't currently get passed an fs_context, and ceph/rbd seem to be using the parameter parsing like a library when there legitimately is not an fs_context to be passed. So it makes sense to take this patch as an immediate fix, and we can take a look at trying to make it more generic later. Reviewed-by: Seth Forshee (DigitalOcean) <sforshee@xxxxxxxxxx>