kernel-janitors.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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	[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).