| Message ID | 20220128220031.3467-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@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@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@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) { > + 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 > >
Tested-by: Nejc <galof.nejc@gmail.com> V V pet., 28. jan. 2022 ob 23:02 je oseba Nejc Galof <galof.nejc@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@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@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@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) { >> + 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 >> >>
Hi Nejc, On Fri, Jan 28, 2022 at 11:18:01PM +0100, Nejc Galof wrote: > Tested-by: Nejc <galof.nejc@gmail.com> I'll update this to Tested-by: Nejc Galof <galof.nejc@gmail.com> and add Reported-by: Nejc Galof <galof.nejc@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@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@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@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@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) { > >> + errno = EINVAL; > >> + return MAP_FAILED; > >> + } > >> + > >> + /* videobuf2 also requires MAP_SHARED. */ > >> + if (!(flags & MAP_SHARED)) { > >> errno = EINVAL; > >> return MAP_FAILED; > >> } > >> > >> base-commit: 7ea52d2b586144fdc033a3ffc1c4a4bbb99b5440
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@gmail.com> > > I'll update this to > > Tested-by: Nejc Galof <galof.nejc@gmail.com> > > and add > > Reported-by: Nejc Galof <galof.nejc@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@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@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@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@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@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
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@gmail.com> > > > > I'll update this to > > > > Tested-by: Nejc Galof <galof.nejc@gmail.com> > > > > and add > > > > Reported-by: Nejc Galof <galof.nejc@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@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@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. > Ideally with some debug prints of the cause of the failure: > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > >> + errno = EINVAL; > > > >> + return MAP_FAILED; > > > >> + } > > > >> + > > > >> + /* videobuf2 also requires MAP_SHARED. */ > > > >> + if (!(flags & MAP_SHARED)) { > > > >> errno = EINVAL; > > > >> return MAP_FAILED; > > > >> } > > > >> > > > >> base-commit: 7ea52d2b586144fdc033a3ffc1c4a4bbb99b5440
Hello. When can I expect this fix in the master branch? Thank you for the answer. Nejc Galof V V ned., 30. jan. 2022 ob 02:20 je oseba Laurent Pinchart < laurent.pinchart@ideasonboard.com> napisala: > 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@gmail.com> > > > > > > I'll update this to > > > > > > Tested-by: Nejc Galof <galof.nejc@gmail.com> > > > > > > and add > > > > > > Reported-by: Nejc Galof <galof.nejc@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@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@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. > > > Ideally with some debug prints of the cause of the failure: > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@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 >
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@gmail.com> > > > > > > I'll update this to > > > > > > Tested-by: Nejc Galof <galof.nejc@gmail.com> > > > > > > and add > > > > > > Reported-by: Nejc Galof <galof.nejc@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@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@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@ideasonboard.com> > > > Ideally with some debug prints of the cause of the failure: > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > >> + errno = EINVAL; > > > > >> + return MAP_FAILED; > > > > >> + } > > > > >> + > > > > >> + /* videobuf2 also requires MAP_SHARED. */ > > > > >> + if (!(flags & MAP_SHARED)) { > > > > >> errno = EINVAL; > > > > >> return MAP_FAILED; > > > > >> } > > > > >> > > > > >> base-commit: 7ea52d2b586144fdc033a3ffc1c4a4bbb99b5440
Hi Nejc, On Tue, Feb 01, 2022 at 10:25:17AM +0100, Nejc Galof wrote: > Hello. > When can I expect this fix in the master branch? I was waiting for Paul to give it a look. It's done now, and I've pushed the fix. > V V ned., 30. jan. 2022 ob 02:20 je oseba Laurent Pinchart napisala: > > On Sat, Jan 29, 2022 at 08:42:56PM +0000, Kieran Bingham wrote: > > > Quoting Laurent Pinchart (2022-01-28 22:27:47) > > > > On Fri, Jan 28, 2022 at 11:18:01PM +0100, Nejc Galof wrote: > > > > > Tested-by: Nejc <galof.nejc@gmail.com> > > > > > > > > I'll update this to > > > > > > > > Tested-by: Nejc Galof <galof.nejc@gmail.com> > > > > > > > > and add > > > > > > > > Reported-by: Nejc Galof <galof.nejc@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@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@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. > > > > > Ideally with some debug prints of the cause of the failure: > > > > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > >> + errno = EINVAL; > > > > > >> + return MAP_FAILED; > > > > > >> + } > > > > > >> + > > > > > >> + /* videobuf2 also requires MAP_SHARED. */ > > > > > >> + if (!(flags & MAP_SHARED)) { > > > > > >> errno = EINVAL; > > > > > >> return MAP_FAILED; > > > > > >> } > > > > > >> > > > > > >> base-commit: 7ea52d2b586144fdc033a3ffc1c4a4bbb99b5440
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) { + errno = EINVAL; + return MAP_FAILED; + } + + /* videobuf2 also requires MAP_SHARED. */ + if (!(flags & MAP_SHARED)) { errno = EINVAL; return MAP_FAILED; }