[libcamera-devel] [PATCH] v4l2: Accept read-only buffers mappings and require MAP_SHARED

[paul.elder at ideasonboard.com]

Hi Laurent,

On Sun, Jan 30, 2022 at 03:19:41AM +0200, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Sat, Jan 29, 2022 at 08:42:56PM +0000, Kieran Bingham wrote:
> > Quoting Laurent Pinchart (2022-01-28 22:27:47)
> > > Hi Nejc,
> > > 
> > > On Fri, Jan 28, 2022 at 11:18:01PM +0100, Nejc Galof wrote:
> > > > Tested-by: Nejc <galof.nejc at gmail.com>
> > > 
> > > I'll update this to
> > > 
> > > Tested-by: Nejc Galof <galof.nejc at gmail.com>
> > > 
> > > and add
> > > 
> > > Reported-by: Nejc Galof <galof.nejc at gmail.com>
> > > 
> > > Thank you for your first contribution to libcamera :-)
> > > 
> > > > V V pet., 28. jan. 2022 ob 23:02 je oseba Nejc Galof napisala:
> > > > 
> > > > > Hello.
> > > > > Tomorrow I will test and send you an answer.
> > > > >
> > > > > Thank you
> > > > > Nejc Galof
> > > > >
> > > > > V V pet., 28. jan. 2022 ob 23:00 je oseba Laurent Pinchart napisala:
> > > > >
> > > > >> V4L2 is happy to map buffers read-only for capture devices (but rejects
> > > > >> write-only mappings). We can support this as the dmabuf mmap()
> > > > >> implementation supports it. This change fixes usage of the V4L2
> > > > >> compatibility layer with OpenCV.
> > > > >>
> > > > >> While at it, attempt to validate the other flags. videobuf2 requires
> > > > >> MAP_SHARED and doesn't check other flags, so mimic the same behaviour.
> > > > >> While unlikly, other flags could get rejected by other kernel layers for
> > > > >> V4L2 buffers but not for dmabuf. This can be handled later if the need
> > > > >> arises.
> > > > >>
> > > > >> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > >> ---
> > > > >>  src/v4l2/v4l2_camera_proxy.cpp | 13 +++++++++++--
> > > > >>  1 file changed, 11 insertions(+), 2 deletions(-)
> > > > >>
> > > > >> Nejc, could you please test this patch ? It's slightly different than
> > > > >> the one I've shared on the IRC channel. If you don't mind appearing in
> > > > >> the git development history, you can reply with
> > > > >>
> > > > >> Tested-by: Name <email at address>
> > > > >>
> > > > >> and I'll add that (as well as a corresponding Reported-by tag) to the
> > > > >> commit before pushing it.
> > > > >>
> > > > >> diff --git a/src/v4l2/v4l2_camera_proxy.cpp
> > > > >> b/src/v4l2/v4l2_camera_proxy.cpp
> > > > >> index f3470a6d312a..ebe7601a4ba6 100644
> > > > >> --- a/src/v4l2/v4l2_camera_proxy.cpp
> > > > >> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> > > > >> @@ -104,8 +104,17 @@ void *V4L2CameraProxy::mmap(V4L2CameraFile *file,
> > > > >> void *addr, size_t length,
> > > > >>
> > > > >>         MutexLocker locker(proxyMutex_);
> > > > >>
> > > > >> -       /* \todo Validate prot and flags properly. */
> > > > >> -       if (prot != (PROT_READ | PROT_WRITE)) {
> > > > >> +       /*
> > > > >> +        * Mimic the videobuf2 behaviour, which requires PROT_READ. Reject
> > > > >> +        * PROT_EXEC as it makes no sense.
> > > > >> +        */
> > > > >> +       if (!(prot & PROT_READ) || prot & PROT_EXEC) {
> > 
> > Looking through
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/common/videobuf2/videobuf2-core.c#n2260
> > 
> > which I expect is the equivalent that this code emulates, I see the
> > MAP_SHARED requirement, but there is no reference to PROT_EXEC that I
> > can see.
> 
> Correct.
> 
> > While it may not make sense supporting executable code in a capture
> > buffer, does it add any specific benefit to restricting it here? or does
> > it cause the potential to cause an app that mapped with PROT_READ |
> > PROT_EXEC that would work with the kernel, but fail with our V4L2 layer?
> > (if there was perhaps some legitimate reason to capture into an
> > executable buffer which I doubt).
> > 
> > Maybe if this restriction really should be added - it should also be
> > added to the kernel too - but aside from that, I think it might help to
> > at least print some reasons why we are failing the call explicitly.
> 
> I'll drop the check for now, as there's no similar check being performed
> on the kernel side, even if I fail to find any valid use case.

With the check dropped,

Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>

> 
> > Ideally with some debug prints of the cause of the failure:
> > 
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > 
> > > > >> +               errno = EINVAL;
> > > > >> +               return MAP_FAILED;
> > > > >> +       }
> > > > >> +
> > > > >> +       /* videobuf2 also requires MAP_SHARED. */
> > > > >> +       if (!(flags & MAP_SHARED)) {
> > > > >>                 errno = EINVAL;
> > > > >>                 return MAP_FAILED;
> > > > >>         }
> > > > >>
> > > > >> base-commit: 7ea52d2b586144fdc033a3ffc1c4a4bbb99b5440