DmaBufAllocator: Open heap device files O_RDONLY
diff mbox series

Message ID 20260709183049.2282221-1-tjmercier@google.com
State New
Headers show
Series
  • DmaBufAllocator: Open heap device files O_RDONLY
Related show

Commit Message

T.J. Mercier July 9, 2026, 6:30 p.m. UTC
The write access mode is not required to issue ioctls and allocate
dma-bufs from heaps. Applications should be opening them as O_RDONLY.

Permission errors can be encountered while attempting to open() files in
/dev/dma_heap/* on systems where write permissions are not available. So
apply the principle of least privilege and remove the write access mode.

Signed-off-by: T.J. Mercier <tjmercier@google.com>
---
 src/libcamera/dma_buf_allocator.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Laurent Pinchart July 9, 2026, 7:47 p.m. UTC | #1
On Thu, Jul 09, 2026 at 11:30:45AM -0700, T.J. Mercier wrote:
> The write access mode is not required to issue ioctls and allocate
> dma-bufs from heaps. Applications should be opening them as O_RDONLY.

Is that explictly documented somewhere, or just the current behaviour ?
I suppose it won't change as that would introduce regressions.

> Permission errors can be encountered while attempting to open() files in
> /dev/dma_heap/* on systems where write permissions are not available. So
> apply the principle of least privilege and remove the write access mode.

I'm curious, what systems have you encountered read-only dma-buf heap
device nodes on ?

> Signed-off-by: T.J. Mercier <tjmercier@google.com>
> ---
>  src/libcamera/dma_buf_allocator.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp
> index d8c62dd67..c673d23ec 100644
> --- a/src/libcamera/dma_buf_allocator.cpp
> +++ b/src/libcamera/dma_buf_allocator.cpp
> @@ -100,7 +100,7 @@ DmaBufAllocator::DmaBufAllocator(DmaBufAllocatorFlags type)
>  		if (!(type & info.type))
>  			continue;
>  
> -		int ret = ::open(info.deviceNodeName, O_RDWR | O_CLOEXEC, 0);
> +		int ret = ::open(info.deviceNodeName, O_RDONLY | O_CLOEXEC, 0);
>  		if (ret < 0) {
>  			ret = errno;
>  			LOG(DmaBufAllocator, Debug)
T.J. Mercier July 9, 2026, 8:04 p.m. UTC | #2
On Thu, Jul 9, 2026 at 12:47 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Thu, Jul 09, 2026 at 11:30:45AM -0700, T.J. Mercier wrote:
> > The write access mode is not required to issue ioctls and allocate
> > dma-bufs from heaps. Applications should be opening them as O_RDONLY.
>
> Is that explictly documented somewhere, or just the current behaviour ?
> I suppose it won't change as that would introduce regressions.

Hi, Unfortunately I don't think it's documented anywhere. Even the
kernel selftests are mixed (which some users were pointing at), but
I've addressed those here:
https://lore.kernel.org/all/20260701192210.2997769-1-tjmercier@google.com/

The file descriptor is really all that's needed, however it is
obtained. The ioctl handler in the dma-heap driver intentionally
performs no permission checks.

> > Permission errors can be encountered while attempting to open() files in
> > /dev/dma_heap/* on systems where write permissions are not available. So
> > apply the principle of least privilege and remove the write access mode.
>
> I'm curious, what systems have you encountered read-only dma-buf heap
> device nodes on ?

Android: https://cs.android.com/android/platform/superproject/+/android-latest-release:system/core/rootdir/ueventd.rc;l=55-57

> > Signed-off-by: T.J. Mercier <tjmercier@google.com>
> > ---
> >  src/libcamera/dma_buf_allocator.cpp | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp
> > index d8c62dd67..c673d23ec 100644
> > --- a/src/libcamera/dma_buf_allocator.cpp
> > +++ b/src/libcamera/dma_buf_allocator.cpp
> > @@ -100,7 +100,7 @@ DmaBufAllocator::DmaBufAllocator(DmaBufAllocatorFlags type)
> >               if (!(type & info.type))
> >                       continue;
> >
> > -             int ret = ::open(info.deviceNodeName, O_RDWR | O_CLOEXEC, 0);
> > +             int ret = ::open(info.deviceNodeName, O_RDONLY | O_CLOEXEC, 0);
> >               if (ret < 0) {
> >                       ret = errno;
> >                       LOG(DmaBufAllocator, Debug)
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series

diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp
index d8c62dd67..c673d23ec 100644
--- a/src/libcamera/dma_buf_allocator.cpp
+++ b/src/libcamera/dma_buf_allocator.cpp
@@ -100,7 +100,7 @@  DmaBufAllocator::DmaBufAllocator(DmaBufAllocatorFlags type)
 		if (!(type & info.type))
 			continue;
 
-		int ret = ::open(info.deviceNodeName, O_RDWR | O_CLOEXEC, 0);
+		int ret = ::open(info.deviceNodeName, O_RDONLY | O_CLOEXEC, 0);
 		if (ret < 0) {
 			ret = errno;
 			LOG(DmaBufAllocator, Debug)