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