linux-kernel - Re: Bad kfree of dma

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: <f3c58dcd-b806-95ef-2434-3084e65e1afb@ti.com>
Date:   Wed, 20 May 2020 12:18:26 +0300
From:   Tomi Valkeinen <tomi.valkeinen@...com>
To:     Marek Szyprowski <m.szyprowski@...sung.com>,
        Linux Media Mailing List <linux-media@...r.kernel.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: Bad kfree of dma_parms in v5.7-rc5

Hi Marek,

On 20/05/2020 12:13, Marek Szyprowski wrote:
> Hi Tomi,
> 
> On 20.05.2020 11:00, Tomi Valkeinen wrote:
>> Commit 9495b7e92f716ab2bd6814fab5e97ab4a39adfdd ("driver core:
>> platform: Initialize dma_parms for platform devices") v5.7-rc5 causes
>> at least some v4l2 platform drivers to break when freeing resources.
>>
>> E.g. drivers/media/platform/ti-vpe/cal.c uses
>> vb2_dma_contig_set_max_seg_size() and
>> vb2_dma_contig_clear_max_seg_size() to manage the dma_params, and
>> similar pattern is seen in other drivers too.
>>
>> After 9495b7e92f716ab2, vb2_dma_contig_set_max_seg_size() will not
>> allocate anything, but vb2_dma_contig_clear_max_seg_size() will still
>> kfree the dma_params.
>>
>> I'm not sure what's the proper fix here. A flag somewhere to indicate
>> that vb2_dma_contig_set_max_seg_size() did allocate, and thus
>> vb2_dma_contig_clear_max_seg_size() must free?
>>
>> Or drop the kzalloc and kfree totally, if dma_params is now supposed
>> to always be there?
> 
> Thanks for reporting this issue!
> 
> Once the mentioned commit has been merged, the code should assume that
> the platform devices does have a struct dma_params allocated, so the
> proper fix is to alloc dma_params only if the bus is not a platform bus:
> 
> if (!dev_is_platform(dev) && !dev->dma_parms) {
>       dev->dma_parms = kzalloc(sizeof(*dev->dma_parms), GFP_KERNEL);
> 
> same check for the free path.

There is also "amba: Initialize dma_parms for amba devices". And the commit message says PCI devices 
do this too.

Guessing this based on the device type doesn't sound like a good idea to me.

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki