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

[Kieran Bingham]

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 <galof.nejc at gmail.com>
> > 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 <
> > > laurent.pinchart at ideasonboard.com> 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.

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.

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
> 
> -- 
> Regards,
> 
> Laurent Pinchart