Re: [PATCH] drivers/net/wan: lapb: Corrected the usage of skb_cow

[David Miller]

From: Xie He 
Date: Fri, 24 Jul 2020 09:33:47 -0700

> This patch fixed 2 issues with the usage of skb_cow in LAPB drivers
> "lapbether" and "hdlc_x25":
> 
> 1) After skb_cow fails, kfree_skb should be called to drop a reference
> to the skb. But in both drivers, kfree_skb is not called.
> 
> 2) skb_cow should be called before skb_push so that is can ensure the
> safety of skb_push. But in "lapbether", it is incorrectly called after
> skb_push.
> 
> More details about these 2 issues:
> 
> 1) The behavior of calling kfree_skb on failure is also the behavior of
> netif_rx, which is called by this function with "return netif_rx(skb);".
> So this function should follow this behavior, too.
> 
> 2) In "lapbether", skb_cow is called after skb_push. This results in 2
> logical issues:
>a) skb_push is not protected by skb_cow;
>b) An extra headroom of 1 byte is ensured after skb_push. This extra
>   headroom has no use in this function. It also has no use in the
>   upper-layer function that this function passes the skb to
>   (x25_lapb_receive_frame in net/x25/x25_dev.c).
> So logically skb_cow should instead be called before skb_push.
> 
> Cc: Eric Dumazet 
> Cc: Martin Schiller 
> Signed-off-by: Xie He 

Applied, thank you.

[PATCH] drivers/net/wan: lapb: Corrected the usage of skb_cow

[Xie He]

This patch fixed 2 issues with the usage of skb_cow in LAPB drivers
"lapbether" and "hdlc_x25":

1) After skb_cow fails, kfree_skb should be called to drop a reference
to the skb. But in both drivers, kfree_skb is not called.

2) skb_cow should be called before skb_push so that is can ensure the
safety of skb_push. But in "lapbether", it is incorrectly called after
skb_push.

More details about these 2 issues:

1) The behavior of calling kfree_skb on failure is also the behavior of
netif_rx, which is called by this function with "return netif_rx(skb);".
So this function should follow this behavior, too.

2) In "lapbether", skb_cow is called after skb_push. This results in 2
logical issues:
   a) skb_push is not protected by skb_cow;
   b) An extra headroom of 1 byte is ensured after skb_push. This extra
  headroom has no use in this function. It also has no use in the
  upper-layer function that this function passes the skb to
  (x25_lapb_receive_frame in net/x25/x25_dev.c).
So logically skb_cow should instead be called before skb_push.

Cc: Eric Dumazet 
Cc: Martin Schiller 
Signed-off-by: Xie He 
---
 drivers/net/wan/hdlc_x25.c  | 4 +++-
 drivers/net/wan/lapbether.c | 8 +---
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c
index c84536b03aa8..f70336bb6f52 100644
--- a/drivers/net/wan/hdlc_x25.c
+++ b/drivers/net/wan/hdlc_x25.c
@@ -71,8 +71,10 @@ static int x25_data_indication(struct net_device *dev, 
struct sk_buff *skb)
 {
unsigned char *ptr;
 
-   if (skb_cow(skb, 1))
+   if (skb_cow(skb, 1)) {
+   kfree_skb(skb);
return NET_RX_DROP;
+   }
 
skb_push(skb, 1);
skb_reset_network_header(skb);
diff --git a/drivers/net/wan/lapbether.c b/drivers/net/wan/lapbether.c
index 284832314f31..b2868433718f 100644
--- a/drivers/net/wan/lapbether.c
+++ b/drivers/net/wan/lapbether.c
@@ -128,10 +128,12 @@ static int lapbeth_data_indication(struct net_device 
*dev, struct sk_buff *skb)
 {
unsigned char *ptr;
 
-   skb_push(skb, 1);
-
-   if (skb_cow(skb, 1))
+   if (skb_cow(skb, 1)) {
+   kfree_skb(skb);
return NET_RX_DROP;
+   }
+
+   skb_push(skb, 1);
 
ptr  = skb->data;
*ptr = X25_IFACE_DATA;
-- 
2.25.1