Message-ID: <643c6d3da9c7f45c32e01dd7179681117557ed4d.camel@collabora.com>
Date: Mon, 13 May 2024 11:06:24 -0400
From: Nicolas Dufresne <nicolas.dufresne@...labora.com>
To: Maxime Ripard <mripard@...hat.com>
Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>, Bryan O'Donoghue
 <bryan.odonoghue@...aro.org>, Dmitry Baryshkov
 <dmitry.baryshkov@...aro.org>,  Hans de Goede <hdegoede@...hat.com>, Sumit
 Semwal <sumit.semwal@...aro.org>, Benjamin Gaignard
 <benjamin.gaignard@...labora.com>, Brian Starkey <Brian.Starkey@....com>, 
 John Stultz <jstultz@...gle.com>, "T.J. Mercier" <tjmercier@...gle.com>,
 Christian König <christian.koenig@....com>, Lennart
 Poettering <mzxreary@...inter.de>,  Robert Mader
 <robert.mader@...labora.com>, Sebastien Bacher
 <sebastien.bacher@...onical.com>, Linux Media Mailing List
 <linux-media@...r.kernel.org>, "dri-devel@...ts.freedesktop.org"
 <dri-devel@...ts.freedesktop.org>,  linaro-mm-sig@...ts.linaro.org, Linux
 Kernel Mailing List <linux-kernel@...r.kernel.org>, Milan Zamazal
 <mzamazal@...hat.com>, Andrey Konovalov <andrey.konovalov.ynk@...il.com>
Subject: Re: Safety of opening up /dev/dma_heap/* to physically present
 users (udev uaccess tag) ?

Le lundi 13 mai 2024 à 15:51 +0200, Maxime Ripard a écrit :
> On Mon, May 13, 2024 at 09:42:00AM -0400, Nicolas Dufresne wrote:
> > Le lundi 13 mai 2024 à 10:29 +0200, Maxime Ripard a écrit :
> > > On Wed, May 08, 2024 at 10:36:08AM +0200, Daniel Vetter wrote:
> > > > On Tue, May 07, 2024 at 04:07:39PM -0400, Nicolas Dufresne wrote:
> > > > > Hi,
> > > > > 
> > > > > Le mardi 07 mai 2024 à 21:36 +0300, Laurent Pinchart a écrit :
> > > > > > Shorter term, we have a problem to solve, and the best option we have
> > > > > > found so far is to rely on dma-buf heaps as a backend for the frame
> > > > > > buffer allocatro helper in libcamera for the use case described above.
> > > > > > This won't work in 100% of the cases, clearly. It's a stop-gap measure
> > > > > > until we can do better.
> > > > > 
> > > > > Considering the security concerned raised on this thread with dmabuf heap
> > > > > allocation not be restricted by quotas, you'd get what you want quickly with
> > > > > memfd + udmabuf instead (which is accounted already).
> > > > > 
> > > > > It was raised that distro don't enable udmabuf, but as stated there by Hans, in
> > > > > any cases distro needs to take action to make the softISP works. This
> > > > > alternative is easy and does not interfere in anyway with your future plan or
> > > > > the libcamera API. You could even have both dmabuf heap (for Raspbian) and the
> > > > > safer memfd+udmabuf for the distro with security concerns.
> > > > > 
> > > > > And for the long term plan, we can certainly get closer by fixing that issue
> > > > > with accounting. This issue also applied to v4l2 io-ops, so it would be nice to
> > > > > find common set of helpers to fix these exporters.
> > > > 
> > > > Yeah if this is just for softisp, then memfd + udmabuf is also what I was
> > > > about to suggest. Not just as a stopgap, but as the real official thing.
> > > > 
> > > > udmabuf does kinda allow you to pin memory, but we can easily fix that by
> > > > adding the right accounting and then either let mlock rlimits or cgroups
> > > > kernel memory limits enforce good behavior.
> > > 
> > > I think the main drawback with memfd is that it'll be broken for devices
> > > without an IOMMU, and while you said that it's uncommon for GPUs, it's
> > > definitely not for codecs and display engines.
> > 
> > In the context of libcamera, the allocation and the alignment done to the video
> > frame is done completely blindly. In that context, there is a lot more then just
> > the allocation type that can go wrong and will lead to a memory copy. The upside
> > of memfd, is that the read cache will help speeding up the copies if they are
> > needed.
> 
> dma-heaps provide cacheable buffers too...

Yes, and why we have cache hints in V4L2 now. There is no clue that softISP code
can read to make the right call. The required cache management in undefined
until all the importer are known. I also don't think heaps currently care to
adapt the dmabuf sync behaviour based on the different importers, or the
addition of a new importer. On top of which, there is insufficient information
on the device to really deduce what is needed.

> 
> > Another important point is that this is only used if the application haven't
> > provided frames. If your embedded application is non-generic, and you have
> > permissions to access the right heap, the application can solve your specific
> > issue. But in the generic Linux space, Linux kernel API are just insufficient
> > for the "just work" scenario.
> 
> ... but they also provide semantics around the memory buffers that no
> other allocation API do. There's at least the mediatek secure playback
> series and another one that I've started to work on to allocate ECC
> protected or unprotected buffers that are just the right use case for
> the heaps, and the target frameworks aren't.

Let's agree we are both off topic now. The libcamera softISP is currently purely
software, and cannot write to any form of protected memory. As for ECC, I would
hope this usage will be coded in the application and that this application has
been authorized to access the appropriate heaps.

And finally, none of this fixes the issue that the heap allocation are not being
accounted properly and allow of an easy memory DoS. So uaccess should be granted
with care, meaning that defaulting a "desktop" library to that, means it will
most of the time not work at all.

Nicolas

Powered by blists - more mailing lists