linux-kernel - Re: [PATCH] drivers/uio/uio_pdrv_genirq.c: Fix memory leak & confusing labels

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Wed, 28 Nov 2012 01:07:26 +0100
From:	Cong Ding <dinggnu@...il.com>
To:	"Hans J. Koch" <hjk@...sjkoch.de>
Cc:	Vitalii Demianets <vitas@...factor.kiev.ua>,
	linux-kernel@...r.kernel.org,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH] drivers/uio/uio_pdrv_genirq.c: Fix memory leak &
 confusing labels

On Wed, Nov 28, 2012 at 12:07 AM, Hans J. Koch <hjk@...sjkoch.de> wrote:
> On Tue, Nov 27, 2012 at 07:29:32PM +0200, Vitalii Demianets wrote:
>> Memory leak was caused by jumping to the wrong exit label. So, it is good time
>> to improve misleading label names too.
>
> I agree that bad0, bad1, and bad2 are not the best choice for label names...
> I don't have any objections to your renaming.
>
> But there's a more serious bug, maybe you can fix that as well while you're
> at it? (See below)
>
> Thanks,
> Hans
>
>>
>> Signed-off-by: Vitalii Demianets <vitas@...factor.kiev.ua>
>> ---
>>  drivers/uio/uio_pdrv_genirq.c |   21 +++++++++++----------
>>  1 files changed, 11 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
>> index 42202cd..b88cf7b 100644
>> --- a/drivers/uio/uio_pdrv_genirq.c
>> +++ b/drivers/uio/uio_pdrv_genirq.c
>> @@ -110,7 +110,7 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
>>               if (!uioinfo) {
>>                       ret = -ENOMEM;
>>                       dev_err(&pdev->dev, "unable to kmalloc\n");
>> -                     goto bad2;
>> +                     goto out;
>>               }
>>               uioinfo->name = pdev->dev.of_node->name;
>>               uioinfo->version = "devicetree";
>> @@ -125,20 +125,20 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
>>
>>       if (!uioinfo || !uioinfo->name || !uioinfo->version) {
>>               dev_err(&pdev->dev, "missing platform_data\n");
>> -             goto bad0;
>> +             goto out_uioinfo;
>>       }
>>
>>       if (uioinfo->handler || uioinfo->irqcontrol ||
>>           uioinfo->irq_flags & IRQF_SHARED) {
>>               dev_err(&pdev->dev, "interrupt configuration error\n");
>> -             goto bad0;
>> +             goto out_uioinfo;
>>       }
>>
>>       priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>>       if (!priv) {
>>               ret = -ENOMEM;
>>               dev_err(&pdev->dev, "unable to kmalloc\n");
>> -             goto bad0;
>> +             goto out_uioinfo;
>>       }
>>
>>       priv->uioinfo = uioinfo;
>> @@ -150,7 +150,7 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
>>               ret = platform_get_irq(pdev, 0);
>>               if (ret < 0) {
>>                       dev_err(&pdev->dev, "failed to get IRQ\n");
>> -                     goto bad0;
>> +                     goto out_priv;
>>               }
>>               uioinfo->irq = ret;
>>       }
>> @@ -205,19 +205,20 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
>>       ret = uio_register_device(&pdev->dev, priv->uioinfo);
>>       if (ret) {
>>               dev_err(&pdev->dev, "unable to register uio device\n");
>> -             goto bad1;
>> +             goto out_pm;
>>       }
>>
>>       platform_set_drvdata(pdev, priv);
>>       return 0;
>> - bad1:
>> -     kfree(priv);
>> +out_pm:
>>       pm_runtime_disable(&pdev->dev);
>> - bad0:
>> +out_priv:
>> +     kfree(priv);
>> +out_uioinfo:
>>       /* kfree uioinfo for OF */
>>       if (pdev->dev.of_node)
>>               kfree(uioinfo);
>
> The free() depends on pdev->dev.of_node, while the allocation doesn't!!!!
> That's another source of memory leaks.
I don't agree. In line 99, it is
struct uio_info *uioinfo = pdev->dev.platform_data;
if uioinfo doesn't equal to NULL, it will run to line 126,
if (!uioinfo || !uioinfo->name || !uioinfo->version) {
and then if uioinfo->name equals to NULL, it runs to line 127 and 128,
and then goto bad0. If in this flow, we have to check
pdev->dev.of_node before free(uioinfo), right?

btw, I think in line 126 it is not necessary to check (!uioinfo),
because if uioinfo equals to NULL, it will go to line 109, and if the
alloc fails, it will go to bad2. uioinfo has no chance to be NULL when
runs to line 126. So I'd like to suggest a patch to avoid unnecessary
check like this

diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
 index 42202cd..3eb4fa2 100644
 --- a/drivers/uio/uio_pdrv_genirq.c
 +++ b/drivers/uio/uio_pdrv_genirq.c
 @@ -123,7 +123,7 @@ static int uio_pdrv_genirq_probe(struct
platform_device *pdev)
             uioinfo->irq = irq;
     }

 -   if (!uioinfo || !uioinfo->name || !uioinfo->version) {
 +   if (!uioinfo->name || !uioinfo->version) {
         dev_err(&pdev->dev, "missing platform_data\n");
         goto bad0;
     }


>
>> - bad2:
>> +out:
>>       return ret;
>>  }
>>
>> --
>> 1.7.8.6
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/