[PATCH] ksmbd: remove unnecessary conditions

[PATCH] ksmbd: remove unnecessary conditions

[Dan Carpenter]

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

Re: [PATCH] ksmbd: remove unnecessary conditions

[Sergey Senozhatsky]

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;

Re: [PATCH] ksmbd: remove unnecessary conditions

[Dan Carpenter]

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

Re: [PATCH] ksmbd: remove unnecessary conditions

[Sergey Senozhatsky]

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.

Re: [PATCH] ksmbd: remove unnecessary conditions

[Dan Carpenter]

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

Re: [PATCH] ksmbd: remove unnecessary conditions

[Sergey Senozhatsky]

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.

Re: [PATCH] ksmbd: remove unnecessary conditions

[Dan Carpenter]

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

Re: [PATCH] ksmbd: remove unnecessary conditions

[Sergey Senozhatsky]

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.