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

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

 



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,



[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