[PATCH] staging: r8188eu: fix a potential integer underflow bug

public inbox for linux-staging@lists.linux.dev
 help / color / mirror / Atom feed

* [PATCH] staging: r8188eu: fix a potential integer underflow bug
@ 2023-02-22 13:59 Dan Carpenter
  2023-02-23  7:00 ` Philipp Hortmann
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Dan Carpenter @ 2023-02-22 13:59 UTC (permalink / raw)
  To: Phillip Potter
  Cc: Pavel Skripkin, Greg Kroah-Hartman, Deepak R Varma, Charlie Sands,
	Mahak Gupta, Alaa Mohamed, linux-staging, kernel-janitors

Here the code is testing to see if skb->len meets a minimum size
requirement.  However if skb->len is very small then the ETH_HLEN
subtraction will result in a negative which is then type promoted
to an unsigned int and the condition will be true.

Generally, when you have an untrusted variable like skb->len, you
should move all the math to the other side of the comparison.

Fixes: 15865124feed ("staging: r8188eu: introduce new core dir for RTL8188eu driver")
Signed-off-by: Dan Carpenter <error27@gmail.com>
---
Compile tested only.  This is basic algebra of moving parts of the
equation from one side to the other and I am surprisingly bad at
something that I was supposed to have learned in 9th grade.

 drivers/staging/r8188eu/core/rtw_br_ext.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c
index a7c67014dde0..f49e32c33372 100644
--- a/drivers/staging/r8188eu/core/rtw_br_ext.c
+++ b/drivers/staging/r8188eu/core/rtw_br_ext.c
@@ -538,7 +538,7 @@ int nat25_db_handle(struct adapter *priv, struct sk_buff *skb, int method)
 		/*------------------------------------------------*/
 		struct ipv6hdr *iph = (struct ipv6hdr *)(skb->data + ETH_HLEN);
 
-		if (sizeof(*iph) >= (skb->len - ETH_HLEN))
+		if (skb->len <= sizeof(*iph) + ETH_HLEN)
 			return -1;
 
 		switch (method) {
-- 
2.39.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] staging: r8188eu: fix a potential integer underflow bug
  2023-02-22 13:59 [PATCH] staging: r8188eu: fix a potential integer underflow bug Dan Carpenter
@ 2023-02-23  7:00 ` Philipp Hortmann
  2023-02-23 11:00 ` Pavel Skripkin
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 6+ messages in thread
From: Philipp Hortmann @ 2023-02-23  7:00 UTC (permalink / raw)
  To: Dan Carpenter, Phillip Potter
  Cc: Pavel Skripkin, Greg Kroah-Hartman, Deepak R Varma, Charlie Sands,
	Mahak Gupta, Alaa Mohamed, linux-staging, kernel-janitors

On 2/22/23 14:59, Dan Carpenter wrote:
> Here the code is testing to see if skb->len meets a minimum size
> requirement.  However if skb->len is very small then the ETH_HLEN
> subtraction will result in a negative which is then type promoted
> to an unsigned int and the condition will be true.
> 
> Generally, when you have an untrusted variable like skb->len, you
> should move all the math to the other side of the comparison.
> 
> Fixes: 15865124feed ("staging: r8188eu: introduce new core dir for RTL8188eu driver")
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
> Compile tested only.  This is basic algebra of moving parts of the
> equation from one side to the other and I am surprisingly bad at
> something that I was supposed to have learned in 9th grade.
> 
>   drivers/staging/r8188eu/core/rtw_br_ext.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c
> index a7c67014dde0..f49e32c33372 100644
> --- a/drivers/staging/r8188eu/core/rtw_br_ext.c
> +++ b/drivers/staging/r8188eu/core/rtw_br_ext.c
> @@ -538,7 +538,7 @@ int nat25_db_handle(struct adapter *priv, struct sk_buff *skb, int method)
>   		/*------------------------------------------------*/
>   		struct ipv6hdr *iph = (struct ipv6hdr *)(skb->data + ETH_HLEN);
>   
> -		if (sizeof(*iph) >= (skb->len - ETH_HLEN))
> +		if (skb->len <= sizeof(*iph) + ETH_HLEN)
>   			return -1;
>   
>   		switch (method) {

Tested-by: Philipp Hortmann <philipp.g.hortmann@gmail.com> # Edimax N150

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] staging: r8188eu: fix a potential integer underflow bug
  2023-02-22 13:59 [PATCH] staging: r8188eu: fix a potential integer underflow bug Dan Carpenter
  2023-02-23  7:00 ` Philipp Hortmann
@ 2023-02-23 11:00 ` Pavel Skripkin
  2023-02-23 13:58   ` Dan Carpenter
  2023-02-23 11:26 ` Dan Carpenter
  2023-03-09  9:09 ` Greg Kroah-Hartman
  3 siblings, 1 reply; 6+ messages in thread
From: Pavel Skripkin @ 2023-02-23 11:00 UTC (permalink / raw)
  To: Dan Carpenter, Phillip Potter
  Cc: Greg Kroah-Hartman, Deepak R Varma, Charlie Sands, Mahak Gupta,
	Alaa Mohamed, linux-staging, kernel-janitors

[-- Attachment #1: Type: text/plain, Size: 1676 bytes --]

Hi Dan,

Dan Carpenter <error27@gmail.com> says:
> Here the code is testing to see if skb->len meets a minimum size
> requirement.  However if skb->len is very small then the ETH_HLEN
> subtraction will result in a negative which is then type promoted
> to an unsigned int and the condition will be true.
> 
> Generally, when you have an untrusted variable like skb->len, you
> should move all the math to the other side of the comparison.
> 
> Fixes: 15865124feed ("staging: r8188eu: introduce new core dir for RTL8188eu driver")
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
> Compile tested only.  This is basic algebra of moving parts of the
> equation from one side to the other and I am surprisingly bad at
> something that I was supposed to have learned in 9th grade.
> 
>   drivers/staging/r8188eu/core/rtw_br_ext.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c
> index a7c67014dde0..f49e32c33372 100644
> --- a/drivers/staging/r8188eu/core/rtw_br_ext.c
> +++ b/drivers/staging/r8188eu/core/rtw_br_ext.c
> @@ -538,7 +538,7 @@ int nat25_db_handle(struct adapter *priv, struct sk_buff *skb, int method)
>   		/*------------------------------------------------*/
>   		struct ipv6hdr *iph = (struct ipv6hdr *)(skb->data + ETH_HLEN);
>   
> -		if (sizeof(*iph) >= (skb->len - ETH_HLEN))
> +		if (skb->len <= sizeof(*iph) + ETH_HLEN)
>   			return -1;


Thanks for the patch!

I am wondering, if it make sense to use generic skb APIs which will do 
error handling for us?

Like following (not even build-tested tho)



With regards,
Pavel Skripkin

[-- Attachment #2: ph1 --]
[-- Type: text/plain, Size: 1820 bytes --]

diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c
index a7c67014dde0..8f5f2ef26056 100644
--- a/drivers/staging/r8188eu/core/rtw_br_ext.c
+++ b/drivers/staging/r8188eu/core/rtw_br_ext.c
@@ -536,26 +536,29 @@ int nat25_db_handle(struct adapter *priv, struct sk_buff *skb, int method)
 		/*------------------------------------------------*/
 		/*         Handle IPV6 frame			  */
 		/*------------------------------------------------*/
-		struct ipv6hdr *iph = (struct ipv6hdr *)(skb->data + ETH_HLEN);
+		u8 header *h = skb->data;
+		struct ipv6hdr *iph = skb_pull(skb, ETH_HLEN);
 
-		if (sizeof(*iph) >= (skb->len - ETH_HLEN))
+		if (!iph)
 			return -1;
 
 		switch (method) {
 		case NAT25_CHECK:
-			if (skb->data[0] & 1)
+			if (h[0] & 1)
 				return 0;
 			return -1;
 		case NAT25_INSERT:
 			if (memcmp(&iph->saddr, "\x0\x0\x0\x0\x0\x0\x0\x0\x0\x0\x0\x0\x0\x0\x0\x0", 16)) {
 				__nat25_generate_ipv6_network_addr(addr, (unsigned int *)&iph->saddr);
-				__nat25_db_network_insert(priv, skb->data + ETH_ALEN, addr);
+				__nat25_db_network_insert(priv, (void *)iph, addr);
+
+				if (iph->nexthdr == IPPROTO_ICMPV6) {
+					struct ipv6hdr *hdr = skb_pull(skb, sizeof(*iph));
+
+					if (!iph)
+						return 0;
 
-				if (iph->nexthdr == IPPROTO_ICMPV6 &&
-						skb->len > (ETH_HLEN +  sizeof(*iph) + 4)) {
-					if (update_nd_link_layer_addr(skb->data + ETH_HLEN + sizeof(*iph),
-								      skb->len - ETH_HLEN - sizeof(*iph), GET_MY_HWADDR(priv))) {
-						struct icmp6hdr  *hdr = (struct icmp6hdr *)(skb->data + ETH_HLEN + sizeof(*iph));
+					if (update_nd_link_layer_addr(hdr, skb_len(skb), GET_MY_HWADDR(priv))) {
 						hdr->icmp6_cksum = 0;
 						hdr->icmp6_cksum = csum_ipv6_magic(&iph->saddr, &iph->daddr,
 										be16_to_cpu(iph->payload_len),

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] staging: r8188eu: fix a potential integer underflow bug
  2023-02-23 11:00 ` Pavel Skripkin
@ 2023-02-23 13:58   ` Dan Carpenter
  0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2023-02-23 13:58 UTC (permalink / raw)
  To: Pavel Skripkin
  Cc: Phillip Potter, Greg Kroah-Hartman, Deepak R Varma, Charlie Sands,
	Mahak Gupta, Alaa Mohamed, linux-staging, kernel-janitors

On Thu, Feb 23, 2023 at 02:00:48PM +0300, Pavel Skripkin wrote:
> > diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c
> > index a7c67014dde0..f49e32c33372 100644
> > --- a/drivers/staging/r8188eu/core/rtw_br_ext.c
> > +++ b/drivers/staging/r8188eu/core/rtw_br_ext.c
> > @@ -538,7 +538,7 @@ int nat25_db_handle(struct adapter *priv, struct sk_buff *skb, int method)
> >   		/*------------------------------------------------*/
> >   		struct ipv6hdr *iph = (struct ipv6hdr *)(skb->data + ETH_HLEN);
> > -		if (sizeof(*iph) >= (skb->len - ETH_HLEN))
> > +		if (skb->len <= sizeof(*iph) + ETH_HLEN)
> >   			return -1;
> 
> 
> Thanks for the patch!
> 
> I am wondering, if it make sense to use generic skb APIs which will do error
> handling for us?
> 
> Like following (not even build-tested tho)
> 
> 
> 
> With regards,
> Pavel Skripkin
> diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c
> index a7c67014dde0..8f5f2ef26056 100644
> --- a/drivers/staging/r8188eu/core/rtw_br_ext.c
> +++ b/drivers/staging/r8188eu/core/rtw_br_ext.c
> @@ -536,26 +536,29 @@ int nat25_db_handle(struct adapter *priv, struct sk_buff *skb, int method)
>  		/*------------------------------------------------*/
>  		/*         Handle IPV6 frame			  */
>  		/*------------------------------------------------*/
> -		struct ipv6hdr *iph = (struct ipv6hdr *)(skb->data + ETH_HLEN);
> +		u8 header *h = skb->data;
> +		struct ipv6hdr *iph = skb_pull(skb, ETH_HLEN);
>  
> -		if (sizeof(*iph) >= (skb->len - ETH_HLEN))
> +		if (!iph)
>  			return -1;
>  
>  		switch (method) {
>  		case NAT25_CHECK:
> -			if (skb->data[0] & 1)
> +			if (h[0] & 1)
>  				return 0;
>  			return -1;
>  		case NAT25_INSERT:
>  			if (memcmp(&iph->saddr, "\x0\x0\x0\x0\x0\x0\x0\x0\x0\x0\x0\x0\x0\x0\x0\x0", 16)) {
>  				__nat25_generate_ipv6_network_addr(addr, (unsigned int *)&iph->saddr);
> -				__nat25_db_network_insert(priv, skb->data + ETH_ALEN, addr);
> +				__nat25_db_network_insert(priv, (void *)iph, addr);

Doing it this way introduces a read overflow because there is no
guarantee that iph is large enough.  You would still need a check for
if (skb->len < sizeof(*iph)).  The existing check is <= instead of <,
but that was in the original code.  I should have investigated why it is
<= and if it should be change to <.  Sorry, this patch was not good.

A better way to do this would be to use skb_pull_data()...  But it's
still buggy because it trusts be16_to_cpu(iph->payload_len).  I need to
zoom out on this one.  Is this NAT25 stuff even required?  Do other
people do NAT stuff in their wifi drivers?

regards,
dan carpenter

diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c
index f49e32c33372..b84b2d4f23c3 100644
--- a/drivers/staging/r8188eu/core/rtw_br_ext.c
+++ b/drivers/staging/r8188eu/core/rtw_br_ext.c
@@ -536,27 +536,26 @@ int nat25_db_handle(struct adapter *priv, struct sk_buff *skb, int method)
 		/*------------------------------------------------*/
 		/*         Handle IPV6 frame			  */
 		/*------------------------------------------------*/
-		struct ipv6hdr *iph = (struct ipv6hdr *)(skb->data + ETH_HLEN);
+		u8 *header = skb_pull_data(skb, ETH_HLEN);
+		struct ipv6hdr *iph = skb_pull_data(skb, sizeof(*iph));
 
-		if (skb->len <= sizeof(*iph) + ETH_HLEN)
+		if (!iph)
 			return -1;
 
 		switch (method) {
 		case NAT25_CHECK:
-			if (skb->data[0] & 1)
+			if (header[0] & 1)
 				return 0;
 			return -1;
 		case NAT25_INSERT:
 			if (memcmp(&iph->saddr, "\x0\x0\x0\x0\x0\x0\x0\x0\x0\x0\x0\x0\x0\x0\x0\x0", 16)) {
 				__nat25_generate_ipv6_network_addr(addr, (unsigned int *)&iph->saddr);
-				__nat25_db_network_insert(priv, skb->data + ETH_ALEN, addr);
+				__nat25_db_network_insert(priv, iph, addr);
 
-				if (iph->nexthdr == IPPROTO_ICMPV6 &&
-						skb->len > (ETH_HLEN +  sizeof(*iph) + 4)) {
-					if (update_nd_link_layer_addr(skb->data + ETH_HLEN + sizeof(*iph),
-								      skb->len - ETH_HLEN - sizeof(*iph), GET_MY_HWADDR(priv))) {
-						struct icmp6hdr  *hdr = (struct icmp6hdr *)(skb->data + ETH_HLEN + sizeof(*iph));
-						hdr->icmp6_cksum = 0;
+				if (iph->nexthdr == IPPROTO_ICMPV6 && skb->len > 4) {
+					if (update_nd_link_layer_addr(skb->data, skb->len,
+								      GET_MY_HWADDR(priv))) {
+						struct icmp6hdr  *hdr = (struct icmp6hdr *)skb->data;
 						hdr->icmp6_cksum = csum_ipv6_magic(&iph->saddr, &iph->daddr,
 										be16_to_cpu(iph->payload_len),
 										IPPROTO_ICMPV6,

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] staging: r8188eu: fix a potential integer underflow bug
  2023-02-22 13:59 [PATCH] staging: r8188eu: fix a potential integer underflow bug Dan Carpenter
  2023-02-23  7:00 ` Philipp Hortmann
  2023-02-23 11:00 ` Pavel Skripkin
@ 2023-02-23 11:26 ` Dan Carpenter
  2023-03-09  9:09 ` Greg Kroah-Hartman
  3 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2023-02-23 11:26 UTC (permalink / raw)
  To: Phillip Potter
  Cc: Pavel Skripkin, Greg Kroah-Hartman, Deepak R Varma, Charlie Sands,
	Mahak Gupta, Alaa Mohamed, linux-staging, kernel-janitors

On Wed, Feb 22, 2023 at 04:59:41PM +0300, Dan Carpenter wrote:
> Here the code is testing to see if skb->len meets a minimum size
> requirement.  However if skb->len is very small then the ETH_HLEN
> subtraction will result in a negative which is then type promoted
> to an unsigned int and the condition will be true.
> 
> Generally, when you have an untrusted variable like skb->len, you
> should move all the math to the other side of the comparison.
> 
> Fixes: 15865124feed ("staging: r8188eu: introduce new core dir for RTL8188eu driver")
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
> Compile tested only.  This is basic algebra of moving parts of the
> equation from one side to the other and I am surprisingly bad at
> something that I was supposed to have learned in 9th grade.
> 
>  drivers/staging/r8188eu/core/rtw_br_ext.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c
> index a7c67014dde0..f49e32c33372 100644
> --- a/drivers/staging/r8188eu/core/rtw_br_ext.c
> +++ b/drivers/staging/r8188eu/core/rtw_br_ext.c
> @@ -538,7 +538,7 @@ int nat25_db_handle(struct adapter *priv, struct sk_buff *skb, int method)
>  		/*------------------------------------------------*/
>  		struct ipv6hdr *iph = (struct ipv6hdr *)(skb->data + ETH_HLEN);
>  
> -		if (sizeof(*iph) >= (skb->len - ETH_HLEN))
> +		if (skb->len <= sizeof(*iph) + ETH_HLEN)
>  			return -1;

NAK.  On reviewing now, if this is a bug, then there is already a read
overflow a few lines earlier.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] staging: r8188eu: fix a potential integer underflow bug
  2023-02-22 13:59 [PATCH] staging: r8188eu: fix a potential integer underflow bug Dan Carpenter
                   ` (2 preceding siblings ...)
  2023-02-23 11:26 ` Dan Carpenter
@ 2023-03-09  9:09 ` Greg Kroah-Hartman
  3 siblings, 0 replies; 6+ messages in thread
From: Greg Kroah-Hartman @ 2023-03-09  9:09 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Phillip Potter, Pavel Skripkin, Deepak R Varma, Charlie Sands,
	Mahak Gupta, Alaa Mohamed, linux-staging, kernel-janitors

On Wed, Feb 22, 2023 at 04:59:41PM +0300, Dan Carpenter wrote:
> Here the code is testing to see if skb->len meets a minimum size
> requirement.  However if skb->len is very small then the ETH_HLEN
> subtraction will result in a negative which is then type promoted
> to an unsigned int and the condition will be true.
> 
> Generally, when you have an untrusted variable like skb->len, you
> should move all the math to the other side of the comparison.
> 
> Fixes: 15865124feed ("staging: r8188eu: introduce new core dir for RTL8188eu driver")
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
> Compile tested only.  This is basic algebra of moving parts of the
> equation from one side to the other and I am surprisingly bad at
> something that I was supposed to have learned in 9th grade.
> 
>  drivers/staging/r8188eu/core/rtw_br_ext.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

This driver is now deleted, so no need to worry about this anymore.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-03-09  9:09 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-22 13:59 [PATCH] staging: r8188eu: fix a potential integer underflow bug Dan Carpenter
2023-02-23  7:00 ` Philipp Hortmann
2023-02-23 11:00 ` Pavel Skripkin
2023-02-23 13:58   ` Dan Carpenter
2023-02-23 11:26 ` Dan Carpenter
2023-03-09  9:09 ` Greg Kroah-Hartman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox