2014-06-26 22:52:12

by Alexey Khoroshilov

[permalink] [raw]
Subject: [PATCH 1/2] rsi: GFP_ATOMIC is not needed in rsi_init_usb_interface()

Signed-off-by: Alexey Khoroshilov <[email protected]>
---
drivers/net/wireless/rsi/rsi_91x_usb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/rsi/rsi_91x_usb.c b/drivers/net/wireless/rsi/rsi_91x_usb.c
index 4c46e5631e2f..2e0254606e3d 100644
--- a/drivers/net/wireless/rsi/rsi_91x_usb.c
+++ b/drivers/net/wireless/rsi/rsi_91x_usb.c
@@ -397,7 +397,7 @@ static int rsi_init_usb_interface(struct rsi_hw *adapter,
return -ENOMEM;
}

- rsi_dev->tx_buffer = kmalloc(2048, GFP_ATOMIC);
+ rsi_dev->tx_buffer = kmalloc(2048, GFP_KERNEL);
rsi_dev->rx_usb_urb[0] = usb_alloc_urb(0, GFP_KERNEL);
rsi_dev->rx_usb_urb[0]->transfer_buffer = adapter->priv->rx_data_pkt;
rsi_dev->tx_blk_size = 252;
--
1.9.1


2014-06-26 22:51:40

by Alexey Khoroshilov

[permalink] [raw]
Subject: [PATCH 2/2] rsi: fix memory leaks and error handling in rsi_91x_usb

The patch fixes a couple of issues:
- absence of deallocation of rsi_dev->rx_usb_urb[0] in the driver;
- potential NULL pointer dereference because of lack of checks for memory
allocation success in rsi_init_usb_interface().

By the way, it makes rsi_probe() returning error code instead of 1
and fixes comments regarding returning values.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Alexey Khoroshilov <[email protected]>
---
drivers/net/wireless/rsi/rsi_91x_usb.c | 58 ++++++++++++++++++++++------------
1 file changed, 38 insertions(+), 20 deletions(-)

diff --git a/drivers/net/wireless/rsi/rsi_91x_usb.c b/drivers/net/wireless/rsi/rsi_91x_usb.c
index 2e0254606e3d..de4d100b84dd 100644
--- a/drivers/net/wireless/rsi/rsi_91x_usb.c
+++ b/drivers/net/wireless/rsi/rsi_91x_usb.c
@@ -25,7 +25,7 @@
* @len: Length to be written.
* @endpoint: Type of endpoint.
*
- * Return: status: 0 on success, -1 on failure.
+ * Return: status: 0 on success, a negative error code on failure.
*/
static int rsi_usb_card_write(struct rsi_hw *adapter,
void *buf,
@@ -60,7 +60,7 @@ static int rsi_usb_card_write(struct rsi_hw *adapter,
* @data: Pointer to the data that has to be written.
* @count: Number of multiple bytes to be written.
*
- * Return: 0 on success, -1 on failure.
+ * Return: 0 on success, a negative error code on failure.
*/
static int rsi_write_multiple(struct rsi_hw *adapter,
u8 endpoint,
@@ -147,7 +147,7 @@ static int rsi_find_bulk_in_and_out_endpoints(struct usb_interface *interface,
* @value: Value to be read.
* @len: length of data to be read.
*
- * Return: status: 0 on success, -1 on failure.
+ * Return: status: 0 on success, a negative error code on failure.
*/
static int rsi_usb_reg_read(struct usb_device *usbdev,
u32 reg,
@@ -189,7 +189,7 @@ static int rsi_usb_reg_read(struct usb_device *usbdev,
* @value: Value to write.
* @len: Length of data to be written.
*
- * Return: status: 0 on success, -1 on failure.
+ * Return: status: 0 on success, a negative error code on failure.
*/
static int rsi_usb_reg_write(struct usb_device *usbdev,
u32 reg,
@@ -249,7 +249,7 @@ static void rsi_rx_done_handler(struct urb *urb)
* rsi_rx_urb_submit() - This function submits the given URB to the USB stack.
* @adapter: Pointer to the adapter structure.
*
- * Return: 0 on success, -1 on failure.
+ * Return: 0 on success, a negative error code on failure.
*/
static int rsi_rx_urb_submit(struct rsi_hw *adapter)
{
@@ -281,7 +281,7 @@ static int rsi_rx_urb_submit(struct rsi_hw *adapter)
* @data: Pointer to the data that has to be written.
* @count: Number of multiple bytes to be written on to the registers.
*
- * Return: status: 0 on success, -1 on failure.
+ * Return: status: 0 on success, a negative error code on failure.
*/
int rsi_usb_write_register_multiple(struct rsi_hw *adapter,
u32 addr,
@@ -331,7 +331,7 @@ int rsi_usb_write_register_multiple(struct rsi_hw *adapter,
* @pkt: Pointer to the data to be written on to the card.
* @len: Length of the data to be written on to the card.
*
- * Return: 0 on success, -1 on failure.
+ * Return: 0 on success, a negative error code on failure.
*/
static int rsi_usb_host_intf_write_pkt(struct rsi_hw *adapter,
u8 *pkt,
@@ -359,6 +359,7 @@ static void rsi_deinit_usb_interface(struct rsi_hw *adapter)
struct rsi_91x_usbdev *dev = (struct rsi_91x_usbdev *)adapter->rsi_dev;

rsi_kill_thread(&dev->rx_thread);
+ usb_free_urb(dev->rx_usb_urb[0]);
kfree(adapter->priv->rx_data_pkt);
kfree(dev->tx_buffer);
}
@@ -368,7 +369,7 @@ static void rsi_deinit_usb_interface(struct rsi_hw *adapter)
* @adapter: Pointer to the adapter structure.
* @pfunction: Pointer to USB interface structure.
*
- * Return: 0 on success, -1 on failure.
+ * Return: 0 on success, a negative error code on failure.
*/
static int rsi_init_usb_interface(struct rsi_hw *adapter,
struct usb_interface *pfunction)
@@ -398,7 +399,15 @@ static int rsi_init_usb_interface(struct rsi_hw *adapter,
}

rsi_dev->tx_buffer = kmalloc(2048, GFP_KERNEL);
+ if (!rsi_dev->tx_buffer) {
+ status = -ENOMEM;
+ goto fail_tx;
+ }
rsi_dev->rx_usb_urb[0] = usb_alloc_urb(0, GFP_KERNEL);
+ if (!rsi_dev->rx_usb_urb[0]) {
+ status = -ENOMEM;
+ goto fail_rx;
+ }
rsi_dev->rx_usb_urb[0]->transfer_buffer = adapter->priv->rx_data_pkt;
rsi_dev->tx_blk_size = 252;

@@ -413,7 +422,7 @@ static int rsi_init_usb_interface(struct rsi_hw *adapter,
rsi_usb_rx_thread, "RX-Thread");
if (status) {
rsi_dbg(ERR_ZONE, "%s: Unable to init rx thrd\n", __func__);
- goto fail;
+ goto fail_thread;
}

#ifdef CONFIG_RSI_DEBUGFS
@@ -424,8 +433,11 @@ static int rsi_init_usb_interface(struct rsi_hw *adapter,
rsi_dbg(INIT_ZONE, "%s: Enabled the interface\n", __func__);
return 0;

-fail:
+fail_thread:
+ usb_free_urb(rsi_dev->rx_usb_urb[0]);
+fail_rx:
kfree(rsi_dev->tx_buffer);
+fail_tx:
kfree(common->rx_data_pkt);
return status;
}
@@ -437,7 +449,7 @@ fail:
* @pfunction: Pointer to the USB interface structure.
* @id: Pointer to the usb_device_id structure.
*
- * Return: 0 on success, -1 on failure.
+ * Return: 0 on success, a negative error code on failure.
*/
static int rsi_probe(struct usb_interface *pfunction,
const struct usb_device_id *id)
@@ -445,6 +457,7 @@ static int rsi_probe(struct usb_interface *pfunction,
struct rsi_hw *adapter;
struct rsi_91x_usbdev *dev;
u16 fw_status;
+ int status;

rsi_dbg(INIT_ZONE, "%s: Init function called\n", __func__);

@@ -452,10 +465,11 @@ static int rsi_probe(struct usb_interface *pfunction,
if (!adapter) {
rsi_dbg(ERR_ZONE, "%s: Failed to init os intf ops\n",
__func__);
- return 1;
+ return -ENOMEM;
}

- if (rsi_init_usb_interface(adapter, pfunction)) {
+ status = rsi_init_usb_interface(adapter, pfunction);
+ if (status) {
rsi_dbg(ERR_ZONE, "%s: Failed to init usb interface\n",
__func__);
goto err;
@@ -465,26 +479,30 @@ static int rsi_probe(struct usb_interface *pfunction,

dev = (struct rsi_91x_usbdev *)adapter->rsi_dev;

- if (rsi_usb_reg_read(dev->usbdev, FW_STATUS_REG, &fw_status, 2) < 0)
+ status = rsi_usb_reg_read(dev->usbdev, FW_STATUS_REG, &fw_status, 2);
+ if (status)
goto err1;
else
fw_status &= 1;

if (!fw_status) {
- if (rsi_usb_device_init(adapter->priv)) {
+ status = rsi_usb_device_init(adapter->priv);
+ if (status) {
rsi_dbg(ERR_ZONE, "%s: Failed in device init\n",
__func__);
goto err1;
}

- if (rsi_usb_reg_write(dev->usbdev,
- USB_INTERNAL_REG_1,
- RSI_USB_READY_MAGIC_NUM, 1) < 0)
+ status = rsi_usb_reg_write(dev->usbdev,
+ USB_INTERNAL_REG_1,
+ RSI_USB_READY_MAGIC_NUM, 1);
+ if (status)
goto err1;
rsi_dbg(INIT_ZONE, "%s: Performed device init\n", __func__);
}

- if (rsi_rx_urb_submit(adapter))
+ status = rsi_rx_urb_submit(adapter);
+ if (status)
goto err1;

return 0;
@@ -493,7 +511,7 @@ err1:
err:
rsi_91x_deinit(adapter);
rsi_dbg(ERR_ZONE, "%s: Failed in probe...Exiting\n", __func__);
- return 1;
+ return status;
}

/**
--
1.9.1

2014-06-27 10:18:20

by Jonas Gorski

[permalink] [raw]
Subject: Re: [PATCH 2/2] rsi: fix memory leaks and error handling in rsi_91x_usb

On Fri, Jun 27, 2014 at 12:51 AM, Alexey Khoroshilov
<[email protected]> wrote:
> The patch fixes a couple of issues:
> - absence of deallocation of rsi_dev->rx_usb_urb[0] in the driver;
> - potential NULL pointer dereference because of lack of checks for memory
> allocation success in rsi_init_usb_interface().
>
> By the way, it makes rsi_probe() returning error code instead of 1
> and fixes comments regarding returning values.
>
> Found by Linux Driver Verification project (linuxtesting.org).
>
> Signed-off-by: Alexey Khoroshilov <[email protected]>

I count four different issues being fixed, so maybe split this into
four patches?


Regards
Jonas