Skip to contentSkip to conversation

rtlwifi: Fix a double free in _rtl_usb_tx_urb_setup()

PATCH In review Dan Carpenter 3 participants 4 messages May 13, 2020 – May 18, 2020

Subsystems (2)
Kernel janitors Realtek wireless driver (rtlwifi family)
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 rtlwifi: Fix a double free in _rtl_usb_tx_urb_setup()

Seven years ago we tried to fix a leak but actually introduced a double free instead. It was an understandable mistake because the code was a bit confusing and the free was done in the wrong place. The "skb" pointer is freed in both _rtl_usb_tx_urb_setup() and _rtl_usb_transmit(). The free belongs _rtl_usb_transmit() instead of _rtl_usb_tx_urb_setup() and I've cleaned the code up a bit to hopefully make it more clear.

Fixes 36ef0b473fbf ("rtlwifi: usb: add missing freeing of skbuff")
Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/net/wireless/realtek/rtlwifi/usb.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)
drivers/net/wireless/realtek/rtlwifi/usb.c +2 -6
index 348b0072cdd69..c66c6dc003783 100644
@@ -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);
Signature · 1 line
2.26.2
#1 lore raw

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
________________________________________
Von: [email protected] <[email protected]> im Auftrag von Dan Carpenter <[email protected]>
Gesendet: Mittwoch, 13. Mai 2020 11:39:51
An: Ping-Ke Shih; Jussi Kivilinna
Cc: Kalle Valo; [email protected]; [email protected]
Betreff: [PATCH] rtlwifi: Fix a double free in _rtl_usb_tx_urb_setup()

Seven years ago we tried to fix a leak but actually introduced a double free instead. It was an understandable mistake because the code was a bit confusing and the free was done in the wrong place. The "skb" pointer is freed in both _rtl_usb_tx_urb_setup() and _rtl_usb_transmit(). The free belongs _rtl_usb_transmit() instead of _rtl_usb_tx_urb_setup() and I've cleaned the code up a bit to hopefully make it more clear.

Fixes 36ef0b473fbf ("rtlwifi: usb: add missing freeing of skbuff")
Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/net/wireless/realtek/rtlwifi/usb.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)
drivers/net/wireless/realtek/rtlwifi/usb.c +2 -6
index 348b0072cdd69..c66c6dc003783 100644
@@ -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);

--
2.26.2

author
#2 lore raw
Quoted 4 lines · Walter Harms

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

Quoted 8 lines

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

#3 lore raw
In reply to Dan Carpenter
Quoted 9 lines · Dan Carpenter

Seven years ago we tried to fix a leak but actually introduced a double free instead. It was an understandable mistake because the code was a bit confusing and the free was done in the wrong place. The "skb" pointer is freed in both _rtl_usb_tx_urb_setup() and _rtl_usb_transmit(). The free belongs _rtl_usb_transmit() instead of _rtl_usb_tx_urb_setup() and I've cleaned the code up a bit to hopefully make it more clear.

Fixes 36ef0b473fbf ("rtlwifi: usb: add missing freeing of skbuff")
Signed-off-by Dan Carpenter <[email protected]>

Patch applied to wireless-drivers-next.git, thanks.

beb12813bc75 rtlwifi: Fix a double free in _rtl_usb_tx_urb_setup()

Signature · 3 lines
https://patchwork.kernel.org/patch/11545535/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches