linux-kernel - Re: [PATCH] irqchip/stm32: Fix "WARNING: invalid free of devm

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] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 2 Dec 2019 13:22:14 +0000
From:   Fabien DESSENNE <fabien.dessenne@...com>
To:     Marc Zyngier <maz@...nel.org>,
        Daode Huang <huangdaode@...ilicon.com>
CC:     "jason@...edaemon.net" <jason@...edaemon.net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "mcoquelin.stm32@...il.com" <mcoquelin.stm32@...il.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "linux-stm32@...md-mailman.stormreply.com" 
        <linux-stm32@...md-mailman.stormreply.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        Alexandre TORGUE <alexandre.torgue@...com>
Subject: Re: [PATCH] irqchip/stm32: Fix "WARNING: invalid free of devm_
 allocated

Hi Daode,


I confirm that this patch is not a good idea, here are some explanations.

irq-stm32-exti.c deals with two different purposes:

- either it is used to probe the "st,stm32mp1-exti" compatible device. 
In that case .probe() is invoked and uses devm_kzalloc() to get memory. 
No need to free memory.

-either is it used for other stm32 devices. In that case, there is no 
probe function, the driver is 'just' init'ed. In that case, 
devm_kzalloc() is not used and explicit free memory is required.

As said by Mark, you have just mixed the two paths.

Fabien



On 02/12/2019 1:40 PM, Marc Zyngier wrote:
> On 2019-12-02 12:29, Marc Zyngier wrote:
>> On 2019-11-28 09:04, Daode Huang wrote:
>>> Since devm_ allocated data can be automaitcally released, it's no
>>> need to free it apparently, just remove it.
>>>
>>> Fixes: cfbf9e497094 ("irqchip/stm32: Use a platform driver for
>>> stm32mp1-exti device")
>>> Signed-off-by: Daode Huang <huangdaode@...ilicon.com>
>>> ---
>>>  drivers/irqchip/irq-stm32-exti.c | 2 --
>>>  1 file changed, 2 deletions(-)
>>>
>>> diff --git a/drivers/irqchip/irq-stm32-exti.c
>>> b/drivers/irqchip/irq-stm32-exti.c
>>> index e00f2fa..46ec0af 100644
>>> --- a/drivers/irqchip/irq-stm32-exti.c
>>> +++ b/drivers/irqchip/irq-stm32-exti.c
>>> @@ -779,8 +779,6 @@ static int __init stm32_exti_init(const struct
>>> stm32_exti_drv_data *drv_data,
>>>      irq_domain_remove(domain);
>>>  out_unmap:
>>>      iounmap(host_data->base);
>>> -    kfree(host_data->chips_data);
>>> -    kfree(host_data);
>>>      return ret;
>>>  }
>>
>> Applied, thanks.
>
> Scratch that. This patch is just wrong, and just reading the code
> makes it obvious. stm32_exti_init() is only called on paths
> that allocate the memory with kmalloc.
>
> Clearly you haven't tried to understand what is going on.
>
>         M.