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

Date:   Wed, 20 May 2020 15:12:47 +0200
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Marek Szyprowski <m.szyprowski@...sung.com>,
        Tomi Valkeinen <tomi.valkeinen@...com>
Cc:     Linux Media Mailing List <linux-media@...r.kernel.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: Bad kfree of dma_parms in v5.7-rc5

+ Greg

On Wed, 20 May 2020 at 14:54, Marek Szyprowski <m.szyprowski@...sung.com> wrote:
>
> Hi Tomi,
>
> On 20.05.2020 14:43, Tomi Valkeinen wrote:
> > On 20/05/2020 12:22, Marek Szyprowski wrote:
> >> On 20.05.2020 11:18, Tomi Valkeinen wrote:
> >>> On 20/05/2020 12:13, Marek Szyprowski wrote:
> >>>> 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.
> >>
> >> Indeed. Then replace the allocation with a simple check for NULL
> >> dma_parms and return an error in such case. This should be enough for
> >> v5.8. Later we can simply get rid of those helpers and inline setting
> >> max segment size directly to the drivers.

That seems like a good idea, in the long run.

> >
> > Is that valid either? Then we assume that dma_parms is always set up
> > by someone else. That's true for platform devices and apparently some
> > other devices, but is it true for all devices now?
>
> # git grep vb2_dma_contig_set_max_seg_size | wc -l
>
> 18
>
> I've checked all clients of the vb2_dma_contig_set_max_seg_size
> function. There are only 9 drivers, all of them are platform device
> drivers. We don't care about off-tree users, so the proposed approach is
> imho fine.

Thanks for reporting and for looking into this. I apologize for the mess!

There is one case, where the above solution could be a problem (unless
I am wrong). That is, s5p_mfc_configure_2port_memory() that calls
s5p_mfc_alloc_memdev(), which allocates/initializes an internal struct
*device. Thus, this doesn't have the dev->dma_parms
allocated/assigned.

In other words, we would need to manage alloc/free for the
dev->dma_parms to have a complete fix. Maybe in
s5p_mfc_configure|unconfigure_2port_memory()!?

Additionally, I think reverting the offending commit, as discussed
above, could cause even more issues, as it's even included for
v5.6-stable kernels. I will go through all cases, more carefully this
time, of how ->dma_parms is managed, to be sure there are no more
conflicting cases.

Kind regards
Uffe