| Message ID | de859d0a-feb9-473d-a5e2-c195a3d47abb@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | New uid & gid mount option parsing helpers | expand |
On Thu 27-06-24 19:26:24, Eric Sandeen wrote: > Multiple filesystems take uid and gid as options, and the code to > create the ID from an integer and validate it is standard boilerplate > that can be moved into common helper functions, so do that for > consistency and less cut&paste. > > This also helps avoid the buggy pattern noted by Seth Jenkins at > https://lore.kernel.org/lkml/CALxfFW4BXhEwxR0Q5LSkg-8Vb4r2MONKCcUCVioehXQKr35eHg@mail.gmail.com/ > because uid/gid parsing will fail before any assignment in most > filesystems. > > Signed-off-by: Eric Sandeen <sandeen@sandeen.net> I like the idea since this seems like a nobrainer but is actually surprisingly subtle... > diff --git a/fs/fs_parser.c b/fs/fs_parser.c > index a4d6ca0b8971..24727ec34e5a 100644 > --- a/fs/fs_parser.c > +++ b/fs/fs_parser.c > @@ -308,6 +308,40 @@ int fs_param_is_fd(struct p_log *log, const struct fs_parameter_spec *p, > } > EXPORT_SYMBOL(fs_param_is_fd); > > +int fs_param_is_uid(struct p_log *log, const struct fs_parameter_spec *p, > + struct fs_parameter *param, struct fs_parse_result *result) > +{ > + kuid_t uid; > + > + if (fs_param_is_u32(log, p, param, result) != 0) > + return fs_param_bad_value(log, param); > + > + uid = make_kuid(current_user_ns(), result->uint_32); But here is the problem: Filesystems mountable in user namespaces need to use fc->user_ns for resolving uids / gids (e.g. like fuse_parse_param()). Having helpers that work for some filesystems and are subtly broken for others is worse than no helpers... Or am I missing something? And the problem with fc->user_ns is that currently __fs_parse() does not get fs_context as an argument... So that will need some larger work. Honza
On Fri, Jun 28, 2024 at 11:45:17AM GMT, Jan Kara wrote: > On Thu 27-06-24 19:26:24, Eric Sandeen wrote: > > Multiple filesystems take uid and gid as options, and the code to > > create the ID from an integer and validate it is standard boilerplate > > that can be moved into common helper functions, so do that for > > consistency and less cut&paste. > > > > This also helps avoid the buggy pattern noted by Seth Jenkins at > > https://lore.kernel.org/lkml/CALxfFW4BXhEwxR0Q5LSkg-8Vb4r2MONKCcUCVioehXQKr35eHg@mail.gmail.com/ > > because uid/gid parsing will fail before any assignment in most > > filesystems. > > > > Signed-off-by: Eric Sandeen <sandeen@sandeen.net> > > I like the idea since this seems like a nobrainer but is actually > surprisingly subtle... > > > diff --git a/fs/fs_parser.c b/fs/fs_parser.c > > index a4d6ca0b8971..24727ec34e5a 100644 > > --- a/fs/fs_parser.c > > +++ b/fs/fs_parser.c > > @@ -308,6 +308,40 @@ int fs_param_is_fd(struct p_log *log, const struct fs_parameter_spec *p, > > } > > EXPORT_SYMBOL(fs_param_is_fd); > > > > +int fs_param_is_uid(struct p_log *log, const struct fs_parameter_spec *p, > > + struct fs_parameter *param, struct fs_parse_result *result) > > +{ > > + kuid_t uid; > > + > > + if (fs_param_is_u32(log, p, param, result) != 0) > > + return fs_param_bad_value(log, param); > > + > > + uid = make_kuid(current_user_ns(), result->uint_32); > > But here is the problem: Filesystems mountable in user namespaces need to use > fc->user_ns for resolving uids / gids (e.g. like fuse_parse_param()). > Having helpers that work for some filesystems and are subtly broken for > others is worse than no helpers... Or am I missing something? > > And the problem with fc->user_ns is that currently __fs_parse() does not > get fs_context as an argument... So that will need some larger work. Not really. If someone does an fsopen() in a namespace but the process that actually sets mount options is in another namespace then it's completely intransparent what uid/gid this will resolve to if it's resovled according to fsopen(). It's also a bit strange if someone ends up handing off a tmpfs fscontext that was created in the initial namespace to some random namespace and they now can set uid/gid options that aren't mapped according to their namespace but instead are 1:1 resolved according to the intial namespace. So this would hinder delegation. The expectation is that uid/gid options are resolved in the caller's namespace and that shouldn't be any different for fscontexts for namespace mountable filesystems. The crucial point is to ensure that the resulting kuid/kgid can be resolved in the namespace the filesystem is mounted in at the end. That's what was lacking in e.g., tmpfs in commit 0200679fc795 ("tmpfs: verify {g,u}id mount options correctly") The fuse conversion is the only inconsistency in that regard.
On 6/28/24 4:45 AM, Jan Kara wrote: > On Thu 27-06-24 19:26:24, Eric Sandeen wrote: >> Multiple filesystems take uid and gid as options, and the code to >> create the ID from an integer and validate it is standard boilerplate >> that can be moved into common helper functions, so do that for >> consistency and less cut&paste. >> >> This also helps avoid the buggy pattern noted by Seth Jenkins at >> https://lore.kernel.org/lkml/CALxfFW4BXhEwxR0Q5LSkg-8Vb4r2MONKCcUCVioehXQKr35eHg@mail.gmail.com/ >> because uid/gid parsing will fail before any assignment in most >> filesystems. >> >> Signed-off-by: Eric Sandeen <sandeen@sandeen.net> > > I like the idea since this seems like a nobrainer but is actually > surprisingly subtle... > >> diff --git a/fs/fs_parser.c b/fs/fs_parser.c >> index a4d6ca0b8971..24727ec34e5a 100644 >> --- a/fs/fs_parser.c >> +++ b/fs/fs_parser.c >> @@ -308,6 +308,40 @@ int fs_param_is_fd(struct p_log *log, const struct fs_parameter_spec *p, >> } >> EXPORT_SYMBOL(fs_param_is_fd); >> >> +int fs_param_is_uid(struct p_log *log, const struct fs_parameter_spec *p, >> + struct fs_parameter *param, struct fs_parse_result *result) >> +{ >> + kuid_t uid; >> + >> + if (fs_param_is_u32(log, p, param, result) != 0) >> + return fs_param_bad_value(log, param); >> + >> + uid = make_kuid(current_user_ns(), result->uint_32); > > But here is the problem: Filesystems mountable in user namespaces need to use > fc->user_ns for resolving uids / gids (e.g. like fuse_parse_param()). > Having helpers that work for some filesystems and are subtly broken for > others is worse than no helpers... Or am I missing something? Yeah, I should have pointed that out. tmpfs still does that check after the initial trivial parsing after this change to use the basic helper: case Opt_uid: kuid = result.uid; /* * 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; I can see your point about risks of a helper that doesn't cover all cases though. > And the problem with fc->user_ns is that currently __fs_parse() does not > get fs_context as an argument... So that will need some larger work. Yup, this was discussed a little when I sent this idea as an RFC, and the (brief/small) consensus was that it was worth going this far for now. Getting fc back into __fs_parse looks rather tricky and Al was not keen on the idea, for some reason. Thanks, -Eric > Honza
On Fri 28-06-24 14:23:35, Christian Brauner wrote: > On Fri, Jun 28, 2024 at 11:45:17AM GMT, Jan Kara wrote: > > On Thu 27-06-24 19:26:24, Eric Sandeen wrote: > > > Multiple filesystems take uid and gid as options, and the code to > > > create the ID from an integer and validate it is standard boilerplate > > > that can be moved into common helper functions, so do that for > > > consistency and less cut&paste. > > > > > > This also helps avoid the buggy pattern noted by Seth Jenkins at > > > https://lore.kernel.org/lkml/CALxfFW4BXhEwxR0Q5LSkg-8Vb4r2MONKCcUCVioehXQKr35eHg@mail.gmail.com/ > > > because uid/gid parsing will fail before any assignment in most > > > filesystems. > > > > > > Signed-off-by: Eric Sandeen <sandeen@sandeen.net> > > > > I like the idea since this seems like a nobrainer but is actually > > surprisingly subtle... > > > > > diff --git a/fs/fs_parser.c b/fs/fs_parser.c > > > index a4d6ca0b8971..24727ec34e5a 100644 > > > --- a/fs/fs_parser.c > > > +++ b/fs/fs_parser.c > > > @@ -308,6 +308,40 @@ int fs_param_is_fd(struct p_log *log, const struct fs_parameter_spec *p, > > > } > > > EXPORT_SYMBOL(fs_param_is_fd); > > > > > > +int fs_param_is_uid(struct p_log *log, const struct fs_parameter_spec *p, > > > + struct fs_parameter *param, struct fs_parse_result *result) > > > +{ > > > + kuid_t uid; > > > + > > > + if (fs_param_is_u32(log, p, param, result) != 0) > > > + return fs_param_bad_value(log, param); > > > + > > > + uid = make_kuid(current_user_ns(), result->uint_32); > > > > But here is the problem: Filesystems mountable in user namespaces need to use > > fc->user_ns for resolving uids / gids (e.g. like fuse_parse_param()). > > Having helpers that work for some filesystems and are subtly broken for > > others is worse than no helpers... Or am I missing something? > > > > And the problem with fc->user_ns is that currently __fs_parse() does not > > get fs_context as an argument... So that will need some larger work. > > Not really. If someone does an fsopen() in a namespace but the process > that actually sets mount options is in another namespace then it's > completely intransparent what uid/gid this will resolve to if it's > resovled according to fsopen(). > > It's also a bit strange if someone ends up handing off a tmpfs fscontext > that was created in the initial namespace to some random namespace and > they now can set uid/gid options that aren't mapped according to their > namespace but instead are 1:1 resolved according to the intial > namespace. So this would hinder delegation. > > The expectation is that uid/gid options are resolved in the caller's > namespace and that shouldn't be any different for fscontexts for > namespace mountable filesystems. The crucial point is to ensure that the > resulting kuid/kgid can be resolved in the namespace the filesystem is > mounted in at the end. That's what was lacking in e.g., tmpfs in commit > 0200679fc795 ("tmpfs: verify {g,u}id mount options correctly") > > The fuse conversion is the only inconsistency in that regard. OK, thanks for explanation! Honza
diff --git a/Documentation/filesystems/mount_api.rst b/Documentation/filesystems/mount_api.rst index 9aaf6ef75eb5..317934c9e8fc 100644 --- a/Documentation/filesystems/mount_api.rst +++ b/Documentation/filesystems/mount_api.rst @@ -645,6 +645,8 @@ The members are as follows: fs_param_is_blockdev Blockdev path * Needs lookup fs_param_is_path Path * Needs lookup fs_param_is_fd File descriptor result->int_32 + fs_param_is_uid User ID (u32) result->uid + fs_param_is_gid Group ID (u32) result->gid ======================= ======================= ===================== Note that if the value is of fs_param_is_bool type, fs_parse() will try @@ -678,6 +680,8 @@ The members are as follows: fsparam_bdev() fs_param_is_blockdev fsparam_path() fs_param_is_path fsparam_fd() fs_param_is_fd + fsparam_uid() fs_param_is_uid + fsparam_gid() fs_param_is_gid ======================= =============================================== all of which take two arguments, name string and option number - for @@ -784,8 +788,9 @@ process the parameters it is given. option number (which it returns). If successful, and if the parameter type indicates the result is a - boolean, integer or enum type, the value is converted by this function and - the result stored in result->{boolean,int_32,uint_32,uint_64}. + boolean, integer, enum, uid, or gid type, the value is converted by this + function and the result stored in + result->{boolean,int_32,uint_32,uint_64,uid,gid}. If a match isn't initially made, the key is prefixed with "no" and no value is present then an attempt will be made to look up the key with the diff --git a/fs/fs_parser.c b/fs/fs_parser.c index a4d6ca0b8971..24727ec34e5a 100644 --- a/fs/fs_parser.c +++ b/fs/fs_parser.c @@ -308,6 +308,40 @@ int fs_param_is_fd(struct p_log *log, const struct fs_parameter_spec *p, } EXPORT_SYMBOL(fs_param_is_fd); +int fs_param_is_uid(struct p_log *log, const struct fs_parameter_spec *p, + struct fs_parameter *param, struct fs_parse_result *result) +{ + kuid_t uid; + + if (fs_param_is_u32(log, p, param, result) != 0) + return fs_param_bad_value(log, param); + + uid = make_kuid(current_user_ns(), result->uint_32); + if (!uid_valid(uid)) + return inval_plog(log, "Invalid uid '%s'", param->string); + + result->uid = uid; + return 0; +} +EXPORT_SYMBOL(fs_param_is_uid); + +int fs_param_is_gid(struct p_log *log, const struct fs_parameter_spec *p, + struct fs_parameter *param, struct fs_parse_result *result) +{ + kgid_t gid; + + if (fs_param_is_u32(log, p, param, result) != 0) + return fs_param_bad_value(log, param); + + gid = make_kgid(current_user_ns(), result->uint_32); + if (!gid_valid(gid)) + return inval_plog(log, "Invalid gid '%s'", param->string); + + result->gid = gid; + return 0; +} +EXPORT_SYMBOL(fs_param_is_gid); + int fs_param_is_blockdev(struct p_log *log, const struct fs_parameter_spec *p, struct fs_parameter *param, struct fs_parse_result *result) { diff --git a/include/linux/fs_parser.h b/include/linux/fs_parser.h index d3350979115f..6cf713a7e6c6 100644 --- a/include/linux/fs_parser.h +++ b/include/linux/fs_parser.h @@ -28,7 +28,7 @@ typedef int fs_param_type(struct p_log *, */ fs_param_type fs_param_is_bool, fs_param_is_u32, fs_param_is_s32, fs_param_is_u64, fs_param_is_enum, fs_param_is_string, fs_param_is_blob, fs_param_is_blockdev, - fs_param_is_path, fs_param_is_fd; + fs_param_is_path, fs_param_is_fd, fs_param_is_uid, fs_param_is_gid; /* * Specification of the type of value a parameter wants. @@ -57,6 +57,8 @@ struct fs_parse_result { int int_32; /* For spec_s32/spec_enum */ unsigned int uint_32; /* For spec_u32{,_octal,_hex}/spec_enum */ u64 uint_64; /* For spec_u64 */ + kuid_t uid; + kgid_t gid; }; }; @@ -131,6 +133,8 @@ static inline bool fs_validate_description(const char *name, #define fsparam_bdev(NAME, OPT) __fsparam(fs_param_is_blockdev, NAME, OPT, 0, NULL) #define fsparam_path(NAME, OPT) __fsparam(fs_param_is_path, NAME, OPT, 0, NULL) #define fsparam_fd(NAME, OPT) __fsparam(fs_param_is_fd, NAME, OPT, 0, NULL) +#define fsparam_uid(NAME, OPT) __fsparam(fs_param_is_uid, NAME, OPT, 0, NULL) +#define fsparam_gid(NAME, OPT) __fsparam(fs_param_is_gid, NAME, OPT, 0, NULL) /* String parameter that allows empty argument */ #define fsparam_string_empty(NAME, OPT) \
Multiple filesystems take uid and gid as options, and the code to create the ID from an integer and validate it is standard boilerplate that can be moved into common helper functions, so do that for consistency and less cut&paste. This also helps avoid the buggy pattern noted by Seth Jenkins at https://lore.kernel.org/lkml/CALxfFW4BXhEwxR0Q5LSkg-8Vb4r2MONKCcUCVioehXQKr35eHg@mail.gmail.com/ because uid/gid parsing will fail before any assignment in most filesystems. Signed-off-by: Eric Sandeen <sandeen@sandeen.net> --- Documentation/filesystems/mount_api.rst | 9 +++++-- fs/fs_parser.c | 34 +++++++++++++++++++++++++ include/linux/fs_parser.h | 6 ++++- 3 files changed, 46 insertions(+), 3 deletions(-)