[net] net: qualcomm: rmnet: Fix a double free
diff mbox series

Message ID 20170908102356.tzysh6qaiesd2umz@mwanda
State Changes Requested, archived
Delegated to: David Miller
Headers show
Series
  • [net] net: qualcomm: rmnet: Fix a double free
Related show

Commit Message

Dan Carpenter Sept. 8, 2017, 10:23 a.m. UTC
This is called from rmnet_map_ingress_handler().  When the
rmnet_map_deaggregate() returns NULL then the caller calls
consume_skb(skb) which frees the skb.  The kfree_skb() on this error
path leads to a double free.

Fixes: ceed73a2cf4a ("drivers: net: ethernet: qualcomm: rmnet: Initial implementation")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
This is from static analysis and not tested.

Comments

Subash Abhinov Kasiviswanathan Sept. 8, 2017, 7:44 p.m. UTC | #1
> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> index 557c9bf1a469..0335fce54201 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> @@ -95,10 +95,8 @@ struct sk_buff *rmnet_map_deaggregate(struct sk_buff 
> *skb)
>  	skb_pull(skb, packet_len);
> 
>  	/* Some hardware can send us empty frames. Catch them */
> -	if (ntohs(maph->pkt_len) == 0) {
> -		kfree_skb(skb);
> +	if (ntohs(maph->pkt_len) == 0)
>  		return NULL;
> -	}
> 
>  	return skbn;
>  }

Thanks for the patch. This is fixing the double free, but is leaking the 
new skb skbn
created. Perhaps we should add the check earlier -

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c 
b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
index 557c9bf..86b8c75 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -84,6 +84,10 @@ struct sk_buff *rmnet_map_deaggregate(struct sk_buff 
*skb)
         if (((int)skb->len - (int)packet_len) < 0)
                 return NULL;

+       /* Some hardware can send us empty frames. Catch them */
+       if (ntohs(maph->pkt_len) == 0)
+               return NULL;
+
         skbn = alloc_skb(packet_len + RMNET_MAP_DEAGGR_SPACING, 
GFP_ATOMIC);
         if (!skbn)
                 return NULL;
@@ -94,11 +98,5 @@ struct sk_buff *rmnet_map_deaggregate(struct sk_buff 
*skb)
         memcpy(skbn->data, skb->data, packet_len);
         skb_pull(skb, packet_len);

-       /* Some hardware can send us empty frames. Catch them */
-       if (ntohs(maph->pkt_len) == 0) {
-               kfree_skb(skb);
-               return NULL;
-       }
-
         return skbn;
  }

Patch
diff mbox series

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
index 557c9bf1a469..0335fce54201 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -95,10 +95,8 @@  struct sk_buff *rmnet_map_deaggregate(struct sk_buff *skb)
 	skb_pull(skb, packet_len);
 
 	/* Some hardware can send us empty frames. Catch them */
-	if (ntohs(maph->pkt_len) == 0) {
-		kfree_skb(skb);
+	if (ntohs(maph->pkt_len) == 0)
 		return NULL;
-	}
 
 	return skbn;
 }