linux-kernel - Re: [PATCH] fsi: Aspeed: Fix a potential double free

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: <6926bb63-836b-b37c-3605-d6df9992bfaf@roeck-us.net>
Date:   Thu, 6 Jan 2022 17:35:10 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Christophe JAILLET <christophe.jaillet@...adoo.fr>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        Greg KH <gregkh@...uxfoundation.org>
Cc:     kernel-janitors@...r.kernel.org, linux-aspeed@...ts.ozlabs.org,
        alistair@...ple.id.au, linux-kernel@...r.kernel.org,
        linux-fsi@...ts.ozlabs.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] fsi: Aspeed: Fix a potential double free

On 1/6/22 10:35 AM, Christophe JAILLET wrote:
> Le 06/01/2022 à 18:25, Guenter Roeck a écrit :
>> On 1/6/22 12:14 AM, Dan Carpenter wrote:
>>> On Mon, Dec 27, 2021 at 07:29:07AM +0100, Greg KH wrote:
>>>> On Sun, Dec 26, 2021 at 05:56:02PM +0100, Christophe JAILLET wrote:
>>>>> 'aspeed' is a devm_alloc'ed, so there is no need to free it explicitly or
>>>>> there will be a double free().
>>>>
>>>> A struct device can never be devm_alloced for obvious reasons.  Perhaps
>>>> that is the real problem here?
>>>>
>>>
>>> I don't understand how "aspeed" is a struct device.
>>>
>>
>> -static void aspeed_master_release(struct device *dev)
>> -{
>> -    struct fsi_master_aspeed *aspeed =
>> -        to_fsi_master_aspeed(dev_to_fsi_master(dev));
>> -
>> -    kfree(aspeed);
>> -}
>>
>> So "dev" is embedded in struct fsi_master, and struct fsi_master is embedded
>> in struct fsi_master_aspeed. Since "struct device" is embedded, the data
>> structure embedding it must be released with the release function, as is done
>> here. The problem is indeed that the data structure is allocated with
>> devm_kzalloc(), which as Greg points out must not be devm_ allocated
>> (because its lifetime does not match the lifetime of devm_ allocated
>> memory).
> 
> Thanks a lot for the detailed explanation.
> Crystal clear for me now.
> 
> Do you want me to send a patch to remove the devm_ or will you?
> 

Sorry, I am way behind with code reviews. I won't have time to submit a patch.

Guenter