Bug 188971

Summary: Function irda_usb_probe() does not set error code when the calls to kzalloc() fails
Product: Drivers Reporter: bianpan (bianpan2010)
Component: NetworkAssignee: drivers_network (drivers_network)
Status: RESOLVED CODE_FIX    
Severity: normal    
Priority: P1    
Hardware: All   
OS: Linux   
Kernel Version: linux-4.9-rc6 Subsystem:
Regression: No Bisected commit-id:
Attachments: The patch fixes the bug

Description bianpan 2016-11-25 11:13:16 UTC
Function kzalloc() returns a NULL pointer if there is no enough memory. In function irda_usb_probe() defined in file drivers/net/irda/irda-usb.c, if the return values of the calls to kzalloc() (at lines 1726 and 1730) are NULL, the control flow jumps to labels "err_out_3" and "error_out_4", cleans allocated memory and returns variable ret. Because variable ret is check at line 1709, the value of variable ret may be 0. As a result, 0 (indicates success) may be returned even if the calls to kzalloc() fail. I think it is better to assign "-ENOMEM" to ret before the call to kzalloc(). Codes and comments related to this bug are summarised as follows.

irda_usb_probe @@ drivers/net/irda/irda-usb.c
1597 static int irda_usb_probe(struct usb_interface *intf,
1598               const struct usb_device_id *id)
1599 {
         ...
1605     int ret = -ENOMEM;
         ...
1706     if (self->needspatch) {
1707         ret = usb_control_msg (self->usbdev, usb_sndctrlpipe (self->usbdev, 0),
1708                        0x02, 0x40, 0, 0, NULL, 0, 500);
1709         if (ret < 0) {
1710             pr_debug("usb_control_msg failed %d\n", ret);
1711             goto err_out_3;
1712         } else {
1713             mdelay(10);
1714         }
1715     }
1716 
1717     self->irda_desc =  irda_desc;
1718     self->present = 1;
1719     self->netopen = 0;
1720     self->usbintf = intf;
1721 
1722     /* Allocate the buffer for speed changes */
1723     /* Don't change this buffer size and allocation without doing
1724      * some heavy and complete testing. Don't ask why :-(
1725      * Jean II */
         // ret may takes value 0. Insert "ret = -ENOMEM;" here?
1726     self->speed_buff = kzalloc(IRDA_USB_SPEED_MTU, GFP_KERNEL);
1727     if (!self->speed_buff)
1728         goto err_out_3;
1729 
1730     self->tx_buff = kzalloc(IRDA_SKB_MAX_MTU + self->header_length,
1731                 GFP_KERNEL);
1732     if (!self->tx_buff)
1733         goto err_out_4;
         ...
1762     return 0;
1763 err_out_6:
1764     unregister_netdev(self->netdev);
1765 err_out_5:
1766     kfree(self->tx_buff);
1767 err_out_4:
1768     kfree(self->speed_buff);
1769 err_out_3:
1770     /* Free all urbs that we may have created */
1771     usb_free_urb(self->speed_urb);
1772 err_out_2:
1773     usb_free_urb(self->tx_urb);
1774 err_out_1:
1775     for (i = 0; i < self->max_rx_urb; i++)
1776         usb_free_urb(self->rx_urb[i]);
1777     kfree(self->rx_urb);
1778 err_free_net:
1779     free_netdev(net);
1780 err_out:
1781     return ret;
1782 }

Thanks very much!
Comment 1 bianpan 2017-05-12 00:21:18 UTC
Created attachment 256443 [details]
The patch fixes the bug

The patch has been merged into the latest version of the Linux kernel. So I will close the bug.