On 3/6/24 4:50 AM, Christian Brauner wrote:
> Fwiw, Opt_{g,u}d I would like to see that either moved completely to the
> VFS or we need to provide standardized helpers.
>
> The issue is that for a userns mountable filesytems the validation done
> here isn't enough and that's easy to miss (Obviously, debugfs isn't
> relevant as it's not userns mountable but still.). For example, for
> in tmpfs I recently fixed a bug where validation was wrong:
>
> case Opt_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;
> break;
>
> The crucial step where the {g,u}id must also be representable in the
> superblock's namespace not just in the caller's was missing. So really
> we should have a generic helper that we can reycle for all Opt_{g,u}id
> mount options or move that Opt_{g,u}id to the VFS itself. There was some
> nastiness involved in this when I last looked at this though. And all
> that invalfc() reporting should then also be identical across
> filesystems.
>
> So that's a ToDo for the future.
Just FWIW, I started looking at this, and making i.e. an fsparam_uid() and
moving all the checking into the parser seemed like a nice idea, and it
removed a lot of boilerplate from several filesystems. Option parsing was
then looking like simply:
fsparam_uid ("uid", Opt_uid),
...
case Opt_uid:
opts->uid = result.uid;
break;
which is pretty nice, I think.
But unfortunately after 7f5d38141e30 ("new primitive: __fs_parse()"), parsers
no longer get an *fc, which means we can't get to fc->user_ns while parsing,
which I guess we need for proper validation.
It seems that 7f5d38141e30 was done for the benefit of rbd (?) - I'll have
to look more closely at that and see if we can resurrect access to fc, unless
there are other ways around this.
-Eric
