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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Dan,

Dan Carpenter <error27@xxxxxxxxx> 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@xxxxxxxxx>
---
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
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),

[Index of Archives]     [Kernel Development]     [Kernel Announce]     [Kernel Newbies]     [Linux Networking Development]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Device Mapper]

  Powered by Linux