| Message ID | 20210907073428.GD18254@kili (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ksmbd: remove unnecessary conditions | expand |
On (21/09/07 10:34), Dan Carpenter wrote: > > id = le32_to_cpu(psid->sub_auth[psid->num_subauth - 1]); > - if (id >= 0) { > - /* > - * Translate raw sid into kuid in the server's user > - * namespace. > - */ > - uid = make_kuid(&init_user_ns, id); > - > - /* If this is an idmapped mount, apply the idmapping. */ > - uid = kuid_from_mnt(user_ns, uid); > - if (uid_valid(uid)) { > - fattr->cf_uid = uid; > - rc = 0; > - } > + /* > + * Translate raw sid into kuid in the server's user > + * namespace. > + */ > + uid = make_kuid(&init_user_ns, id); Can make_kuid() return INVALID_UID? IOW, uid_valid(uid) here as well? > + > + /* If this is an idmapped mount, apply the idmapping. */ > + uid = kuid_from_mnt(user_ns, uid); > + if (uid_valid(uid)) { > + fattr->cf_uid = uid; > + rc = 0; > } [..] > + /* > + * Translate raw sid into kgid in the server's user > + * namespace. > + */ > + gid = make_kgid(&init_user_ns, id); Ditto. > + /* If this is an idmapped mount, apply the idmapping. */ > + gid = kgid_from_mnt(user_ns, gid); > + if (gid_valid(gid)) { > + fattr->cf_gid = gid; > + rc = 0;
On Tue, Sep 07, 2021 at 05:06:04PM +0900, Sergey Senozhatsky wrote: > On (21/09/07 10:34), Dan Carpenter wrote: > > > > id = le32_to_cpu(psid->sub_auth[psid->num_subauth - 1]); > > - if (id >= 0) { > > - /* > > - * Translate raw sid into kuid in the server's user > > - * namespace. > > - */ > > - uid = make_kuid(&init_user_ns, id); > > - > > - /* If this is an idmapped mount, apply the idmapping. */ > > - uid = kuid_from_mnt(user_ns, uid); > > - if (uid_valid(uid)) { > > - fattr->cf_uid = uid; > > - rc = 0; > > - } > > + /* > > + * Translate raw sid into kuid in the server's user > > + * namespace. > > + */ > > + uid = make_kuid(&init_user_ns, id); > > Can make_kuid() return INVALID_UID? IOW, uid_valid(uid) here as well? No need to check twice. We're going to check at the end. > > > + > > + /* If this is an idmapped mount, apply the idmapping. */ > > + uid = kuid_from_mnt(user_ns, uid); > > + if (uid_valid(uid)) { ^^^^^^^^^^^^^^ The check here is sufficient. regards, dan carpenter
On (21/09/07 11:54), Dan Carpenter wrote: > On Tue, Sep 07, 2021 at 05:06:04PM +0900, Sergey Senozhatsky wrote: > > On (21/09/07 10:34), Dan Carpenter wrote: > > > > > > id = le32_to_cpu(psid->sub_auth[psid->num_subauth - 1]); > > > - if (id >= 0) { > > > - /* > > > - * Translate raw sid into kuid in the server's user > > > - * namespace. > > > - */ > > > - uid = make_kuid(&init_user_ns, id); > > > - > > > - /* If this is an idmapped mount, apply the idmapping. */ > > > - uid = kuid_from_mnt(user_ns, uid); > > > - if (uid_valid(uid)) { > > > - fattr->cf_uid = uid; > > > - rc = 0; > > > - } > > > + /* > > > + * Translate raw sid into kuid in the server's user > > > + * namespace. > > > + */ > > > + uid = make_kuid(&init_user_ns, id); > > > > Can make_kuid() return INVALID_UID? IOW, uid_valid(uid) here as well? > > No need to check twice. We're going to check at the end. > > > > > > + > > > + /* If this is an idmapped mount, apply the idmapping. */ > > > + uid = kuid_from_mnt(user_ns, uid); > > > + if (uid_valid(uid)) { > ^^^^^^^^^^^^^^ > The check here is sufficient. My point was more that a potentially invalid UID is passed to kuid_from_mnt() and kgid_from_mnt(). I don't see map_id_up(), for example, checking that passed UID is valid. So decided to double check.
On Tue, Sep 07, 2021 at 06:04:03PM +0900, Sergey Senozhatsky wrote: > On (21/09/07 11:54), Dan Carpenter wrote: > > On Tue, Sep 07, 2021 at 05:06:04PM +0900, Sergey Senozhatsky wrote: > > > On (21/09/07 10:34), Dan Carpenter wrote: > > > > > > > > id = le32_to_cpu(psid->sub_auth[psid->num_subauth - 1]); > > > > - if (id >= 0) { > > > > - /* > > > > - * Translate raw sid into kuid in the server's user > > > > - * namespace. > > > > - */ > > > > - uid = make_kuid(&init_user_ns, id); > > > > - > > > > - /* If this is an idmapped mount, apply the idmapping. */ > > > > - uid = kuid_from_mnt(user_ns, uid); > > > > - if (uid_valid(uid)) { > > > > - fattr->cf_uid = uid; > > > > - rc = 0; > > > > - } > > > > + /* > > > > + * Translate raw sid into kuid in the server's user > > > > + * namespace. > > > > + */ > > > > + uid = make_kuid(&init_user_ns, id); > > > > > > Can make_kuid() return INVALID_UID? IOW, uid_valid(uid) here as well? > > > > No need to check twice. We're going to check at the end. > > > > > > > > > + > > > > + /* If this is an idmapped mount, apply the idmapping. */ > > > > + uid = kuid_from_mnt(user_ns, uid); > > > > + if (uid_valid(uid)) { > > ^^^^^^^^^^^^^^ > > The check here is sufficient. > > My point was more that a potentially invalid UID is passed to kuid_from_mnt() > and kgid_from_mnt(). I don't see map_id_up(), for example, checking that > passed UID is valid. So decided to double check. But you've seen it now, right? The kuid_from_mnt() will return INVALID_UID if you pass it any unknown uid (including INVALID_UID). regards, dan carpenter
On (21/09/07 12:14), Dan Carpenter wrote: [..] > > > > > > > > Can make_kuid() return INVALID_UID? IOW, uid_valid(uid) here as well? > > > > > > No need to check twice. We're going to check at the end. > > > > > > > > > > > > + > > > > > + /* If this is an idmapped mount, apply the idmapping. */ > > > > > + uid = kuid_from_mnt(user_ns, uid); > > > > > + if (uid_valid(uid)) { > > > ^^^^^^^^^^^^^^ > > > The check here is sufficient. > > > > My point was more that a potentially invalid UID is passed to kuid_from_mnt() > > and kgid_from_mnt(). I don't see map_id_up(), for example, checking that > > passed UID is valid. So decided to double check. > > But you've seen it now, right? A linear search in array of 5 elements or a binary search in array of 340 elements? Yea, I saw it. I'd prefer one extra uid_valid(), if you'd ask me - why call the function if we already know that it'll fail.
On Tue, Sep 07, 2021 at 06:59:38PM +0900, Sergey Senozhatsky wrote: > On (21/09/07 12:14), Dan Carpenter wrote: > [..] > > > > > > > > > > Can make_kuid() return INVALID_UID? IOW, uid_valid(uid) here as well? > > > > > > > > No need to check twice. We're going to check at the end. > > > > > > > > > > > > > > > + > > > > > > + /* If this is an idmapped mount, apply the idmapping. */ > > > > > > + uid = kuid_from_mnt(user_ns, uid); > > > > > > + if (uid_valid(uid)) { > > > > ^^^^^^^^^^^^^^ > > > > The check here is sufficient. > > > > > > My point was more that a potentially invalid UID is passed to kuid_from_mnt() > > > and kgid_from_mnt(). I don't see map_id_up(), for example, checking that > > > passed UID is valid. So decided to double check. > > > > But you've seen it now, right? > > A linear search in array of 5 elements or a binary search in array of 340 > elements? Yea, I saw it. I'd prefer one extra uid_valid(), if you'd ask > me - why call the function if we already know that it'll fail. It's a failure path. Hopefully people will only give us valid data. We would normally only optimize the failure path if we thought that it could be used as a DoS vector. regards, dan carpenter
On (21/09/07 13:17), Dan Carpenter wrote: > > > > > > But you've seen it now, right? > > > > A linear search in array of 5 elements or a binary search in array of 340 > > elements? Yea, I saw it. I'd prefer one extra uid_valid(), if you'd ask > > me - why call the function if we already know that it'll fail. > > It's a failure path. Hopefully people will only give us valid data. > I usually prefer to assume that remote clients are _not exactly_ nice folks. Can this be a DoS vector - probably not. `cmp; je;` is several thousand times cheaper that a bsearch, and this is a remote user request servicing path. So. Just my 5 cents. I'll leave it to you and Namjae to decide.
diff --git a/fs/ksmbd/smbacl.c b/fs/ksmbd/smbacl.c index 16da99a9963c..0a95cdec8c80 100644 --- a/fs/ksmbd/smbacl.c +++ b/fs/ksmbd/smbacl.c @@ -274,38 +274,34 @@ static int sid_to_id(struct user_namespace *user_ns, uid_t id; id = le32_to_cpu(psid->sub_auth[psid->num_subauth - 1]); - if (id >= 0) { - /* - * Translate raw sid into kuid in the server's user - * namespace. - */ - uid = make_kuid(&init_user_ns, id); - - /* If this is an idmapped mount, apply the idmapping. */ - uid = kuid_from_mnt(user_ns, uid); - if (uid_valid(uid)) { - fattr->cf_uid = uid; - rc = 0; - } + /* + * Translate raw sid into kuid in the server's user + * namespace. + */ + uid = make_kuid(&init_user_ns, id); + + /* If this is an idmapped mount, apply the idmapping. */ + uid = kuid_from_mnt(user_ns, uid); + if (uid_valid(uid)) { + fattr->cf_uid = uid; + rc = 0; } } else { kgid_t gid; gid_t id; id = le32_to_cpu(psid->sub_auth[psid->num_subauth - 1]); - if (id >= 0) { - /* - * Translate raw sid into kgid in the server's user - * namespace. - */ - gid = make_kgid(&init_user_ns, id); - - /* If this is an idmapped mount, apply the idmapping. */ - gid = kgid_from_mnt(user_ns, gid); - if (gid_valid(gid)) { - fattr->cf_gid = gid; - rc = 0; - } + /* + * Translate raw sid into kgid in the server's user + * namespace. + */ + gid = make_kgid(&init_user_ns, id); + + /* If this is an idmapped mount, apply the idmapping. */ + gid = kgid_from_mnt(user_ns, gid); + if (gid_valid(gid)) { + fattr->cf_gid = gid; + rc = 0; } }
uid_t are unsigned so they can't be less than zero. Remove the conditions since they are always true. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- fs/ksmbd/smbacl.c | 48 ++++++++++++++++++++++------------------------- 1 file changed, 22 insertions(+), 26 deletions(-)