Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

24 participants
2284 discussions

[PATCH] dma-buf/fence-array: Add flex array to struct dma_fence_array

by Christophe JAILLET

This is an effort to get rid of all multiplications from allocation functions in order to prevent integer overflows [1][2]. The "struct dma_fence_array" can be refactored to add a flex array in order to have the "callback structures allocated behind the array" be more explicit. Do so: - makes the code more readable and safer. - allows using __counted_by() for additional checks - avoids some pointer arithmetic in dma_fence_array_enable_signaling() Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-a… [1] Link: https://github.com/KSPP/linux/issues/160 [2] Signed-off-by: Christophe JAILLET <christophe.jaillet(a)wanadoo.fr> --- Compile tested only. Also, I don't think that 'cb' is a great name and the associated kernel-doc description could certainly be improved. Any proposal welcomed :) --- drivers/dma-buf/dma-fence-array.c | 10 ++++------ include/linux/dma-fence-array.h | 3 +++ 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c index 9b3ce8948351..9c55afaca607 100644 --- a/drivers/dma-buf/dma-fence-array.c +++ b/drivers/dma-buf/dma-fence-array.c @@ -70,7 +70,7 @@ static void dma_fence_array_cb_func(struct dma_fence *f, static bool dma_fence_array_enable_signaling(struct dma_fence *fence) { struct dma_fence_array *array = to_dma_fence_array(fence); - struct dma_fence_array_cb *cb = (void *)(&array[1]); + struct dma_fence_array_cb *cb = array->cb; unsigned i; for (i = 0; i < array->num_fences; ++i) { @@ -168,22 +168,20 @@ struct dma_fence_array *dma_fence_array_create(int num_fences, bool signal_on_any) { struct dma_fence_array *array; - size_t size = sizeof(*array); WARN_ON(!num_fences || !fences); - /* Allocate the callback structures behind the array. */ - size += num_fences * sizeof(struct dma_fence_array_cb); - array = kzalloc(size, GFP_KERNEL); + array = kzalloc(struct_size(array, cb, num_fences), GFP_KERNEL); if (!array) return NULL; + array->num_fences = num_fences; + spin_lock_init(&array->lock); dma_fence_init(&array->base, &dma_fence_array_ops, &array->lock, context, seqno); init_irq_work(&array->work, irq_dma_fence_array_work); - array->num_fences = num_fences; atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences); array->fences = fences; diff --git a/include/linux/dma-fence-array.h b/include/linux/dma-fence-array.h index ec7f25def392..a793f9d5c73b 100644 --- a/include/linux/dma-fence-array.h +++ b/include/linux/dma-fence-array.h @@ -33,6 +33,7 @@ struct dma_fence_array_cb { * @num_pending: fences in the array still pending * @fences: array of the fences * @work: internal irq_work function + * @cb: array of callback helpers */ struct dma_fence_array { struct dma_fence base; @@ -43,6 +44,8 @@ struct dma_fence_array { struct dma_fence **fences; struct irq_work work; + + struct dma_fence_array_cb cb[] __counted_by(num_fences); }; /** -- 2.45.1

1 day, 10 hours

[PATCH 0/8] dma-buf: heaps: Support carved-out heaps and ECC related-flags

by Maxime Ripard

Hi, This series is the follow-up of the discussion that John and I had a few months ago here: https://lore.kernel.org/all/CANDhNCquJn6bH3KxKf65BWiTYLVqSd9892-xtFDHHqqyrr… The initial problem we were discussing was that I'm currently working on a platform which has a memory layout with ECC enabled. However, enabling the ECC has a number of drawbacks on that platform: lower performance, increased memory usage, etc. So for things like framebuffers, the trade-off isn't great and thus there's a memory region with ECC disabled to allocate from for such use cases. After a suggestion from John, I chose to start using heap allocations flags to allow for userspace to ask for a particular ECC setup. This is then backed by a new heap type that runs from reserved memory chunks flagged as such, and the existing DT properties to specify the ECC properties. We could also easily extend this mechanism to support more flags, or through a new ioctl to discover which flags a given heap supports. I submitted a draft PR to the DT schema for the bindings used in this PR: https://github.com/devicetree-org/dt-schema/pull/138 Let me know what you think, Maxime Signed-off-by: Maxime Ripard <mripard(a)kernel.org> --- Maxime Ripard (8): dma-buf: heaps: Introduce a new heap for reserved memory of: Add helper to retrieve ECC memory bits dma-buf: heaps: Import uAPI header dma-buf: heaps: Add ECC protection flags dma-buf: heaps: system: Remove global variable dma-buf: heaps: system: Handle ECC flags dma-buf: heaps: cma: Handle ECC flags dma-buf: heaps: carveout: Handle ECC flags drivers/dma-buf/dma-heap.c | 4 + drivers/dma-buf/heaps/Kconfig | 8 + drivers/dma-buf/heaps/Makefile | 1 + drivers/dma-buf/heaps/carveout_heap.c | 330 ++++++++++++++++++++++++++++++++++ drivers/dma-buf/heaps/cma_heap.c | 10 ++ drivers/dma-buf/heaps/system_heap.c | 29 ++- include/linux/dma-heap.h | 2 + include/linux/of.h | 25 +++ include/uapi/linux/dma-heap.h | 5 +- 9 files changed, 407 insertions(+), 7 deletions(-) --- base-commit: a38297e3fb012ddfa7ce0321a7e5a8daeb1872b6 change-id: 20240515-dma-buf-ecc-heap-28a311d2c94e Best regards, -- Maxime Ripard <mripard(a)kernel.org>

3 days, 11 hours

Re: Safety of opening up /dev/dma_heap/* to physically present users (udev uaccess tag) ?

by Daniel Vetter

On Mon, May 06, 2024 at 04:01:42PM +0200, Hans de Goede wrote: > Hi Sima, > > On 5/6/24 3:38 PM, Daniel Vetter wrote: > > On Mon, May 06, 2024 at 02:05:12PM +0200, Maxime Ripard wrote: > >> Hi, > >> > >> On Mon, May 06, 2024 at 01:49:17PM GMT, Hans de Goede wrote: > >>> Hi dma-buf maintainers, et.al., > >>> > >>> Various people have been working on making complex/MIPI cameras work OOTB > >>> with mainline Linux kernels and an opensource userspace stack. > >>> > >>> The generic solution adds a software ISP (for Debayering and 3A) to > >>> libcamera. Libcamera's API guarantees that buffers handed to applications > >>> using it are dma-bufs so that these can be passed to e.g. a video encoder. > >>> > >>> In order to meet this API guarantee the libcamera software ISP allocates > >>> dma-bufs from userspace through one of the /dev/dma_heap/* heaps. For > >>> the Fedora COPR repo for the PoC of this: > >>> https://hansdegoede.dreamwidth.org/28153.html > >> > >> For the record, we're also considering using them for ARM KMS devices, > >> so it would be better if the solution wasn't only considering v4l2 > >> devices. > >> > >>> I have added a simple udev rule to give physically present users access > >>> to the dma_heap-s: > >>> > >>> KERNEL=="system", SUBSYSTEM=="dma_heap", TAG+="uaccess" > >>> > >>> (and on Rasperry Pi devices any users in the video group get access) > >>> > >>> This was just a quick fix for the PoC. Now that we are ready to move out > >>> of the PoC phase and start actually integrating this into distributions > >>> the question becomes if this is an acceptable solution; or if we need some > >>> other way to deal with this ? > >>> > >>> Specifically the question is if this will have any negative security > >>> implications? I can certainly see this being used to do some sort of > >>> denial of service attack on the system (1). This is especially true for > >>> the cma heap which generally speaking is a limited resource. > >> > >> There's plenty of other ways to exhaust CMA, like allocating too much > >> KMS or v4l2 buffers. I'm not sure we should consider dma-heaps > >> differently than those if it's part of our threat model. > > > > So generally for an arm soc where your display needs cma, your render node > > doesn't. And user applications only have access to the later, while only > > the compositor gets a kms fd through logind. At least in drm aside from > > vc4 there's really no render driver that just gives you access to cma and > > allows you to exhaust that, you need to be a compositor with drm master > > access to the display. > > > > Which means we're mostly protected against bad applications, and that's > > not a threat the "user physically sits in front of the machine accounts > > for", and which giving cma access to everyone would open up. And with > > flathub/snaps/... this is very much an issue. > > I agree that bad applications are an issue, but not for the flathub / snaps > case. Flatpacks / snaps run sandboxed and don't have access to a full /dev > so those should not be able to open /dev/dma_heap/* independent of > the ACLs on /dev/dma_heap/*. The plan is for cameras using the > libcamera software ISP to always be accessed through pipewire and > the camera portal, so in this case pipewere is taking the place of > the compositor in your kms vs render node example. Yeah essentially if you clarify to "set the permissions such that pipewire can do allocations", then I think that makes sense. And is at the same level as e.g. drm kms giving compsitors (but _only_ compositors) special access rights. > So this reduces the problem to bad apps packaged by regular distributions > and if any of those misbehave the distros should fix that. > > So I think that for the denial of service side allowing physical > present users (but not sandboxed apps running as those users) to > access /dev/dma_heap/* should be ok. > > My bigger worry is if dma_heap (u)dma-bufs can be abused in other > ways then causing a denial of service. > > I guess that the answer there is that causing other security issues > should not be possible ? Well pinned memory exhaustion is a very useful tool to make all kinds of other kernel issues exploitable. Like if you have that you can weaponize all kinds of kmalloc error paths (and since it's untracked memory the oom killer will not get you of these issuees). I think for the pipewire based desktop it'd be best if you only allow pipewire to get at an fd for allocating from dma-heaps, kinda like logind furnishes the kms master fd ... Still has the issue that you can't nuke these buffers, but that's for another day. But at least from a "limit attack surface" design pov I think this would be better than just handing out access to the current user outright. But that's also not the worst option I guess, as long as snaps/flatpacks only go through the pipewire service. -Sima -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

3 days, 16 hours

Re: Safety of opening up /dev/dma_heap/* to physically present users (udev uaccess tag) ?

by Laurent Pinchart

Hi Nicolas, On Wed, May 15, 2024 at 01:43:58PM -0400, nicolas.dufresne(a)collabora.corp-partner.google.com wrote: > Le mardi 14 mai 2024 à 23:42 +0300, Laurent Pinchart a écrit : > > > You'll hit the same limitation as we hit in GStreamer, which is that KMS driver > > > only offer allocation for render buffers and most of them are missing allocators > > > for YUV buffers, even though they can import in these formats. (kms allocators, > > > except dumb, which has other issues, are format aware). > > > > My experience on Arm platforms is that the KMS drivers offer allocation > > for scanout buffers, not render buffers, and mostly using the dumb > > allocator API. If the KMS device can scan out YUV natively, YUV buffer > > allocation should be supported. Am I missing something here ? > > There is two APIs, Dumb is the legacy allocation API, only used by display Is it legacy only ? I understand the dumb buffers API to be officially supported, to allocate scanout buffers suitable for software rendering. > drivers indeed, and the API does not include a pixel format or a modifier. The > allocation of YUV buffer has been made through a small hack, > > bpp = number of bits per component (of luma plane if multiple planes) > width = width > height = height * X > > Where X will vary, "3 / 2" is used for 420 subsampling, "2" for 422 and "3" for > 444. It is far from idea, requires deep knowledge of each formats in the > application I'm not sure I see that as an issue, but our experiences and uses cases may vary :-) > and cannot allocate each planes seperatly. For semi-planar or planar formats, unless I'm mistaken, you can either allocate a single buffer and use it with appropriate offsets when constructing your framebuffer (with DRM_IOCTL_MODE_ADDFB2), or allocate one buffer per plane. > The second is to use the driver specific allocation API. This is then abstracted > by GBM. This allows allocating render buffers with notably modifiers and/or use > cases. But no support for YUV formats or multi-planar formats. GBM is the way to go for render buffers indeed. It has been designed with only graphics buffer management use cases in mind, so it's unfortunately not an option as a generic allocator, at least in its current form. -- Regards, Laurent Pinchart

3 days, 16 hours

Re: Safety of opening up /dev/dma_heap/* to physically present users (udev uaccess tag) ?

by Laurent Pinchart

On Mon, May 13, 2024 at 11:10:00AM -0400, Nicolas Dufresne wrote: > Le lundi 13 mai 2024 à 11:34 +0300, Laurent Pinchart a écrit : > > On Mon, May 13, 2024 at 10:29:22AM +0200, Maxime Ripard wrote: > > > 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. > > > > If the application wants to share buffers between the camera and a > > display engine or codec, it should arguably not use the libcamera > > FrameBufferAllocator, but allocate the buffers from the display or the > > encoder. memfd wouldn't be used in that case. > > > > We need to eat our own dogfood though. If we want to push the > > responsibility for buffer allocation in the buffer sharing case to the > > application, we need to modify the cam application to do so when using > > the KMS backend. > > Agreed, and the new dmabuf feedback on wayland can also be used on top of this. > > You'll hit the same limitation as we hit in GStreamer, which is that KMS driver > only offer allocation for render buffers and most of them are missing allocators > for YUV buffers, even though they can import in these formats. (kms allocators, > except dumb, which has other issues, are format aware). My experience on Arm platforms is that the KMS drivers offer allocation for scanout buffers, not render buffers, and mostly using the dumb allocator API. If the KMS device can scan out YUV natively, YUV buffer allocation should be supported. Am I missing something here ? -- Regards, Laurent Pinchart

3 days, 16 hours

[PATCH v5 0/9] dma-buf: heaps: Add restricted heap

by Yong Wu

The purpose of this patchset is for MediaTek secure video playback, and also to enable other potential uses of this in the future. The 'restricted dma-heap' will be used to allocate dma_buf objects that reference memory in the secure world that is inaccessible/unmappable by the non-secure (i.e. kernel/userspace) world. That memory will be used by the secure/ trusted world to store secure information (i.e. decrypted media content). The dma_bufs allocated from the kernel will be passed to V4L2 for video decoding (as input and output). They will also be used by the drm system for rendering of the content. This patchset adds two MediaTek restricted heaps and they will be used in v4l2[1] and drm[2]. 1) restricted_mtk_cm: secure chunk memory for MediaTek SVP (Secure Video Path). The buffer is reserved for the secure world after bootup and it is used for vcodec's ES/working buffer; 2) restricted_mtk_cma: secure CMA memory for MediaTek SVP. This buffer is dynamically reserved for the secure world and will be got when we start playing secure videos. Once the security video playing is complete, the CMA will be released. This heap is used for the vcodec's frame buffer. [1] https://lore.kernel.org/linux-mediatek/20240412090851.24999-1-yunfei.dong@m… [2] https://lore.kernel.org/linux-mediatek/20240403102701.369-1-shawn.sung@medi… Change note: v5: 1) Reconstruct TEE commands to allow the kernel to obtain the PA of the TEE buffer to initialize a valid sg table. 2) Previously, PA was hidden from the kernel. Then the kernel checks if this is restricted buffer by "if (sg_page(sg) == NULL)". In this version, we will add a new explicit interface (sg_dma_is_restricted) for users to determine whether this is a restricted buffer. 3) some words improve, like using "rheap". Rebase on v6.9-rc7. v4: https://lore.kernel.org/linux-mediatek/20240112092014.23999-1-yong.wu@media… 1) Rename the heap name from "secure" to "restricted". suggested from Simon/Pekka. There are still several "secure" string in MTK file since we use ARM platform in which we call this "secure world"/ "secure command". v3: https://lore.kernel.org/linux-mediatek/20231212024607.3681-1-yong.wu@mediat… 1) Separate the secure heap to a common file(secure_heap.c) and mtk special file (secure_heap_mtk.c), and put all the tee related code into our special file. 2) About dt-binding, Add "mediatek," prefix since this is Mediatek TEE firmware definition. 3) Remove the normal CMA heap which is a draft for qcom. Rebase on v6.7-rc1. v2: https://lore.kernel.org/linux-mediatek/20231111111559.8218-1-yong.wu@mediat… 1) Move John's patches into the vcodec patchset since they use the new dma heap interface directly. https://lore.kernel.org/linux-mediatek/20231106120423.23364-1-yunfei.dong@m… 2) Reword the dt-binding description. 3) Rename the heap name from mtk_svp to secure_mtk_cm. This means the current vcodec/DRM upstream code doesn't match this. 4) Add a normal CMA heap. currently it should be a draft version. 5) Regarding the UUID, I still use hard code, but put it in a private data which allow the others could set their own UUID. What's more, UUID is necessary for the session with TEE. If we don't have it, we can't communicate with the TEE, including the get_uuid interface, which tries to make uuid more generic, not working. If there is other way to make UUID more general, please free to tell me. v1: https://lore.kernel.org/linux-mediatek/20230911023038.30649-1-yong.wu@media… Base on v6.6-rc1. Yong Wu (9): dt-bindings: reserved-memory: Add mediatek,dynamic-restricted-region scatterlist: Add a flag for the restricted memory lib/scatterlist: Add sg_dup_table dma-buf: heaps: Initialize a restricted heap dma-buf: heaps: restricted_heap: Add private heap ops dma-buf: heaps: restricted_heap: Add dma_ops dma-buf: heaps: restricted_heap: Add MediaTek restricted heap and heap_init dma-buf: heaps: restricted_heap_mtk: Add TEE memory service call dma_buf: heaps: restricted_heap_mtk: Add a new CMA heap .../mediatek,dynamic-restricted-region.yaml | 43 ++ drivers/dma-buf/heaps/Kconfig | 16 + drivers/dma-buf/heaps/Makefile | 4 +- drivers/dma-buf/heaps/restricted_heap.c | 219 +++++++++ drivers/dma-buf/heaps/restricted_heap.h | 45 ++ drivers/dma-buf/heaps/restricted_heap_mtk.c | 423 ++++++++++++++++++ drivers/dma-buf/heaps/system_heap.c | 27 +- include/linux/scatterlist.h | 36 ++ lib/scatterlist.c | 26 ++ 9 files changed, 812 insertions(+), 27 deletions(-) create mode 100644 Documentation/devicetree/bindings/reserved-memory/mediatek,dynamic-restricted-region.yaml create mode 100644 drivers/dma-buf/heaps/restricted_heap.c create mode 100644 drivers/dma-buf/heaps/restricted_heap.h create mode 100644 drivers/dma-buf/heaps/restricted_heap_mtk.c -- 2.18.0

3 days, 18 hours

[PATCH v4 0/7] dma-buf: heaps: Add restricted heap

by Yong Wu

The purpose of this patchset is for MediaTek secure video playback, and also to enable other potential uses of this in the future. The 'restricted dma-heap' will be used to allocate dma_buf objects that reference memory in the secure world that is inaccessible/unmappable by the non-secure (i.e. kernel/userspace) world. That memory will be used by the secure/ trusted world to store secure information (i.e. decrypted media content). The dma_bufs allocated from the kernel will be passed to V4L2 for video decoding (as input and output). They will also be used by the drm system for rendering of the content. This patchset adds two MediaTek restricted heaps and they will be used in v4l2[1] and drm[2]. 1) restricted_mtk_cm: secure chunk memory for MediaTek SVP (Secure Video Path). The buffer is reserved for the secure world after bootup and it is used for vcodec's ES/working buffer; 2) restricted_mtk_cma: secure CMA memory for MediaTek SVP. This buffer is dynamically reserved for the secure world and will be got when we start playing secure videos. Once the security video playing is complete, the CMA will be released. This heap is used for the vcodec's frame buffer. [1] https://lore.kernel.org/linux-mediatek/20231206081538.17056-1-yunfei.dong@m… [2] https://lore.kernel.org/all/20231223182932.27683-1-jason-jh.lin@mediatek.co… Change note: v4: 1) Rename the heap name from "secure" to "restricted". suggested from Simon/Pekka. There are still several "secure" string in MTK file since we use ARM platform in which we call this "secure world"/ "secure command". v3: https://lore.kernel.org/linux-mediatek/20231212024607.3681-1-yong.wu@mediat… 1) Separate the secure heap to a common file(secure_heap.c) and mtk special file (secure_heap_mtk.c), and put all the tee related code into our special file. 2) About dt-binding, Add "mediatek," prefix since this is Mediatek TEE firmware definition. 3) Remove the normal CMA heap which is a draft for qcom. Rebase on v6.7-rc1. v2: https://lore.kernel.org/linux-mediatek/20231111111559.8218-1-yong.wu@mediat… 1) Move John's patches into the vcodec patchset since they use the new dma heap interface directly. https://lore.kernel.org/linux-mediatek/20231106120423.23364-1-yunfei.dong@m… 2) Reword the dt-binding description. 3) Rename the heap name from mtk_svp to secure_mtk_cm. This means the current vcodec/DRM upstream code doesn't match this. 4) Add a normal CMA heap. currently it should be a draft version. 5) Regarding the UUID, I still use hard code, but put it in a private data which allow the others could set their own UUID. What's more, UUID is necessary for the session with TEE. If we don't have it, we can't communicate with the TEE, including the get_uuid interface, which tries to make uuid more generic, not working. If there is other way to make UUID more general, please free to tell me. v1: https://lore.kernel.org/linux-mediatek/20230911023038.30649-1-yong.wu@media… Base on v6.6-rc1. Yong Wu (7): dt-bindings: reserved-memory: Add mediatek,dynamic-restricted-region dma-buf: heaps: Initialize a restricted heap dma-buf: heaps: restricted_heap: Add private heap ops dma-buf: heaps: restricted_heap: Add dma_ops dma-buf: heaps: restricted_heap: Add MediaTek restricted heap and heap_init dma-buf: heaps: restricted_heap_mtk: Add TEE memory service call dma_buf: heaps: restricted_heap_mtk: Add a new CMA heap .../mediatek,dynamic-restricted-region.yaml | 43 +++ drivers/dma-buf/heaps/Kconfig | 16 + drivers/dma-buf/heaps/Makefile | 4 +- drivers/dma-buf/heaps/restricted_heap.c | 237 +++++++++++++ drivers/dma-buf/heaps/restricted_heap.h | 43 +++ drivers/dma-buf/heaps/restricted_heap_mtk.c | 322 ++++++++++++++++++ 6 files changed, 664 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/reserved-memory/mediatek,dynamic-restricted-region.yaml create mode 100644 drivers/dma-buf/heaps/restricted_heap.c create mode 100644 drivers/dma-buf/heaps/restricted_heap.h create mode 100644 drivers/dma-buf/heaps/restricted_heap_mtk.c -- 2.18.0

4 days, 22 hours

Re: Safety of opening up /dev/dma_heap/* to physically present users (udev uaccess tag) ?

by Laurent Pinchart

On Mon, May 13, 2024 at 11:06:24AM -0400, Nicolas Dufresne wrote: > 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: > > > > > > 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. I think that issue should be fixed, regardless of whether or not we end up using dma heaps for libcamera. If we do use them, maybe there will be a higher incentive for somebody involved in this conversation to tackle that problem first :-) And maybe, as a result, the rest of the Linux community will consider with a more open mind usage of dma heaps on desktop systems. -- Regards, Laurent Pinchart

5 days, 7 hours

Re: Safety of opening up /dev/dma_heap/* to physically present users (udev uaccess tag) ?

by Laurent Pinchart

On Mon, May 13, 2024 at 10:29:22AM +0200, Maxime Ripard wrote: > 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. If the application wants to share buffers between the camera and a display engine or codec, it should arguably not use the libcamera FrameBufferAllocator, but allocate the buffers from the display or the encoder. memfd wouldn't be used in that case. We need to eat our own dogfood though. If we want to push the responsibility for buffer allocation in the buffer sharing case to the application, we need to modify the cam application to do so when using the KMS backend. -- Regards, Laurent Pinchart

6 days, 19 hours

Re: [PATCH] epoll: try to be a _bit_ better about file lifetimes

by David Laight

From: Christian Brauner > Sent: 10 May 2024 11:55 > > > For the uapi issue you describe below my take would be that we should just > > try, and hope that everyone's been dutifully using O_CLOEXEC. But maybe > > I'm biased from the gpu world, where we've been hammering it in that > > "O_CLOEXEC or bust" mantra since well over a decade. Really the only valid > > Oh, we're very much on the same page. All new file descriptor types that > I've added over the years are O_CLOEXEC by default. IOW, you need to > remove O_CLOEXEC explicitly (see pidfd as an example). And imho, any new > fd type that's added should just be O_CLOEXEC by default. For fd a shell redirect creates you may want so be able to say 'this fd will have O_CLOEXEC set after the next exec'. Also (possibly) a flag that can't be cleared once set and that gets kept by dup() etc. But maybe that is excessive? I've certainly used: # ip netns exec ns command 3</sys/class/net in order to be able to (easily) read status for interfaces in the default namespace and a specific namespace. The would be hard if the O_CLOEXEC flag had got set by default. (Especially without a shell builtin to clear it.) David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)

1 week, 1 day

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig