netdev - Re: [PATCH 3/3] net: can: peak_usb: Do not do dma on the stack

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]

Message-ID: <52AC5F0F.20105@pengutronix.de>
Date:	Sat, 14 Dec 2013 14:37:19 +0100
From:	Marc Kleine-Budde <mkl@...gutronix.de>
To:	Stephane Grosjean <s.grosjean@...k-system.com>
CC:	netdev@...r.kernel.org, linux-can@...r.kernel.org
Subject: Re: [PATCH 3/3] net: can: peak_usb: Do not do dma on the stack

On 12/12/2013 03:44 PM, Stephane Grosjean wrote:
> Going back into linux-can peak_usb, and I'm currently having a quick
> look to what has changed during these several past months.
> I agree with the below patch that fixes the "DMAusage with USB core"
> issue, but - maybe I'm wrong - isn't there now amemory leak issue in
> "pcan_usb_pro_init()" function below?
> 
> I mean, you now allocate "fi" and "bi" memory objects but, AFAIK, they
> aren't freed anywhere in the function, don't they?

Thanks for pointing out.

[...]

>> b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
>> index 30d79bf..8ee9d15 100644
>> --- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
>> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
[...]
>> @@ -851,21 +860,24 @@ static int pcan_usb_pro_stop(struct
>> peak_usb_device *dev)
>>    */
>>   static int pcan_usb_pro_init(struct peak_usb_device *dev)
>>   {
>> -    struct pcan_usb_pro_interface *usb_if;
>>       struct pcan_usb_pro_device *pdev =
>>               container_of(dev, struct pcan_usb_pro_device, dev);
>> +    struct pcan_usb_pro_interface *usb_if = NULL;
>> +    struct pcan_usb_pro_fwinfo *fi = NULL;
>> +    struct pcan_usb_pro_blinfo *bi = NULL;
>> +    int err;
>>         /* do this for 1st channel only */
>>       if (!dev->prev_siblings) {
>> -        struct pcan_usb_pro_fwinfo fi;
>> -        struct pcan_usb_pro_blinfo bi;
>> -        int err;
>> -
>>           /* allocate netdevices common structure attached to first
>> one */
>>           usb_if = kzalloc(sizeof(struct pcan_usb_pro_interface),
>>                    GFP_KERNEL);
>> -        if (!usb_if)
>> -            return -ENOMEM;
>> +        fi = kmalloc(sizeof(struct pcan_usb_pro_fwinfo), GFP_KERNEL);
>> +        bi = kmalloc(sizeof(struct pcan_usb_pro_blinfo), GFP_KERNEL);
>> +        if (!usb_if || !fi || !bi) {
>> +            err = -ENOMEM;
>> +            goto err_out;
>> +        }
>>             /* number of ts msgs to ignore before taking one into
>> account */
>>           usb_if->cm_ignore_count = 5;
>> @@ -877,34 +889,34 @@ static int pcan_usb_pro_init(struct
>> peak_usb_device *dev)
>>            */
>>           err = pcan_usb_pro_send_req(dev, PCAN_USBPRO_REQ_INFO,
>>                           PCAN_USBPRO_INFO_FW,
>> -                        &fi, sizeof(fi));
>> +                        fi, sizeof(*fi));
>>           if (err) {
>> -            kfree(usb_if);
>>               dev_err(dev->netdev->dev.parent,
>>                   "unable to read %s firmware info (err %d)\n",
>>                   pcan_usb_pro.name, err);
>> -            return err;
>> +            goto err_out;
>>           }
>>             err = pcan_usb_pro_send_req(dev, PCAN_USBPRO_REQ_INFO,
>>                           PCAN_USBPRO_INFO_BL,
>> -                        &bi, sizeof(bi));
>> +                        bi, sizeof(*bi));
>>           if (err) {
>> -            kfree(usb_if);
>>               dev_err(dev->netdev->dev.parent,
>>                   "unable to read %s bootloader info (err %d)\n",
>>                   pcan_usb_pro.name, err);
>> -            return err;
>> +            goto err_out;
>>           }
>>   +        /* tell the device the can driver is running */
>> +        err = pcan_usb_pro_drv_loaded(dev, 1);
>> +        if (err)
>> +            goto err_out;
>> +
>>           dev_info(dev->netdev->dev.parent,
>>                "PEAK-System %s hwrev %u serial %08X.%08X (%u
>> channels)\n",
>>                pcan_usb_pro.name,
>> -             bi.hw_rev, bi.serial_num_hi, bi.serial_num_lo,
>> +             bi->hw_rev, bi->serial_num_hi, bi->serial_num_lo,
>>                pcan_usb_pro.ctrl_count);
>> -
>> -        /* tell the device the can driver is running */
>> -        pcan_usb_pro_drv_loaded(dev, 1);
>>       } else {
>>           usb_if = pcan_usb_pro_dev_if(dev->prev_siblings);
>>       }
>> @@ -916,6 +928,13 @@ static int pcan_usb_pro_init(struct
>> peak_usb_device *dev)
>>       pcan_usb_pro_set_led(dev, 0, 1);
>>         return 0;
>> +
>> + err_out:
>> +    kfree(bi);
>> +    kfree(fi);
>> +    kfree(usb_if);
>> +
>> +    return err;
>>   }
>>     static void pcan_usb_pro_exit(struct peak_usb_device *dev)

I'll send a fix.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


Download attachment "signature.asc" of type "application/pgp-signature" (260 bytes)