netdev - Re: [bug report] net: dsa: ksz: fix skb freeing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 16 Aug 2017 23:52:53 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     vivien.didelot@...oirfairelinux.com, netdev@...r.kernel.org
Subject: Re: [bug report] net: dsa: ksz: fix skb freeing

On Thu, Aug 17, 2017 at 12:27:19AM +0300, Dan Carpenter wrote:
> Hello Vivien Didelot,
> 
> The patch e71cb9e00922: "net: dsa: ksz: fix skb freeing" from Aug 9,
> 2017, leads to the following static checker warning:
> 
> 	net/dsa/tag_ksz.c:64 ksz_xmit()
> 	warn: 'nskb' was already freed.

> net/dsa/tag_ksz.c
>     35  static struct sk_buff *ksz_xmit(struct sk_buff *skb, struct net_device *dev)
>     36  {
>     37          struct dsa_slave_priv *p = netdev_priv(dev);
>     38          struct sk_buff *nskb;
>     39          int padlen;
>     40          u8 *tag;
>     41  
>     42          padlen = (skb->len >= ETH_ZLEN) ? 0 : ETH_ZLEN - skb->len;
>     43  
>     44          if (skb_tailroom(skb) >= padlen + KSZ_INGRESS_TAG_LEN) {
>     45                  if (skb_put_padto(skb, skb->len + padlen))
>     46                          return NULL;
>     47  
>     48                  nskb = skb;
>     49          } else {
>     50                  nskb = alloc_skb(NET_IP_ALIGN + skb->len +
>     51                                   padlen + KSZ_INGRESS_TAG_LEN, GFP_ATOMIC);
>     52                  if (!nskb)
>     53                          return NULL;
>     54                  skb_reserve(nskb, NET_IP_ALIGN);
>     55  
>     56                  skb_reset_mac_header(nskb);
>     57                  skb_set_network_header(nskb,
>     58                                         skb_network_header(skb) - skb->head);
>     59                  skb_set_transport_header(nskb,
>     60                                           skb_transport_header(skb) - skb->head);
>     61                  skb_copy_and_csum_dev(skb, skb_put(nskb, skb->len));
>     62  
>     63                  if (skb_put_padto(nskb, nskb->len + padlen)) {
>     64                          kfree_skb(nskb);
>                                 ^^^^^^^^^^^^^^
> This patch deliberately adds a kfree_skb() here, but it looks like a
> double free, and my static checker complains.  My guess is you had
> intended to free "skb" instead of "nskb"?  I'm not positive.

I actually think it is a miss understanding about skb_put_padto()
actually does. If it returns an error code, it also frees the skb it
has been asked to modify. So free'ing nsbk is wrong. So line 64 simply
needs to be removed.

      Andrew