| Message ID | 20260709183049.2282221-1-tjmercier@google.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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)
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
Hi T.J. On Thu, Jul 09, 2026 at 01:04:42PM -0700, T.J. Mercier wrote: > 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. > Has this landed ? In this case I suppose we should go ahead and collect this patch ? Thanks j > > > 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
On Mon, Aug 3, 2026 at 3:01 AM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi T.J. > > On Thu, Jul 09, 2026 at 01:04:42PM -0700, T.J. Mercier wrote: > > 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. > > > > Has this landed ? Thanks for following up. I was asked to split the changes for different trees. The netdev patch has landed: https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=be6f0d0bae22 The udmabuf patch is still waiting: https://lore.kernel.org/all/20260722211037.1925308-1-tjmercier@google.com/ > In this case I suppose we should go ahead and collect this patch ? I would appreciate it! > > Thanks > j Thanks, T.J. > > > > 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
Hi 2026. 07. 09. 22:04 keltezéssel, T.J. Mercier írta: > 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. I have to say, it seems a bit odd to me that one can allocate without write access. >> >> 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 I'm wondering, why are those heaps made read-only? Couldn't it be the intent that they are not to be used? In any case, if the upstream consensus is O_RDONLY, then Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >>> 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
On Tue, Aug 4, 2026 at 3:42 AM Barnabás Pőcze <barnabas.pocze@ideasonboard.com> wrote: > > Hi Hi Barnabás, thanks for the review. > > 2026. 07. 09. 22:04 keltezéssel, T.J. Mercier írta: > > 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. > > I have to say, it seems a bit odd to me that one can allocate without write access. > > > >> > >> 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 > > I'm wondering, why are those heaps made read-only? > Couldn't it be the intent that they are not to be used? No, the system heap is the most widely used heap on Android and there is a requirement that all Android devices have one. They were made read only because write access isn't required to use them, and the security team noticed. The overwhelming majority of all device drivers don't require write permissions on the device file used to call their ioctl handlers. In the case of dma-buf heaps, ioctl() is really the only functional I/O syscall. > In any case, if the upstream consensus is O_RDONLY, then > > Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > > >>> 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 >
On Tue, Aug 04, 2026 at 09:33:49AM -0700, T.J. Mercier wrote: > On Tue, Aug 4, 2026 at 3:42 AM Barnabás Pőcze wrote: > > 2026. 07. 09. 22:04 keltezéssel, T.J. Mercier írta: > > > On Thu, Jul 9, 2026 at 12:47 PM Laurent Pinchart 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. > > > > I have to say, it seems a bit odd to me that one can allocate without write access. > > > > >> 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 > > > > I'm wondering, why are those heaps made read-only? > > Couldn't it be the intent that they are not to be used? > > No, the system heap is the most widely used heap on Android and there > is a requirement that all Android devices have one. They were made > read only because write access isn't required to use them, and the > security team noticed. The overwhelming majority of all device drivers > don't require write permissions on the device file used to call their > ioctl handlers. In the case of dma-buf heaps, ioctl() is really the > only functional I/O syscall. How is R/W a security issue, when a device only exposes ioctls ? > > In any case, if the upstream consensus is O_RDONLY, then > > > > Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > > >>> 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)
On Sun, Aug 9, 2026 at 11:11 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Tue, Aug 04, 2026 at 09:33:49AM -0700, T.J. Mercier wrote: > > On Tue, Aug 4, 2026 at 3:42 AM Barnabás Pőcze wrote: > > > 2026. 07. 09. 22:04 keltezéssel, T.J. Mercier írta: > > > > On Thu, Jul 9, 2026 at 12:47 PM Laurent Pinchart 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. > > > > > > I have to say, it seems a bit odd to me that one can allocate without write access. > > > > > > >> 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 > > > > > > I'm wondering, why are those heaps made read-only? > > > Couldn't it be the intent that they are not to be used? > > > > No, the system heap is the most widely used heap on Android and there > > is a requirement that all Android devices have one. They were made > > read only because write access isn't required to use them, and the > > security team noticed. The overwhelming majority of all device drivers > > don't require write permissions on the device file used to call their > > ioctl handlers. In the case of dma-buf heaps, ioctl() is really the > > only functional I/O syscall. > > How is R/W a security issue, when a device only exposes ioctls ? It might never be, but the principle of least privilege is about proactively reducing attack surface. Even if there's no known exploit today, removing write access keeps SELinux policies tighter and eliminates unnecessary privileges. > > > In any case, if the upstream consensus is O_RDONLY, then > > > > > > Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > > > > >>> 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
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)
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(-)