* [PATCH] ksmbd: remove unnecessary conditions
@ 2021-09-07 7:34 Dan Carpenter
2021-09-07 8:06 ` Sergey Senozhatsky
0 siblings, 1 reply; 8+ messages in thread
From: Dan Carpenter @ 2021-09-07 7:34 UTC (permalink / raw)
To: Namjae Jeon
Cc: Sergey Senozhatsky, Steve French, Hyunchul Lee, linux-cifs,
linux-kernel, kernel-janitors
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(-)
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;
}
}
--
2.20.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] ksmbd: remove unnecessary conditions
2021-09-07 7:34 [PATCH] ksmbd: remove unnecessary conditions Dan Carpenter
@ 2021-09-07 8:06 ` Sergey Senozhatsky
2021-09-07 8:54 ` Dan Carpenter
0 siblings, 1 reply; 8+ messages in thread
From: Sergey Senozhatsky @ 2021-09-07 8:06 UTC (permalink / raw)
To: Dan Carpenter
Cc: Namjae Jeon, Sergey Senozhatsky, Steve French, Hyunchul Lee,
linux-cifs, linux-kernel, kernel-janitors
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;
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] ksmbd: remove unnecessary conditions
2021-09-07 8:06 ` Sergey Senozhatsky
@ 2021-09-07 8:54 ` Dan Carpenter
2021-09-07 9:04 ` Sergey Senozhatsky
0 siblings, 1 reply; 8+ messages in thread
From: Dan Carpenter @ 2021-09-07 8:54 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Namjae Jeon, Steve French, Hyunchul Lee, linux-cifs,
linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] ksmbd: remove unnecessary conditions
2021-09-07 8:54 ` Dan Carpenter
@ 2021-09-07 9:04 ` Sergey Senozhatsky
2021-09-07 9:14 ` Dan Carpenter
0 siblings, 1 reply; 8+ messages in thread
From: Sergey Senozhatsky @ 2021-09-07 9:04 UTC (permalink / raw)
To: Dan Carpenter
Cc: Sergey Senozhatsky, Namjae Jeon, Steve French, Hyunchul Lee,
linux-cifs, linux-kernel, kernel-janitors
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] ksmbd: remove unnecessary conditions
2021-09-07 9:04 ` Sergey Senozhatsky
@ 2021-09-07 9:14 ` Dan Carpenter
2021-09-07 9:59 ` Sergey Senozhatsky
0 siblings, 1 reply; 8+ messages in thread
From: Dan Carpenter @ 2021-09-07 9:14 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Namjae Jeon, Steve French, Hyunchul Lee, linux-cifs,
linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] ksmbd: remove unnecessary conditions
2021-09-07 9:14 ` Dan Carpenter
@ 2021-09-07 9:59 ` Sergey Senozhatsky
2021-09-07 10:17 ` Dan Carpenter
0 siblings, 1 reply; 8+ messages in thread
From: Sergey Senozhatsky @ 2021-09-07 9:59 UTC (permalink / raw)
To: Dan Carpenter
Cc: Sergey Senozhatsky, Namjae Jeon, Steve French, Hyunchul Lee,
linux-cifs, linux-kernel, kernel-janitors
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] ksmbd: remove unnecessary conditions
2021-09-07 9:59 ` Sergey Senozhatsky
@ 2021-09-07 10:17 ` Dan Carpenter
2021-09-07 10:34 ` Sergey Senozhatsky
0 siblings, 1 reply; 8+ messages in thread
From: Dan Carpenter @ 2021-09-07 10:17 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Namjae Jeon, Steve French, Hyunchul Lee, linux-cifs,
linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] ksmbd: remove unnecessary conditions
2021-09-07 10:17 ` Dan Carpenter
@ 2021-09-07 10:34 ` Sergey Senozhatsky
0 siblings, 0 replies; 8+ messages in thread
From: Sergey Senozhatsky @ 2021-09-07 10:34 UTC (permalink / raw)
To: Dan Carpenter
Cc: Sergey Senozhatsky, Namjae Jeon, Steve French, Hyunchul Lee,
linux-cifs, linux-kernel, kernel-janitors
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-09-07 10:34 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-07 7:34 [PATCH] ksmbd: remove unnecessary conditions Dan Carpenter
2021-09-07 8:06 ` Sergey Senozhatsky
2021-09-07 8:54 ` Dan Carpenter
2021-09-07 9:04 ` Sergey Senozhatsky
2021-09-07 9:14 ` Dan Carpenter
2021-09-07 9:59 ` Sergey Senozhatsky
2021-09-07 10:17 ` Dan Carpenter
2021-09-07 10:34 ` Sergey Senozhatsky
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).