AW: [PATCH] rtlwifi: Fix a double free in _rtl_usb_tx_urb_setup()

Commit Message

Walter Harms May 13, 2020, 1:38 p.m. UTC

IMHO _rtl_usb_transmit() should not free() either
it should return -1.
The only caller is rtl_usb_tx() where we need a check:

if ( _rtl_usb_transmit()  < 0)
  goto err_free;

but i am confused, rtl_usb_tx() is returning NETDEV_TX_OK in an error case ?

err_free:
     dev_kfree_skb_any(skb);
      return NETDEV_TX_OK;

hope that helps,
  wh

Comments

Dan Carpenter May 13, 2020, 1:44 p.m. UTC | #1

On Wed, May 13, 2020 at 01:38:09PM +0000, Walter Harms wrote:
> IMHO _rtl_usb_transmit() should not free() either
> it should return -1.
> The only caller is rtl_usb_tx() where we need a check:
> 
> if ( _rtl_usb_transmit()  < 0)
>   goto err_free;
> 
> but i am confused, rtl_usb_tx() is returning NETDEV_TX_OK in an error case ?
> 
> err_free:
>      dev_kfree_skb_any(skb);
>       return NETDEV_TX_OK;

This is a pretty typical pattern in networking.  For convenience we are
pretending that the transmit always succeeds and that the packet was
lost somewhere in the network.  The TCP layer will ask for a resend.

regards,
dan carpenter

Patch

diff --git a/drivers/net/wireless/realtek/rtlwifi/usb.c b/drivers/net/wireless/realtek/rtlwifi/usb.c
index 348b0072cdd69..c66c6dc003783 100644
--- a/drivers/net/wireless/realtek/rtlwifi/usb.c
+++ b/drivers/net/wireless/realtek/rtlwifi/usb.c
@@ -881,10 +881,8 @@  static struct urb *_rtl_usb_tx_urb_setup(struct ieee80211_hw *hw,

        WARN_ON(NULL == skb);
        _urb = usb_alloc_urb(0, GFP_ATOMIC);
-       if (!_urb) {
-               kfree_skb(skb);
+       if (!_urb)
                return NULL;
-       }
        _rtl_install_trx_info(rtlusb, skb, ep_num);
        usb_fill_bulk_urb(_urb, rtlusb->udev, usb_sndbulkpipe(rtlusb->udev,
                          ep_num), skb->data, skb->len, _rtl_tx_complete, skb);
@@ -898,7 +896,6 @@  static void _rtl_usb_transmit(struct ieee80211_hw *hw, struct sk_buff *skb,
        struct rtl_usb *rtlusb = rtl_usbdev(rtl_usbpriv(hw));
        u32 ep_num;
        struct urb *_urb = NULL;
-       struct sk_buff *_skb = NULL;

        WARN_ON(NULL == rtlusb->usb_tx_aggregate_hdl);
        if (unlikely(IS_USB_STOP(rtlusb))) {
@@ -907,8 +904,7 @@  static void _rtl_usb_transmit(struct ieee80211_hw *hw, struct sk_buff *skb,
                return;
        }
        ep_num = rtlusb->ep_map.ep_mapping[qnum];
-       _skb = skb;
-       _urb = _rtl_usb_tx_urb_setup(hw, _skb, ep_num);
+       _urb = _rtl_usb_tx_urb_setup(hw, skb, ep_num);
        if (unlikely(!_urb)) {
                pr_err("Can't allocate urb. Drop skb!\n");
                kfree_skb(skb);