linux-kernel - Re: [RFC PATCH 0/2] dma-mapping: introduce helper for setting 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: <20180911233933.GA1402@kunai>
Date:   Wed, 12 Sep 2018 01:39:33 +0200
From:   Wolfram Sang <wsa@...-dreams.de>
To:     Robin Murphy <robin.murphy@....com>
Cc:     Wolfram Sang <wsa+renesas@...g-engineering.com>,
        iommu@...ts.linux-foundation.org,
        linux-renesas-soc@...r.kernel.org, Christoph Hellwig <hch@....de>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 0/2] dma-mapping: introduce helper for setting
 dma_parms

Hi Robin,

> > 	od = devm_kzalloc(&pdev->dev, sizeof(*od), GFP_KERNEL);
> > 	if (!od)
> > 		return -ENOMEM;
> > 
> > 	pdev->dev.dma_parms = &od->dma_parms;
> > 	dma_set_max_seg_size(&pdev->dev, 0x3FFFFFFF);
> > 
> > And that's all about handling dma_parms. So, on unbind, the memory for
> > 'od' gets freed and dma_params is a dangling pointer.
> 
> That's the typical case - the dma_parms structure is simply embedded in some
> other private data which tends to have the appropriate lifetime anyway. I
> can't see that the dangling pointer is an issue when it's set
> unconditionally on probe and valid until remove, because anyone
> dereferencing dev->dma_parms when dev has no driver bound is doing something
> very very wrong anyway. I suppose we could zero it in
> __device_release_driver() for good measure though - shame we've found
> something dma_deconfigure() could have been useful for just after we killed
> it ;)

I see. Yes, I was aware that the misuse of this dangling pointer is
somewhat academical. Yet, it was easy to fix and clearing this pointer
is good programming practice, I'd say. I agree that clearing the pointer
in __device_release_driver is a good option, too, if documentation about
its expected life cycle (== get's cleared on unbind) is clear about
that. Probably that life cycle confusion led to the more complicated
code in the exynos_drm driver. I will look into all of that tomorrow.

> Note that ultimately we'd like to have a single structure to hold all the
> masks and other gubbins (per the very original intent of dma_parms), so

I was wondering about that, yes.

> there's a fair chance of this getting fundamentally rejigged at some point
> anyway.

Makes sense. Yet, as this change is gonna be small, I think it's still
nice to have.

Thanks for the input!

   Wolfram


Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)