[PATCH V2] staging: rtl8192e: Fix signedness bug in rtllib_rx_assoc_resp()

[PATCH V2] staging: rtl8192e: Fix signedness bug in rtllib_rx_assoc_resp()

[Haowen Bai]

function rtllib_rx_assoc_resp () unsigned errcode receive auth_parse()'s
errcode -ENOMEM.

Signed-off-by: Haowen Bai <baihaowen@meizu.com>
---
V1->V2: reduce return random value; print its own error message.

 drivers/staging/rtl8192e/rtllib_softmac.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/rtl8192e/rtllib_softmac.c b/drivers/staging/rtl8192e/rtllib_softmac.c
index 82bf05eb1cbf..8a0961e64a8c 100644
--- a/drivers/staging/rtl8192e/rtllib_softmac.c
+++ b/drivers/staging/rtl8192e/rtllib_softmac.c
@@ -1764,7 +1764,7 @@ static void rtllib_softmac_check_all_nets(struct rtllib_device *ieee)
 	spin_unlock_irqrestore(&ieee->lock, flags);
 }
 
-static inline u16 auth_parse(struct net_device *dev, struct sk_buff *skb,
+static inline s16 auth_parse(struct net_device *dev, struct sk_buff *skb,
 			     u8 **challenge, int *chlen)
 {
 	struct rtllib_authentication *a;
@@ -1773,7 +1773,7 @@ static inline u16 auth_parse(struct net_device *dev, struct sk_buff *skb,
 	if (skb->len <  (sizeof(struct rtllib_authentication) -
 	    sizeof(struct rtllib_info_element))) {
 		netdev_dbg(dev, "invalid len in auth resp: %d\n", skb->len);
-		return 0xcafe;
+		return -EINVAL;
 	}
 	*challenge = NULL;
 	a = (struct rtllib_authentication *) skb->data;
@@ -1787,7 +1787,7 @@ static inline u16 auth_parse(struct net_device *dev, struct sk_buff *skb,
 				return -ENOMEM;
 		}
 	}
-	return le16_to_cpu(a->status);
+	return a->status;
 }
 
 static int auth_rq_parse(struct net_device *dev, struct sk_buff *skb, u8 *dest)
@@ -2282,7 +2282,7 @@ rtllib_rx_assoc_resp(struct rtllib_device *ieee, struct sk_buff *skb,
 
 static void rtllib_rx_auth_resp(struct rtllib_device *ieee, struct sk_buff *skb)
 {
-	u16 errcode;
+	s16 errcode;
 	u8 *challenge;
 	int chlen = 0;
 	bool bSupportNmode = true, bHalfSupportNmode = false;
@@ -2292,8 +2292,8 @@ static void rtllib_rx_auth_resp(struct rtllib_device *ieee, struct sk_buff *skb)
 	if (errcode) {
 		ieee->softmac_stats.rx_auth_rs_err++;
 		netdev_info(ieee->dev,
-			    "Authentication response status code 0x%x",
-			    errcode);
+			    "Authentication response status code %d",
+			    le16_to_cpu(errcode));
 		rtllib_associate_abort(ieee);
 		return;
 	}
-- 
2.7.4

Re: [PATCH V2] staging: rtl8192e: Fix signedness bug in rtllib_rx_assoc_resp()

[Fabio M. De Francesco]

On venerdì 15 aprile 2022 04:09:31 CEST Haowen Bai wrote:
> function rtllib_rx_assoc_resp () unsigned errcode receive auth_parse()'s
> errcode -ENOMEM.
> 
> Signed-off-by: Haowen Bai <baihaowen@meizu.com>
> ---
> V1->V2: reduce return random value; print its own error message.
> 
>  drivers/staging/rtl8192e/rtllib_softmac.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 

> [snip]

It looks like you are doing too many things and that those aren't even 
discussed in your commit message.

> @@ -2292,8 +2292,8 @@ static void rtllib_rx_auth_resp(struct 
rtllib_device *ieee, struct sk_buff *skb)
>  	if (errcode) {
>  		ieee->softmac_stats.rx_auth_rs_err++;
>  		netdev_info(ieee->dev,
> -			    "Authentication response status code 
0x%x",
> -			    errcode);
> +			    "Authentication response status code %d",
> +			    le16_to_cpu(errcode));

Why did you call le16_to_cpu(errcode)? 
If I'm not missing something, it looks that auth_parse() already returns 
native endian u16 values.

Thanks,

Fabio M. De Francesco

Re: [PATCH V2] staging: rtl8192e: Fix signedness bug in rtllib_rx_assoc_resp()

[Dan Carpenter]

On Fri, Apr 15, 2022 at 07:22:36AM +0200, Fabio M. De Francesco wrote:
> On venerdì 15 aprile 2022 04:09:31 CEST Haowen Bai wrote:
> > function rtllib_rx_assoc_resp () unsigned errcode receive auth_parse()'s
> > errcode -ENOMEM.
> > 
> > Signed-off-by: Haowen Bai <baihaowen@meizu.com>
> > ---
> > V1->V2: reduce return random value; print its own error message.
> > 
> >  drivers/staging/rtl8192e/rtllib_softmac.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> 
> > [snip]
> 
> It looks like you are doing too many things and that those aren't even 
> discussed in your commit message.
> 

Nope.  The patch is one thing, but maybe it could be described better.
Here is my proposed commit message:

"The rtllib_rx_assoc_resp() function has a signedness bug because it's
a declared as a u16 but it return -ENOMEM.  When you look at it more
closely it returns a mix of error codes including 0xcafe, -ENOMEM, and
a->status which is WLAN_STATUS_NOT_SUPPORTED_AUTH_ALG.  This is a mess.

Clean it up to just return standard kernel error codes.  We can print
out the a->status before returning a regular error code.  The printks
in the caller need to be adjusted as well."

regards,
dan carpenter

[PATCH V3] staging: rtl8192e: Fix signedness bug in rtllib_rx_assoc_resp()

[Haowen Bai]

The rtllib_rx_assoc_resp() function has a signedness bug because it's
a declared as a u16 but it return -ENOMEM.  When you look at it more
closely it returns a mix of error codes including 0xcafe, -ENOMEM, and
a->status which is WLAN_STATUS_NOT_SUPPORTED_AUTH_ALG.  This is a mess.

Clean it up to just return standard kernel error codes.  We can print
out the a->status before returning a regular error code.  The printks
in the caller need to be adjusted as well.

Signed-off-by: Haowen Bai <baihaowen@meizu.com>
---
V1->V2: reduce return random value; print its own error message.
V2->V3: change commit message; change s16 -> int.

 drivers/staging/rtl8192e/rtllib_softmac.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/rtl8192e/rtllib_softmac.c b/drivers/staging/rtl8192e/rtllib_softmac.c
index 82bf05eb1cbf..4a1b9a94930f 100644
--- a/drivers/staging/rtl8192e/rtllib_softmac.c
+++ b/drivers/staging/rtl8192e/rtllib_softmac.c
@@ -1764,7 +1764,7 @@ static void rtllib_softmac_check_all_nets(struct rtllib_device *ieee)
 	spin_unlock_irqrestore(&ieee->lock, flags);
 }
 
-static inline u16 auth_parse(struct net_device *dev, struct sk_buff *skb,
+static inline int auth_parse(struct net_device *dev, struct sk_buff *skb,
 			     u8 **challenge, int *chlen)
 {
 	struct rtllib_authentication *a;
@@ -1773,7 +1773,7 @@ static inline u16 auth_parse(struct net_device *dev, struct sk_buff *skb,
 	if (skb->len <  (sizeof(struct rtllib_authentication) -
 	    sizeof(struct rtllib_info_element))) {
 		netdev_dbg(dev, "invalid len in auth resp: %d\n", skb->len);
-		return 0xcafe;
+		return -EINVAL;
 	}
 	*challenge = NULL;
 	a = (struct rtllib_authentication *) skb->data;
@@ -1787,7 +1787,7 @@ static inline u16 auth_parse(struct net_device *dev, struct sk_buff *skb,
 				return -ENOMEM;
 		}
 	}
-	return le16_to_cpu(a->status);
+	return a->status;
 }
 
 static int auth_rq_parse(struct net_device *dev, struct sk_buff *skb, u8 *dest)
@@ -2282,7 +2282,7 @@ rtllib_rx_assoc_resp(struct rtllib_device *ieee, struct sk_buff *skb,
 
 static void rtllib_rx_auth_resp(struct rtllib_device *ieee, struct sk_buff *skb)
 {
-	u16 errcode;
+	int errcode;
 	u8 *challenge;
 	int chlen = 0;
 	bool bSupportNmode = true, bHalfSupportNmode = false;
@@ -2292,8 +2292,8 @@ static void rtllib_rx_auth_resp(struct rtllib_device *ieee, struct sk_buff *skb)
 	if (errcode) {
 		ieee->softmac_stats.rx_auth_rs_err++;
 		netdev_info(ieee->dev,
-			    "Authentication response status code 0x%x",
-			    errcode);
+			    "Authentication response status code %d",
+			    le16_to_cpu(errcode));
 		rtllib_associate_abort(ieee);
 		return;
 	}
-- 
2.7.4

Re: [PATCH V3] staging: rtl8192e: Fix signedness bug in rtllib_rx_assoc_resp()

[Fabio M. De Francesco]

On venerdì 15 aprile 2022 07:50:36 CEST Haowen Bai wrote:
> The rtllib_rx_assoc_resp() function has a signedness bug because it's
> a declared as a u16 but it return -ENOMEM.  When you look at it more
> closely it returns a mix of error codes including 0xcafe, -ENOMEM, and
> a->status which is WLAN_STATUS_NOT_SUPPORTED_AUTH_ALG.  This is a mess.
> 
> Clean it up to just return standard kernel error codes.  We can print
> out the a->status before returning a regular error code.  The printks
> in the caller need to be adjusted as well.

This commit message suggested by Dan Carpenter is much better. The previous 
one made me think that you were doing several different logical changes.

>
> [snip]
>  
>  static void rtllib_rx_auth_resp(struct rtllib_device *ieee, struct 
sk_buff *skb)
>  {
> -	u16 errcode;
> +	int errcode;
>  	u8 *challenge;
>  	int chlen = 0;
>  	bool bSupportNmode = true, bHalfSupportNmode = false;
> @@ -2292,8 +2292,8 @@ static void rtllib_rx_auth_resp(struct 
rtllib_device *ieee, struct sk_buff *skb)
>  	if (errcode) {
>  		ieee->softmac_stats.rx_auth_rs_err++;
>  		netdev_info(ieee->dev,
> -			    "Authentication response status code 
0x%x",
> -			    errcode);
> +			    "Authentication response status code %d",
> +			    le16_to_cpu(errcode));

This is something that I'm still missing. Why do we need that call to 
le16_to_cpu on "errcode"?

"errcode" is returned by auth_parse()? I see that this function already 
changes the endianness of the returned value.

Thanks,

Fabio


>  		rtllib_associate_abort(ieee);
>  		return;
>  	}
> -- 
> 2.7.4
> 
> 
> 

Re: [PATCH V3] staging: rtl8192e: Fix signedness bug in rtllib_rx_assoc_resp()

[Fabio M. De Francesco]

On venerdì 15 aprile 2022 08:06:17 CEST Fabio M. De Francesco wrote:
> On venerdì 15 aprile 2022 07:50:36 CEST Haowen Bai wrote:
> > The rtllib_rx_assoc_resp() function has a signedness bug because it's
> > a declared as a u16 but it return -ENOMEM.  When you look at it more
> > closely it returns a mix of error codes including 0xcafe, -ENOMEM, and
> > a->status which is WLAN_STATUS_NOT_SUPPORTED_AUTH_ALG.  This is a mess.
> > 
> > Clean it up to just return standard kernel error codes.  We can print
> > out the a->status before returning a regular error code.  The printks
> > in the caller need to be adjusted as well.
> 
> This commit message suggested by Dan Carpenter is much better. The 
previous 
> one made me think that you were doing several different logical changes.
> 
> >
> > [snip]
> >  
> >  static void rtllib_rx_auth_resp(struct rtllib_device *ieee, struct 
> sk_buff *skb)
> >  {
> > -	u16 errcode;
> > +	int errcode;
> >  	u8 *challenge;
> >  	int chlen = 0;
> >  	bool bSupportNmode = true, bHalfSupportNmode = false;
> > @@ -2292,8 +2292,8 @@ static void rtllib_rx_auth_resp(struct 
> rtllib_device *ieee, struct sk_buff *skb)
> >  	if (errcode) {
> >  		ieee->softmac_stats.rx_auth_rs_err++;
> >  		netdev_info(ieee->dev,
> > -			    "Authentication response status code 
> 0x%x",
> > -			    errcode);
> > +			    "Authentication response status code %d",
> > +			    le16_to_cpu(errcode));
> 
> This is something that I'm still missing. Why do we need that call to 
> le16_to_cpu on "errcode"?
> 
> "errcode" is returned by auth_parse()? I see that this function already 
> changes the endianness of the returned value.

Sorry, I missed that you also changed auth_code().

Fabio

> 
> Thanks,
> 
> Fabio
> 
> 
> >  		rtllib_associate_abort(ieee);
> >  		return;
> >  	}
> > -- 
> > 2.7.4
> > 
> > 
> > 
> 
> 
> 
> 
> 

[PATCH V4] staging: rtl8192e: Fix signedness bug in rtllib_rx_assoc_resp()

[Haowen Bai]

This commit message suggested by Dan Carpenter as below:

The rtllib_rx_assoc_resp() function has a signedness bug because it's
a declared as a u16 but it return -ENOMEM.  When you look at it more
closely it returns a mix of error codes including 0xcafe, -ENOMEM, and
a->status which is WLAN_STATUS_NOT_SUPPORTED_AUTH_ALG.  This is a mess.

Clean it up to just return standard kernel error codes.  We can print
out the a->status before returning a regular error code.  The printks
in the caller need to be adjusted as well.

Signed-off-by: Haowen Bai <baihaowen@meizu.com>
---
V1->V2: reduce return random value; print its own error message.
V2->V3: change commit message; change s16 -> int.
V3->V4: add message suggested by in title.

 drivers/staging/rtl8192e/rtllib_softmac.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/rtl8192e/rtllib_softmac.c b/drivers/staging/rtl8192e/rtllib_softmac.c
index 82bf05eb1cbf..4a1b9a94930f 100644
--- a/drivers/staging/rtl8192e/rtllib_softmac.c
+++ b/drivers/staging/rtl8192e/rtllib_softmac.c
@@ -1764,7 +1764,7 @@ static void rtllib_softmac_check_all_nets(struct rtllib_device *ieee)
 	spin_unlock_irqrestore(&ieee->lock, flags);
 }
 
-static inline u16 auth_parse(struct net_device *dev, struct sk_buff *skb,
+static inline int auth_parse(struct net_device *dev, struct sk_buff *skb,
 			     u8 **challenge, int *chlen)
 {
 	struct rtllib_authentication *a;
@@ -1773,7 +1773,7 @@ static inline u16 auth_parse(struct net_device *dev, struct sk_buff *skb,
 	if (skb->len <  (sizeof(struct rtllib_authentication) -
 	    sizeof(struct rtllib_info_element))) {
 		netdev_dbg(dev, "invalid len in auth resp: %d\n", skb->len);
-		return 0xcafe;
+		return -EINVAL;
 	}
 	*challenge = NULL;
 	a = (struct rtllib_authentication *) skb->data;
@@ -1787,7 +1787,7 @@ static inline u16 auth_parse(struct net_device *dev, struct sk_buff *skb,
 				return -ENOMEM;
 		}
 	}
-	return le16_to_cpu(a->status);
+	return a->status;
 }
 
 static int auth_rq_parse(struct net_device *dev, struct sk_buff *skb, u8 *dest)
@@ -2282,7 +2282,7 @@ rtllib_rx_assoc_resp(struct rtllib_device *ieee, struct sk_buff *skb,
 
 static void rtllib_rx_auth_resp(struct rtllib_device *ieee, struct sk_buff *skb)
 {
-	u16 errcode;
+	int errcode;
 	u8 *challenge;
 	int chlen = 0;
 	bool bSupportNmode = true, bHalfSupportNmode = false;
@@ -2292,8 +2292,8 @@ static void rtllib_rx_auth_resp(struct rtllib_device *ieee, struct sk_buff *skb)
 	if (errcode) {
 		ieee->softmac_stats.rx_auth_rs_err++;
 		netdev_info(ieee->dev,
-			    "Authentication response status code 0x%x",
-			    errcode);
+			    "Authentication response status code %d",
+			    le16_to_cpu(errcode));
 		rtllib_associate_abort(ieee);
 		return;
 	}
-- 
2.7.4

Re: [PATCH V4] staging: rtl8192e: Fix signedness bug in rtllib_rx_assoc_resp()

[Dan Carpenter]

On Fri, Apr 15, 2022 at 02:15:25PM +0800, Haowen Bai wrote:
> This commit message suggested by Dan Carpenter as below:
> 
> The rtllib_rx_assoc_resp() function has a signedness bug because it's
> a declared as a u16 but it return -ENOMEM.  When you look at it more
> closely it returns a mix of error codes including 0xcafe, -ENOMEM, and
> a->status which is WLAN_STATUS_NOT_SUPPORTED_AUTH_ALG.  This is a mess.
> 
> Clean it up to just return standard kernel error codes.  We can print
> out the a->status before returning a regular error code.  The printks
> in the caller need to be adjusted as well.
> 
> Signed-off-by: Haowen Bai <baihaowen@meizu.com>
> ---
> V1->V2: reduce return random value; print its own error message.
> V2->V3: change commit message; change s16 -> int.
> V3->V4: add message suggested by in title.
> 
>  drivers/staging/rtl8192e/rtllib_softmac.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/rtl8192e/rtllib_softmac.c b/drivers/staging/rtl8192e/rtllib_softmac.c
> index 82bf05eb1cbf..4a1b9a94930f 100644
> --- a/drivers/staging/rtl8192e/rtllib_softmac.c
> +++ b/drivers/staging/rtl8192e/rtllib_softmac.c
> @@ -1764,7 +1764,7 @@ static void rtllib_softmac_check_all_nets(struct rtllib_device *ieee)
>  	spin_unlock_irqrestore(&ieee->lock, flags);
>  }
>  
> -static inline u16 auth_parse(struct net_device *dev, struct sk_buff *skb,
> +static inline int auth_parse(struct net_device *dev, struct sk_buff *skb,
>  			     u8 **challenge, int *chlen)
>  {
>  	struct rtllib_authentication *a;
> @@ -1773,7 +1773,7 @@ static inline u16 auth_parse(struct net_device *dev, struct sk_buff *skb,
>  	if (skb->len <  (sizeof(struct rtllib_authentication) -
>  	    sizeof(struct rtllib_info_element))) {
>  		netdev_dbg(dev, "invalid len in auth resp: %d\n", skb->len);
> -		return 0xcafe;
> +		return -EINVAL;
>  	}
>  	*challenge = NULL;
>  	a = (struct rtllib_authentication *) skb->data;
> @@ -1787,7 +1787,7 @@ static inline u16 auth_parse(struct net_device *dev, struct sk_buff *skb,
>  				return -ENOMEM;
>  		}
>  	}
> -	return le16_to_cpu(a->status);
> +	return a->status;

See previous responses.

regards,
dan carpenter

[PATCH V4] staging: rtl8192e: Fix signedness bug in rtllib_rx_assoc_resp()

[Haowen Bai]

This commit message suggested by Dan Carpenter as below:

The rtllib_rx_assoc_resp() function has a signedness bug because it's
a declared as a u16 but it return -ENOMEM.  When you look at it more
closely it returns a mix of error codes including 0xcafe, -ENOMEM, and
a->status which is WLAN_STATUS_NOT_SUPPORTED_AUTH_ALG.  This is a mess.

Clean it up to just return standard kernel error codes.  We can print
out the a->status before returning a regular error code.  The printks
in the caller need to be adjusted as well.

Signed-off-by: Haowen Bai <baihaowen@meizu.com>
---
V1->V2: reduce return random value; print its own error message.
V2->V3: change commit message; change s16 -> int.
V3->V4: add message suggested by in title. If you look up what a->status 
is, it can only be WLAN_STATUS_NOT_SUPPORTED_AUTH_ALG which is not worth 
preserving really

 drivers/staging/rtl8192e/rtllib_softmac.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/rtl8192e/rtllib_softmac.c b/drivers/staging/rtl8192e/rtllib_softmac.c
index 82bf05eb1cbf..ac0d131c1248 100644
--- a/drivers/staging/rtl8192e/rtllib_softmac.c
+++ b/drivers/staging/rtl8192e/rtllib_softmac.c
@@ -1764,7 +1764,7 @@ static void rtllib_softmac_check_all_nets(struct rtllib_device *ieee)
 	spin_unlock_irqrestore(&ieee->lock, flags);
 }
 
-static inline u16 auth_parse(struct net_device *dev, struct sk_buff *skb,
+static inline int auth_parse(struct net_device *dev, struct sk_buff *skb,
 			     u8 **challenge, int *chlen)
 {
 	struct rtllib_authentication *a;
@@ -1773,7 +1773,7 @@ static inline u16 auth_parse(struct net_device *dev, struct sk_buff *skb,
 	if (skb->len <  (sizeof(struct rtllib_authentication) -
 	    sizeof(struct rtllib_info_element))) {
 		netdev_dbg(dev, "invalid len in auth resp: %d\n", skb->len);
-		return 0xcafe;
+		return -EINVAL;
 	}
 	*challenge = NULL;
 	a = (struct rtllib_authentication *) skb->data;
@@ -1787,7 +1787,13 @@ static inline u16 auth_parse(struct net_device *dev, struct sk_buff *skb,
 				return -ENOMEM;
 		}
 	}
-	return le16_to_cpu(a->status);
+
+	if (a->status) {
+		netdev_info(ieee->dev, "auth_parse() failed");
+		return -EINVAL;
+	}
+
+	return 0;
 }
 
 static int auth_rq_parse(struct net_device *dev, struct sk_buff *skb, u8 *dest)
@@ -2282,7 +2288,7 @@ rtllib_rx_assoc_resp(struct rtllib_device *ieee, struct sk_buff *skb,
 
 static void rtllib_rx_auth_resp(struct rtllib_device *ieee, struct sk_buff *skb)
 {
-	u16 errcode;
+	int errcode;
 	u8 *challenge;
 	int chlen = 0;
 	bool bSupportNmode = true, bHalfSupportNmode = false;
@@ -2292,8 +2298,8 @@ static void rtllib_rx_auth_resp(struct rtllib_device *ieee, struct sk_buff *skb)
 	if (errcode) {
 		ieee->softmac_stats.rx_auth_rs_err++;
 		netdev_info(ieee->dev,
-			    "Authentication response status code 0x%x",
-			    errcode);
+			    "Authentication response status code %d",
+			    le16_to_cpu(errcode));
 		rtllib_associate_abort(ieee);
 		return;
 	}
-- 
2.7.4

Re: [PATCH V4] staging: rtl8192e: Fix signedness bug in rtllib_rx_assoc_resp()

[Dan Carpenter]

On Fri, Apr 15, 2022 at 02:39:27PM +0800, Haowen Bai wrote:
> This commit message suggested by Dan Carpenter as below:

You don't need to add this.  If you feel you must then put it below the
--- cut off line and it will be stored on lore.kernel.org until the end
of time.

> @@ -2292,8 +2298,8 @@ static void rtllib_rx_auth_resp(struct rtllib_device *ieee, struct sk_buff *skb)
>  	if (errcode) {
>  		ieee->softmac_stats.rx_auth_rs_err++;
>  		netdev_info(ieee->dev,
> -			    "Authentication response status code 0x%x",
> -			    errcode);
> +			    "Authentication response status code %d",
> +			    le16_to_cpu(errcode));

The error code is not a le16.  It's just an int.  The way to prevent
these kinds of issues in the future is to run Sparse with the endian
checking enabled.

https://lwn.net/Articles/205624/

regards,
dan carpenter

[PATCH V4] staging: rtl8192e: Fix signedness bug in rtllib_rx_assoc_resp()

[Haowen Bai]

The rtllib_rx_assoc_resp() function has a signedness bug because it's
a declared as a u16 but it return -ENOMEM.  When you look at it more
closely it returns a mix of error codes including 0xcafe, -ENOMEM, and
a->status which is WLAN_STATUS_NOT_SUPPORTED_AUTH_ALG.  This is a mess.

Clean it up to just return standard kernel error codes.  We can print
out the a->status before returning a regular error code.  The printks
in the caller need to be adjusted as well.

Signed-off-by: Haowen Bai <baihaowen@meizu.com>
---
V1->V2: reduce return random value; print its own error message.
V2->V3: change commit message; change s16 -> int.
V3->V4: add message suggested by Dan Carpenter. If you look up what 
a->status is, it can only be WLAN_STATUS_NOT_SUPPORTED_AUTH_ALG which 
is not worth preserving really.

 drivers/staging/rtl8192e/rtllib_softmac.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/rtl8192e/rtllib_softmac.c b/drivers/staging/rtl8192e/rtllib_softmac.c
index 82bf05eb1cbf..38ac733c3245 100644
--- a/drivers/staging/rtl8192e/rtllib_softmac.c
+++ b/drivers/staging/rtl8192e/rtllib_softmac.c
@@ -1764,7 +1764,7 @@ static void rtllib_softmac_check_all_nets(struct rtllib_device *ieee)
 	spin_unlock_irqrestore(&ieee->lock, flags);
 }
 
-static inline u16 auth_parse(struct net_device *dev, struct sk_buff *skb,
+static inline int auth_parse(struct net_device *dev, struct sk_buff *skb,
 			     u8 **challenge, int *chlen)
 {
 	struct rtllib_authentication *a;
@@ -1773,7 +1773,7 @@ static inline u16 auth_parse(struct net_device *dev, struct sk_buff *skb,
 	if (skb->len <  (sizeof(struct rtllib_authentication) -
 	    sizeof(struct rtllib_info_element))) {
 		netdev_dbg(dev, "invalid len in auth resp: %d\n", skb->len);
-		return 0xcafe;
+		return -EINVAL;
 	}
 	*challenge = NULL;
 	a = (struct rtllib_authentication *) skb->data;
@@ -1787,7 +1787,13 @@ static inline u16 auth_parse(struct net_device *dev, struct sk_buff *skb,
 				return -ENOMEM;
 		}
 	}
-	return le16_to_cpu(a->status);
+
+	if (a->status) {
+		netdev_info(ieee->dev, "auth_parse() failed");
+		return -EINVAL;
+	}
+
+	return 0;
 }
 
 static int auth_rq_parse(struct net_device *dev, struct sk_buff *skb, u8 *dest)
@@ -2282,7 +2288,7 @@ rtllib_rx_assoc_resp(struct rtllib_device *ieee, struct sk_buff *skb,
 
 static void rtllib_rx_auth_resp(struct rtllib_device *ieee, struct sk_buff *skb)
 {
-	u16 errcode;
+	int errcode;
 	u8 *challenge;
 	int chlen = 0;
 	bool bSupportNmode = true, bHalfSupportNmode = false;
@@ -2292,8 +2298,7 @@ static void rtllib_rx_auth_resp(struct rtllib_device *ieee, struct sk_buff *skb)
 	if (errcode) {
 		ieee->softmac_stats.rx_auth_rs_err++;
 		netdev_info(ieee->dev,
-			    "Authentication response status code 0x%x",
-			    errcode);
+			    "Authentication response status code %d", errcode);
 		rtllib_associate_abort(ieee);
 		return;
 	}
-- 
2.7.4

Re: [PATCH V4] staging: rtl8192e: Fix signedness bug in rtllib_rx_assoc_resp()

[Greg Kroah-Hartman]

On Fri, Apr 15, 2022 at 03:21:35PM +0800, Haowen Bai wrote:
> The rtllib_rx_assoc_resp() function has a signedness bug because it's
> a declared as a u16 but it return -ENOMEM.  When you look at it more
> closely it returns a mix of error codes including 0xcafe, -ENOMEM, and
> a->status which is WLAN_STATUS_NOT_SUPPORTED_AUTH_ALG.  This is a mess.
> 
> Clean it up to just return standard kernel error codes.  We can print
> out the a->status before returning a regular error code.  The printks
> in the caller need to be adjusted as well.
> 
> Signed-off-by: Haowen Bai <baihaowen@meizu.com>
> ---
> V1->V2: reduce return random value; print its own error message.
> V2->V3: change commit message; change s16 -> int.
> V3->V4: add message suggested by Dan Carpenter. If you look up what 
> a->status is, it can only be WLAN_STATUS_NOT_SUPPORTED_AUTH_ALG which 
> is not worth preserving really.

I see 3 different v4 patches.  Obviously something went wrong, please
submit a new one, and properly number it and say what changed between
them all.

thanks,

greg k-h

[PATCH V4] staging: rtl8192e: Fix signedness bug in rtllib_rx_assoc_resp()

[Haowen Bai]

The rtllib_rx_assoc_resp() function has a signedness bug because it's
a declared as a u16 but it return -ENOMEM.  When you look at it more
closely it returns a mix of error codes including 0xcafe, -ENOMEM, and
a->status which is WLAN_STATUS_NOT_SUPPORTED_AUTH_ALG.  This is a mess.

Clean it up to just return standard kernel error codes.  We can print
out the a->status before returning a regular error code.  The printks
in the caller need to be adjusted as well.

Signed-off-by: Haowen Bai <baihaowen@meizu.com>
---
V1->V2: reduce return random value; print its own error message.
V2->V3: change commit message; change s16 -> int.
V3->V4: 
1. change message suggested by Dan Carpenter;
2. hold a->status in auth_parse() and return error code or 0 on success.
3. print le16_to_cpu(errcode) -> int %d.

 drivers/staging/rtl8192e/rtllib_softmac.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/rtl8192e/rtllib_softmac.c b/drivers/staging/rtl8192e/rtllib_softmac.c
index 82bf05eb1cbf..38ac733c3245 100644
--- a/drivers/staging/rtl8192e/rtllib_softmac.c
+++ b/drivers/staging/rtl8192e/rtllib_softmac.c
@@ -1764,7 +1764,7 @@ static void rtllib_softmac_check_all_nets(struct rtllib_device *ieee)
 	spin_unlock_irqrestore(&ieee->lock, flags);
 }
 
-static inline u16 auth_parse(struct net_device *dev, struct sk_buff *skb,
+static inline int auth_parse(struct net_device *dev, struct sk_buff *skb,
 			     u8 **challenge, int *chlen)
 {
 	struct rtllib_authentication *a;
@@ -1773,7 +1773,7 @@ static inline u16 auth_parse(struct net_device *dev, struct sk_buff *skb,
 	if (skb->len <  (sizeof(struct rtllib_authentication) -
 	    sizeof(struct rtllib_info_element))) {
 		netdev_dbg(dev, "invalid len in auth resp: %d\n", skb->len);
-		return 0xcafe;
+		return -EINVAL;
 	}
 	*challenge = NULL;
 	a = (struct rtllib_authentication *) skb->data;
@@ -1787,7 +1787,13 @@ static inline u16 auth_parse(struct net_device *dev, struct sk_buff *skb,
 				return -ENOMEM;
 		}
 	}
-	return le16_to_cpu(a->status);
+
+	if (a->status) {
+		netdev_info(ieee->dev, "auth_parse() failed");
+		return -EINVAL;
+	}
+
+	return 0;
 }
 
 static int auth_rq_parse(struct net_device *dev, struct sk_buff *skb, u8 *dest)
@@ -2282,7 +2288,7 @@ rtllib_rx_assoc_resp(struct rtllib_device *ieee, struct sk_buff *skb,
 
 static void rtllib_rx_auth_resp(struct rtllib_device *ieee, struct sk_buff *skb)
 {
-	u16 errcode;
+	int errcode;
 	u8 *challenge;
 	int chlen = 0;
 	bool bSupportNmode = true, bHalfSupportNmode = false;
@@ -2292,8 +2298,7 @@ static void rtllib_rx_auth_resp(struct rtllib_device *ieee, struct sk_buff *skb)
 	if (errcode) {
 		ieee->softmac_stats.rx_auth_rs_err++;
 		netdev_info(ieee->dev,
-			    "Authentication response status code 0x%x",
-			    errcode);
+			    "Authentication response status code %d", errcode);
 		rtllib_associate_abort(ieee);
 		return;
 	}
-- 
2.7.4

Re: [PATCH V4] staging: rtl8192e: Fix signedness bug in rtllib_rx_assoc_resp()

[kernel test robot]

Hi Haowen,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on v5.18-rc3]
[also build test ERROR on next-20220421]
[cannot apply to staging/staging-testing]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Haowen-Bai/staging-rtl8192e-Fix-signedness-bug-in-rtllib_rx_assoc_resp/20220421-093531
base:    b2d229d4ddb17db541098b83524d901257e93845
config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20220421/202204211558.TapLZO4j-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/e1395cdafd4dea7a368f2d9599832636035a03d2
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Haowen-Bai/staging-rtl8192e-Fix-signedness-bug-in-rtllib_rx_assoc_resp/20220421-093531
        git checkout e1395cdafd4dea7a368f2d9599832636035a03d2
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash drivers/staging/rtl8192e/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/staging/rtl8192e/rtllib_softmac.c: In function 'auth_parse':
>> drivers/staging/rtl8192e/rtllib_softmac.c:1792:29: error: 'ieee' undeclared (first use in this function)
    1792 |                 netdev_info(ieee->dev, "auth_parse() failed");
         |                             ^~~~
   drivers/staging/rtl8192e/rtllib_softmac.c:1792:29: note: each undeclared identifier is reported only once for each function it appears in


vim +/ieee +1792 drivers/staging/rtl8192e/rtllib_softmac.c

  1766	
  1767	static inline int auth_parse(struct net_device *dev, struct sk_buff *skb,
  1768				     u8 **challenge, int *chlen)
  1769	{
  1770		struct rtllib_authentication *a;
  1771		u8 *t;
  1772	
  1773		if (skb->len <  (sizeof(struct rtllib_authentication) -
  1774		    sizeof(struct rtllib_info_element))) {
  1775			netdev_dbg(dev, "invalid len in auth resp: %d\n", skb->len);
  1776			return -EINVAL;
  1777		}
  1778		*challenge = NULL;
  1779		a = (struct rtllib_authentication *) skb->data;
  1780		if (skb->len > (sizeof(struct rtllib_authentication) + 3)) {
  1781			t = skb->data + sizeof(struct rtllib_authentication);
  1782	
  1783			if (*(t++) == MFIE_TYPE_CHALLENGE) {
  1784				*chlen = *(t++);
  1785				*challenge = kmemdup(t, *chlen, GFP_ATOMIC);
  1786				if (!*challenge)
  1787					return -ENOMEM;
  1788			}
  1789		}
  1790	
  1791		if (a->status) {
> 1792			netdev_info(ieee->dev, "auth_parse() failed");
  1793			return -EINVAL;
  1794		}
  1795	
  1796		return 0;
  1797	}
  1798	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

[PATCH V5] staging: rtl8192e: Fix signedness bug in rtllib_rx_assoc_resp()

[Haowen Bai]

The rtllib_rx_assoc_resp() function has a signedness bug because it's
a declared as a u16 but it return -ENOMEM.  When you look at it more
closely it returns a mix of error codes including 0xcafe, -ENOMEM, and
a->status which is WLAN_STATUS_NOT_SUPPORTED_AUTH_ALG.  This is a mess.

Clean it up to just return standard kernel error codes.  We can print
out the a->status before returning a regular error code.  The printks
in the caller need to be adjusted as well.

Signed-off-by: Haowen Bai <baihaowen@meizu.com>
---
V1->V2: reduce return random value; print its own error message.
V2->V3: change commit message; change s16 -> int.
V3->V4: 
1. change message suggested by Dan Carpenter;
2. hold a->status in auth_parse() and return error code or 0 on success.
3. print le16_to_cpu(errcode) -> int %d.
V4->V5: fix compile error.

 drivers/staging/rtl8192e/rtllib_softmac.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/rtl8192e/rtllib_softmac.c b/drivers/staging/rtl8192e/rtllib_softmac.c
index 82bf05eb1cbf..38ac733c3245 100644
--- a/drivers/staging/rtl8192e/rtllib_softmac.c
+++ b/drivers/staging/rtl8192e/rtllib_softmac.c
@@ -1764,7 +1764,7 @@ static void rtllib_softmac_check_all_nets(struct rtllib_device *ieee)
 	spin_unlock_irqrestore(&ieee->lock, flags);
 }
 
-static inline u16 auth_parse(struct net_device *dev, struct sk_buff *skb,
+static inline int auth_parse(struct net_device *dev, struct sk_buff *skb,
 			     u8 **challenge, int *chlen)
 {
 	struct rtllib_authentication *a;
@@ -1773,7 +1773,7 @@ static inline u16 auth_parse(struct net_device *dev, struct sk_buff *skb,
 	if (skb->len <  (sizeof(struct rtllib_authentication) -
 	    sizeof(struct rtllib_info_element))) {
 		netdev_dbg(dev, "invalid len in auth resp: %d\n", skb->len);
-		return 0xcafe;
+		return -EINVAL;
 	}
 	*challenge = NULL;
 	a = (struct rtllib_authentication *) skb->data;
@@ -1787,7 +1787,13 @@ static inline u16 auth_parse(struct net_device *dev, struct sk_buff *skb,
 				return -ENOMEM;
 		}
 	}
-	return le16_to_cpu(a->status);
+
+	if (a->status) {
+		netdev_dbg(dev, "auth_parse() failed\n");
+		return -EINVAL;
+	}
+
+	return 0;
 }
 
 static int auth_rq_parse(struct net_device *dev, struct sk_buff *skb, u8 *dest)
@@ -2282,7 +2288,7 @@ rtllib_rx_assoc_resp(struct rtllib_device *ieee, struct sk_buff *skb,
 
 static void rtllib_rx_auth_resp(struct rtllib_device *ieee, struct sk_buff *skb)
 {
-	u16 errcode;
+	int errcode;
 	u8 *challenge;
 	int chlen = 0;
 	bool bSupportNmode = true, bHalfSupportNmode = false;
@@ -2292,8 +2298,7 @@ static void rtllib_rx_auth_resp(struct rtllib_device *ieee, struct sk_buff *skb)
 	if (errcode) {
 		ieee->softmac_stats.rx_auth_rs_err++;
 		netdev_info(ieee->dev,
-			    "Authentication response status code 0x%x",
-			    errcode);
+			    "Authentication response status code %d", errcode);
 		rtllib_associate_abort(ieee);
 		return;
 	}
-- 
2.7.4

Re: [PATCH V5] staging: rtl8192e: Fix signedness bug in rtllib_rx_assoc_resp()

[Dan Carpenter]

On Thu, Apr 21, 2022 at 04:21:17PM +0800, Haowen Bai wrote:
> The rtllib_rx_assoc_resp() function has a signedness bug because it's
> a declared as a u16 but it return -ENOMEM.  When you look at it more
> closely it returns a mix of error codes including 0xcafe, -ENOMEM, and
> a->status which is WLAN_STATUS_NOT_SUPPORTED_AUTH_ALG.  This is a mess.
> 
> Clean it up to just return standard kernel error codes.  We can print
> out the a->status before returning a regular error code.  The printks
> in the caller need to be adjusted as well.
> 
> Signed-off-by: Haowen Bai <baihaowen@meizu.com>
> ---
> V1->V2: reduce return random value; print its own error message.
> V2->V3: change commit message; change s16 -> int.
> V3->V4: 
> 1. change message suggested by Dan Carpenter;
> 2. hold a->status in auth_parse() and return error code or 0 on success.
> 3. print le16_to_cpu(errcode) -> int %d.
> V4->V5: fix compile error.
> 

Looks ok.

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter

Re: [PATCH V4] staging: rtl8192e: Fix signedness bug in rtllib_rx_assoc_resp()

[baihaowen]

在 4/15/22 2:58 PM, Dan Carpenter 写道:
> On Fri, Apr 15, 2022 at 02:39:27PM +0800, Haowen Bai wrote:
>> This commit message suggested by Dan Carpenter as below:
> You don't need to add this.  If you feel you must then put it below the
> --- cut off line and it will be stored on lore.kernel.org until the end
> of time.
>
>> @@ -2292,8 +2298,8 @@ static void rtllib_rx_auth_resp(struct rtllib_device *ieee, struct sk_buff *skb)
>>  	if (errcode) {
>>  		ieee->softmac_stats.rx_auth_rs_err++;
>>  		netdev_info(ieee->dev,
>> -			    "Authentication response status code 0x%x",
>> -			    errcode);
>> +			    "Authentication response status code %d",
>> +			    le16_to_cpu(errcode));
> The error code is not a le16.  It's just an int.  The way to prevent
> these kinds of issues in the future is to run Sparse with the endian
> checking enabled.
>
> https://lwn.net/Articles/205624/
>
> regards,
> dan carpenter
>
Dear Dan Carpenter
Got it, thank you for your professional and patience.

-- 
Haowen Bai

Re: [PATCH V3] staging: rtl8192e: Fix signedness bug in rtllib_rx_assoc_resp()

[Dan Carpenter]

On Fri, Apr 15, 2022 at 01:50:36PM +0800, Haowen Bai wrote:
> The rtllib_rx_assoc_resp() function has a signedness bug because it's
> a declared as a u16 but it return -ENOMEM.  When you look at it more
> closely it returns a mix of error codes including 0xcafe, -ENOMEM, and
> a->status which is WLAN_STATUS_NOT_SUPPORTED_AUTH_ALG.  This is a mess.
> 
> Clean it up to just return standard kernel error codes.  We can print
> out the a->status before returning a regular error code.  The printks
> in the caller need to be adjusted as well.
> 
> Signed-off-by: Haowen Bai <baihaowen@meizu.com>
> ---
> V1->V2: reduce return random value; print its own error message.
> V2->V3: change commit message; change s16 -> int.
> 
>  drivers/staging/rtl8192e/rtllib_softmac.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/rtl8192e/rtllib_softmac.c b/drivers/staging/rtl8192e/rtllib_softmac.c
> index 82bf05eb1cbf..4a1b9a94930f 100644
> --- a/drivers/staging/rtl8192e/rtllib_softmac.c
> +++ b/drivers/staging/rtl8192e/rtllib_softmac.c
> @@ -1764,7 +1764,7 @@ static void rtllib_softmac_check_all_nets(struct rtllib_device *ieee)
>  	spin_unlock_irqrestore(&ieee->lock, flags);
>  }
>  
> -static inline u16 auth_parse(struct net_device *dev, struct sk_buff *skb,
> +static inline int auth_parse(struct net_device *dev, struct sk_buff *skb,
>  			     u8 **challenge, int *chlen)
>  {
>  	struct rtllib_authentication *a;
> @@ -1773,7 +1773,7 @@ static inline u16 auth_parse(struct net_device *dev, struct sk_buff *skb,
>  	if (skb->len <  (sizeof(struct rtllib_authentication) -
>  	    sizeof(struct rtllib_info_element))) {
>  		netdev_dbg(dev, "invalid len in auth resp: %d\n", skb->len);
> -		return 0xcafe;
> +		return -EINVAL;
>  	}
>  	*challenge = NULL;
>  	a = (struct rtllib_authentication *) skb->data;
> @@ -1787,7 +1787,7 @@ static inline u16 auth_parse(struct net_device *dev, struct sk_buff *skb,
>  				return -ENOMEM;
>  		}
>  	}
> -	return le16_to_cpu(a->status);
> +	return a->status;

Please do not return a->status.  See my previous email.  You're sending
new versions too quickly.  Wait a day between new versions.

regards,
dan carpenter

Re: [PATCH V2] staging: rtl8192e: Fix signedness bug in rtllib_rx_assoc_resp()

[Dan Carpenter]

On Fri, Apr 15, 2022 at 10:09:31AM +0800, Haowen Bai wrote:
> function rtllib_rx_assoc_resp () unsigned errcode receive auth_parse()'s
> errcode -ENOMEM.
> 
> Signed-off-by: Haowen Bai <baihaowen@meizu.com>
> ---
> V1->V2: reduce return random value; print its own error message.
> 
>  drivers/staging/rtl8192e/rtllib_softmac.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/rtl8192e/rtllib_softmac.c b/drivers/staging/rtl8192e/rtllib_softmac.c
> index 82bf05eb1cbf..8a0961e64a8c 100644
> --- a/drivers/staging/rtl8192e/rtllib_softmac.c
> +++ b/drivers/staging/rtl8192e/rtllib_softmac.c
> @@ -1764,7 +1764,7 @@ static void rtllib_softmac_check_all_nets(struct rtllib_device *ieee)
>  	spin_unlock_irqrestore(&ieee->lock, flags);
>  }
>  
> -static inline u16 auth_parse(struct net_device *dev, struct sk_buff *skb,
> +static inline s16 auth_parse(struct net_device *dev, struct sk_buff *skb,

Could you make this an int instead of s16.  s16 is always a bit weird.

>  			     u8 **challenge, int *chlen)
>  {
>  	struct rtllib_authentication *a;
> @@ -1773,7 +1773,7 @@ static inline u16 auth_parse(struct net_device *dev, struct sk_buff *skb,
>  	if (skb->len <  (sizeof(struct rtllib_authentication) -
>  	    sizeof(struct rtllib_info_element))) {
>  		netdev_dbg(dev, "invalid len in auth resp: %d\n", skb->len);
> -		return 0xcafe;
> +		return -EINVAL;
>  	}
>  	*challenge = NULL;
>  	a = (struct rtllib_authentication *) skb->data;
> @@ -1787,7 +1787,7 @@ static inline u16 auth_parse(struct net_device *dev, struct sk_buff *skb,
>  				return -ENOMEM;
>  		}
>  	}
> -	return le16_to_cpu(a->status);
> +	return a->status;

But then just say:

	if (a->status) {
		netdev_info(ieee->dev, "blah blah failed");
		return -EINVAL;
	}

	return 0;

If you look up what a->status is, it can only be
WLAN_STATUS_NOT_SUPPORTED_AUTH_ALG which is not worth preserving really.

regards,
dan carpenter