| Message ID | 20200616131244.70308-3-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, Thank you for the patch. On Tue, Jun 16, 2020 at 10:12:31PM +0900, Paul Elder wrote: > The ioctl handlers currently don't check if arg is null, so if it ever > is, it will cause a segfault. Check that arg is null and return -EFAULT > in all vidioc ioctl handlers. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> Can't you check that in V4L2CameraProxy::ioctl() ? > --- > src/v4l2/v4l2_camera_proxy.cpp | 33 +++++++++++++++++++++++++++++++-- > 1 file changed, 31 insertions(+), 2 deletions(-) > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp > index 594dd13..5b74b53 100644 > --- a/src/v4l2/v4l2_camera_proxy.cpp > +++ b/src/v4l2/v4l2_camera_proxy.cpp > @@ -238,6 +238,9 @@ int V4L2CameraProxy::vidioc_querycap(struct v4l2_capability *arg) > { > LOG(V4L2Compat, Debug) << "Servicing vidioc_querycap"; > > + if (arg == nullptr) > + return -EFAULT; > + > *arg = capabilities_; > > return 0; > @@ -247,6 +250,8 @@ int V4L2CameraProxy::vidioc_enum_fmt(int fd, struct v4l2_fmtdesc *arg) > { > LOG(V4L2Compat, Debug) << "Servicing vidioc_enum_fmt fd = " << fd; > > + if (arg == nullptr) > + return -EFAULT; > > if (!validateBufferType(arg->type) || > arg->index >= streamConfig_.formats().pixelformats().size()) > @@ -264,6 +269,8 @@ int V4L2CameraProxy::vidioc_g_fmt(int fd, struct v4l2_format *arg) > { > LOG(V4L2Compat, Debug) << "Servicing vidioc_g_fmt fd = " << fd; > > + if (arg == nullptr) > + return -EFAULT; > > if (!validateBufferType(arg->type)) > return -EINVAL; > @@ -303,6 +310,9 @@ int V4L2CameraProxy::vidioc_s_fmt(int fd, struct v4l2_format *arg) > { > LOG(V4L2Compat, Debug) << "Servicing vidioc_s_fmt fd = " << fd; > > + if (arg == nullptr) > + return -EFAULT; > + > int ret = lock(fd); > if (ret < 0) > return ret; > @@ -334,6 +344,9 @@ int V4L2CameraProxy::vidioc_try_fmt(int fd, struct v4l2_format *arg) > { > LOG(V4L2Compat, Debug) << "Servicing vidioc_try_fmt fd = " << fd; > > + if (arg == nullptr) > + return -EFAULT; > + > if (!validateBufferType(arg->type)) > return -EINVAL; > > @@ -361,6 +374,8 @@ int V4L2CameraProxy::vidioc_reqbufs(int fd, struct v4l2_requestbuffers *arg) > { > LOG(V4L2Compat, Debug) << "Servicing vidioc_reqbufs fd = " << fd; > > + if (arg == nullptr) > + return -EFAULT; > > int ret = lock(fd); > if (ret < 0) > @@ -444,6 +459,9 @@ int V4L2CameraProxy::vidioc_querybuf(int fd, struct v4l2_buffer *arg) > { > LOG(V4L2Compat, Debug) << "Servicing vidioc_querybuf fd = " << fd; > > + if (arg == nullptr) > + return -EFAULT; > + > int ret = lock(fd); > if (ret < 0) > return ret; > @@ -461,8 +479,10 @@ int V4L2CameraProxy::vidioc_querybuf(int fd, struct v4l2_buffer *arg) > > int V4L2CameraProxy::vidioc_qbuf(int fd, struct v4l2_buffer *arg) > { > - LOG(V4L2Compat, Debug) << "Servicing vidioc_qbuf, index = " > - << arg->index << " fd = " << fd; > + LOG(V4L2Compat, Debug) << "Servicing vidioc_qbuf fd = " << fd; > + > + if (arg == nullptr) > + return -EFAULT; > > int ret = lock(fd); > if (ret < 0) > @@ -487,6 +507,9 @@ int V4L2CameraProxy::vidioc_dqbuf(int fd, struct v4l2_buffer *arg) > { > LOG(V4L2Compat, Debug) << "Servicing vidioc_dqbuf fd = " << fd; > > + if (arg == nullptr) > + return -EFAULT; > + > int ret = lock(fd); > if (ret < 0) > return ret; > @@ -522,6 +545,9 @@ int V4L2CameraProxy::vidioc_streamon(int fd, int *arg) > { > LOG(V4L2Compat, Debug) << "Servicing vidioc_streamon fd = " << fd; > > + if (arg == nullptr) > + return -EFAULT; > + > int ret = lock(fd); > if (ret < 0) > return ret; > @@ -538,6 +564,9 @@ int V4L2CameraProxy::vidioc_streamoff(int fd, int *arg) > { > LOG(V4L2Compat, Debug) << "Servicing vidioc_streamoff fd = " << fd; > > + if (arg == nullptr) > + return -EFAULT; > + > int ret = lock(fd); > if (ret < 0) > return ret;
On Wed, Jun 17, 2020 at 04:06:07AM +0300, Laurent Pinchart wrote: > Hi Paul, > > Thank you for the patch. > > On Tue, Jun 16, 2020 at 10:12:31PM +0900, Paul Elder wrote: > > The ioctl handlers currently don't check if arg is null, so if it ever > > is, it will cause a segfault. Check that arg is null and return -EFAULT > > in all vidioc ioctl handlers. > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > Can't you check that in V4L2CameraProxy::ioctl() ? We can't do that. v4l2-compliance requires that 1) querycap shold not return EFAULT when arg == NULL, and 2) invalid ioctl should return ENOTTY. Because of the second point, we would still have to check at every ioctl. Paul > > --- > > src/v4l2/v4l2_camera_proxy.cpp | 33 +++++++++++++++++++++++++++++++-- > > 1 file changed, 31 insertions(+), 2 deletions(-) > > > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp > > index 594dd13..5b74b53 100644 > > --- a/src/v4l2/v4l2_camera_proxy.cpp > > +++ b/src/v4l2/v4l2_camera_proxy.cpp > > @@ -238,6 +238,9 @@ int V4L2CameraProxy::vidioc_querycap(struct v4l2_capability *arg) > > { > > LOG(V4L2Compat, Debug) << "Servicing vidioc_querycap"; > > > > + if (arg == nullptr) > > + return -EFAULT; > > + > > *arg = capabilities_; > > > > return 0; > > @@ -247,6 +250,8 @@ int V4L2CameraProxy::vidioc_enum_fmt(int fd, struct v4l2_fmtdesc *arg) > > { > > LOG(V4L2Compat, Debug) << "Servicing vidioc_enum_fmt fd = " << fd; > > > > + if (arg == nullptr) > > + return -EFAULT; > > > > if (!validateBufferType(arg->type) || > > arg->index >= streamConfig_.formats().pixelformats().size()) > > @@ -264,6 +269,8 @@ int V4L2CameraProxy::vidioc_g_fmt(int fd, struct v4l2_format *arg) > > { > > LOG(V4L2Compat, Debug) << "Servicing vidioc_g_fmt fd = " << fd; > > > > + if (arg == nullptr) > > + return -EFAULT; > > > > if (!validateBufferType(arg->type)) > > return -EINVAL; > > @@ -303,6 +310,9 @@ int V4L2CameraProxy::vidioc_s_fmt(int fd, struct v4l2_format *arg) > > { > > LOG(V4L2Compat, Debug) << "Servicing vidioc_s_fmt fd = " << fd; > > > > + if (arg == nullptr) > > + return -EFAULT; > > + > > int ret = lock(fd); > > if (ret < 0) > > return ret; > > @@ -334,6 +344,9 @@ int V4L2CameraProxy::vidioc_try_fmt(int fd, struct v4l2_format *arg) > > { > > LOG(V4L2Compat, Debug) << "Servicing vidioc_try_fmt fd = " << fd; > > > > + if (arg == nullptr) > > + return -EFAULT; > > + > > if (!validateBufferType(arg->type)) > > return -EINVAL; > > > > @@ -361,6 +374,8 @@ int V4L2CameraProxy::vidioc_reqbufs(int fd, struct v4l2_requestbuffers *arg) > > { > > LOG(V4L2Compat, Debug) << "Servicing vidioc_reqbufs fd = " << fd; > > > > + if (arg == nullptr) > > + return -EFAULT; > > > > int ret = lock(fd); > > if (ret < 0) > > @@ -444,6 +459,9 @@ int V4L2CameraProxy::vidioc_querybuf(int fd, struct v4l2_buffer *arg) > > { > > LOG(V4L2Compat, Debug) << "Servicing vidioc_querybuf fd = " << fd; > > > > + if (arg == nullptr) > > + return -EFAULT; > > + > > int ret = lock(fd); > > if (ret < 0) > > return ret; > > @@ -461,8 +479,10 @@ int V4L2CameraProxy::vidioc_querybuf(int fd, struct v4l2_buffer *arg) > > > > int V4L2CameraProxy::vidioc_qbuf(int fd, struct v4l2_buffer *arg) > > { > > - LOG(V4L2Compat, Debug) << "Servicing vidioc_qbuf, index = " > > - << arg->index << " fd = " << fd; > > + LOG(V4L2Compat, Debug) << "Servicing vidioc_qbuf fd = " << fd; > > + > > + if (arg == nullptr) > > + return -EFAULT; > > > > int ret = lock(fd); > > if (ret < 0) > > @@ -487,6 +507,9 @@ int V4L2CameraProxy::vidioc_dqbuf(int fd, struct v4l2_buffer *arg) > > { > > LOG(V4L2Compat, Debug) << "Servicing vidioc_dqbuf fd = " << fd; > > > > + if (arg == nullptr) > > + return -EFAULT; > > + > > int ret = lock(fd); > > if (ret < 0) > > return ret; > > @@ -522,6 +545,9 @@ int V4L2CameraProxy::vidioc_streamon(int fd, int *arg) > > { > > LOG(V4L2Compat, Debug) << "Servicing vidioc_streamon fd = " << fd; > > > > + if (arg == nullptr) > > + return -EFAULT; > > + > > int ret = lock(fd); > > if (ret < 0) > > return ret; > > @@ -538,6 +564,9 @@ int V4L2CameraProxy::vidioc_streamoff(int fd, int *arg) > > { > > LOG(V4L2Compat, Debug) << "Servicing vidioc_streamoff fd = " << fd; > > > > + if (arg == nullptr) > > + return -EFAULT; > > + > > int ret = lock(fd); > > if (ret < 0) > > return ret; > > -- > Regards, > > Laurent Pinchart
Hi Paul, Laurent On Wed, Jun 17, 2020 at 04:06:07AM +0300, Laurent Pinchart wrote: > Hi Paul, > > Thank you for the patch. > > On Tue, Jun 16, 2020 at 10:12:31PM +0900, Paul Elder wrote: > > The ioctl handlers currently don't check if arg is null, so if it ever > > is, it will cause a segfault. Check that arg is null and return -EFAULT > > in all vidioc ioctl handlers. > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > Can't you check that in V4L2CameraProxy::ioctl() ? > And maybe use !arg for the check ? Thanks j > > --- > > src/v4l2/v4l2_camera_proxy.cpp | 33 +++++++++++++++++++++++++++++++-- > > 1 file changed, 31 insertions(+), 2 deletions(-) > > > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp > > index 594dd13..5b74b53 100644 > > --- a/src/v4l2/v4l2_camera_proxy.cpp > > +++ b/src/v4l2/v4l2_camera_proxy.cpp > > @@ -238,6 +238,9 @@ int V4L2CameraProxy::vidioc_querycap(struct v4l2_capability *arg) > > { > > LOG(V4L2Compat, Debug) << "Servicing vidioc_querycap"; > > > > + if (arg == nullptr) > > + return -EFAULT; > > + > > *arg = capabilities_; > > > > return 0; > > @@ -247,6 +250,8 @@ int V4L2CameraProxy::vidioc_enum_fmt(int fd, struct v4l2_fmtdesc *arg) > > { > > LOG(V4L2Compat, Debug) << "Servicing vidioc_enum_fmt fd = " << fd; > > > > + if (arg == nullptr) > > + return -EFAULT; > > > > if (!validateBufferType(arg->type) || > > arg->index >= streamConfig_.formats().pixelformats().size()) > > @@ -264,6 +269,8 @@ int V4L2CameraProxy::vidioc_g_fmt(int fd, struct v4l2_format *arg) > > { > > LOG(V4L2Compat, Debug) << "Servicing vidioc_g_fmt fd = " << fd; > > > > + if (arg == nullptr) > > + return -EFAULT; > > > > if (!validateBufferType(arg->type)) > > return -EINVAL; > > @@ -303,6 +310,9 @@ int V4L2CameraProxy::vidioc_s_fmt(int fd, struct v4l2_format *arg) > > { > > LOG(V4L2Compat, Debug) << "Servicing vidioc_s_fmt fd = " << fd; > > > > + if (arg == nullptr) > > + return -EFAULT; > > + > > int ret = lock(fd); > > if (ret < 0) > > return ret; > > @@ -334,6 +344,9 @@ int V4L2CameraProxy::vidioc_try_fmt(int fd, struct v4l2_format *arg) > > { > > LOG(V4L2Compat, Debug) << "Servicing vidioc_try_fmt fd = " << fd; > > > > + if (arg == nullptr) > > + return -EFAULT; > > + > > if (!validateBufferType(arg->type)) > > return -EINVAL; > > > > @@ -361,6 +374,8 @@ int V4L2CameraProxy::vidioc_reqbufs(int fd, struct v4l2_requestbuffers *arg) > > { > > LOG(V4L2Compat, Debug) << "Servicing vidioc_reqbufs fd = " << fd; > > > > + if (arg == nullptr) > > + return -EFAULT; > > > > int ret = lock(fd); > > if (ret < 0) > > @@ -444,6 +459,9 @@ int V4L2CameraProxy::vidioc_querybuf(int fd, struct v4l2_buffer *arg) > > { > > LOG(V4L2Compat, Debug) << "Servicing vidioc_querybuf fd = " << fd; > > > > + if (arg == nullptr) > > + return -EFAULT; > > + > > int ret = lock(fd); > > if (ret < 0) > > return ret; > > @@ -461,8 +479,10 @@ int V4L2CameraProxy::vidioc_querybuf(int fd, struct v4l2_buffer *arg) > > > > int V4L2CameraProxy::vidioc_qbuf(int fd, struct v4l2_buffer *arg) > > { > > - LOG(V4L2Compat, Debug) << "Servicing vidioc_qbuf, index = " > > - << arg->index << " fd = " << fd; > > + LOG(V4L2Compat, Debug) << "Servicing vidioc_qbuf fd = " << fd; > > + > > + if (arg == nullptr) > > + return -EFAULT; > > > > int ret = lock(fd); > > if (ret < 0) > > @@ -487,6 +507,9 @@ int V4L2CameraProxy::vidioc_dqbuf(int fd, struct v4l2_buffer *arg) > > { > > LOG(V4L2Compat, Debug) << "Servicing vidioc_dqbuf fd = " << fd; > > > > + if (arg == nullptr) > > + return -EFAULT; > > + > > int ret = lock(fd); > > if (ret < 0) > > return ret; > > @@ -522,6 +545,9 @@ int V4L2CameraProxy::vidioc_streamon(int fd, int *arg) > > { > > LOG(V4L2Compat, Debug) << "Servicing vidioc_streamon fd = " << fd; > > > > + if (arg == nullptr) > > + return -EFAULT; > > + > > int ret = lock(fd); > > if (ret < 0) > > return ret; > > @@ -538,6 +564,9 @@ int V4L2CameraProxy::vidioc_streamoff(int fd, int *arg) > > { > > LOG(V4L2Compat, Debug) << "Servicing vidioc_streamoff fd = " << fd; > > > > + if (arg == nullptr) > > + return -EFAULT; > > + > > int ret = lock(fd); > > if (ret < 0) > > return ret; > > -- > Regards, > > Laurent Pinchart > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Paul, On Wed, Jun 17, 2020 at 06:30:31PM +0900, Paul Elder wrote: > On Wed, Jun 17, 2020 at 04:06:07AM +0300, Laurent Pinchart wrote: > > Hi Paul, > > > > Thank you for the patch. > > > > On Tue, Jun 16, 2020 at 10:12:31PM +0900, Paul Elder wrote: > > > The ioctl handlers currently don't check if arg is null, so if it ever > > > is, it will cause a segfault. Check that arg is null and return -EFAULT > > > in all vidioc ioctl handlers. > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > Can't you check that in V4L2CameraProxy::ioctl() ? > > We can't do that. v4l2-compliance requires that 1) querycap shold not > return EFAULT when arg == NULL, and 2) invalid ioctl should return @@ -238,6 +238,9 @@ int V4L2CameraProxy::vidioc_querycap(struct v4l2_capability *arg) { LOG(V4L2Compat, Debug) << "Servicing vidioc_querycap"; + if (arg == nullptr) + return -EFAULT; + Doesn't this return EFAUL on querycap with !arg ? > ENOTTY. Because of the second point, we would still have to check at > every ioctl. You could work it around with a bool is_ioctl_valid(unsigned long request) { switch (request) { case VIDIOC_QUERYCAP: case VIDIOC_G_FMT: .... return true; default: return false; } } If you happen to have to check for an ioctl to be supported in multiple places, otherwise I'm not sure it's worth it. > > > Paul > > > > > --- > > > src/v4l2/v4l2_camera_proxy.cpp | 33 +++++++++++++++++++++++++++++++-- > > > 1 file changed, 31 insertions(+), 2 deletions(-) > > > > > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp > > > index 594dd13..5b74b53 100644 > > > --- a/src/v4l2/v4l2_camera_proxy.cpp > > > +++ b/src/v4l2/v4l2_camera_proxy.cpp > > > @@ -238,6 +238,9 @@ int V4L2CameraProxy::vidioc_querycap(struct v4l2_capability *arg) > > > { > > > LOG(V4L2Compat, Debug) << "Servicing vidioc_querycap"; > > > > > > + if (arg == nullptr) > > > + return -EFAULT; > > > + > > > *arg = capabilities_; > > > > > > return 0; > > > @@ -247,6 +250,8 @@ int V4L2CameraProxy::vidioc_enum_fmt(int fd, struct v4l2_fmtdesc *arg) > > > { > > > LOG(V4L2Compat, Debug) << "Servicing vidioc_enum_fmt fd = " << fd; > > > > > > + if (arg == nullptr) > > > + return -EFAULT; > > > > > > if (!validateBufferType(arg->type) || > > > arg->index >= streamConfig_.formats().pixelformats().size()) > > > @@ -264,6 +269,8 @@ int V4L2CameraProxy::vidioc_g_fmt(int fd, struct v4l2_format *arg) > > > { > > > LOG(V4L2Compat, Debug) << "Servicing vidioc_g_fmt fd = " << fd; > > > > > > + if (arg == nullptr) > > > + return -EFAULT; > > > > > > if (!validateBufferType(arg->type)) > > > return -EINVAL; > > > @@ -303,6 +310,9 @@ int V4L2CameraProxy::vidioc_s_fmt(int fd, struct v4l2_format *arg) > > > { > > > LOG(V4L2Compat, Debug) << "Servicing vidioc_s_fmt fd = " << fd; > > > > > > + if (arg == nullptr) > > > + return -EFAULT; > > > + > > > int ret = lock(fd); > > > if (ret < 0) > > > return ret; > > > @@ -334,6 +344,9 @@ int V4L2CameraProxy::vidioc_try_fmt(int fd, struct v4l2_format *arg) > > > { > > > LOG(V4L2Compat, Debug) << "Servicing vidioc_try_fmt fd = " << fd; > > > > > > + if (arg == nullptr) > > > + return -EFAULT; > > > + > > > if (!validateBufferType(arg->type)) > > > return -EINVAL; > > > > > > @@ -361,6 +374,8 @@ int V4L2CameraProxy::vidioc_reqbufs(int fd, struct v4l2_requestbuffers *arg) > > > { > > > LOG(V4L2Compat, Debug) << "Servicing vidioc_reqbufs fd = " << fd; > > > > > > + if (arg == nullptr) > > > + return -EFAULT; > > > > > > int ret = lock(fd); > > > if (ret < 0) > > > @@ -444,6 +459,9 @@ int V4L2CameraProxy::vidioc_querybuf(int fd, struct v4l2_buffer *arg) > > > { > > > LOG(V4L2Compat, Debug) << "Servicing vidioc_querybuf fd = " << fd; > > > > > > + if (arg == nullptr) > > > + return -EFAULT; > > > + > > > int ret = lock(fd); > > > if (ret < 0) > > > return ret; > > > @@ -461,8 +479,10 @@ int V4L2CameraProxy::vidioc_querybuf(int fd, struct v4l2_buffer *arg) > > > > > > int V4L2CameraProxy::vidioc_qbuf(int fd, struct v4l2_buffer *arg) > > > { > > > - LOG(V4L2Compat, Debug) << "Servicing vidioc_qbuf, index = " > > > - << arg->index << " fd = " << fd; > > > + LOG(V4L2Compat, Debug) << "Servicing vidioc_qbuf fd = " << fd; > > > + > > > + if (arg == nullptr) > > > + return -EFAULT; > > > > > > int ret = lock(fd); > > > if (ret < 0) > > > @@ -487,6 +507,9 @@ int V4L2CameraProxy::vidioc_dqbuf(int fd, struct v4l2_buffer *arg) > > > { > > > LOG(V4L2Compat, Debug) << "Servicing vidioc_dqbuf fd = " << fd; > > > > > > + if (arg == nullptr) > > > + return -EFAULT; > > > + > > > int ret = lock(fd); > > > if (ret < 0) > > > return ret; > > > @@ -522,6 +545,9 @@ int V4L2CameraProxy::vidioc_streamon(int fd, int *arg) > > > { > > > LOG(V4L2Compat, Debug) << "Servicing vidioc_streamon fd = " << fd; > > > > > > + if (arg == nullptr) > > > + return -EFAULT; > > > + > > > int ret = lock(fd); > > > if (ret < 0) > > > return ret; > > > @@ -538,6 +564,9 @@ int V4L2CameraProxy::vidioc_streamoff(int fd, int *arg) > > > { > > > LOG(V4L2Compat, Debug) << "Servicing vidioc_streamoff fd = " << fd; > > > > > > + if (arg == nullptr) > > > + return -EFAULT; > > > + > > > int ret = lock(fd); > > > if (ret < 0) > > > return ret; > > > > -- > > Regards, > > > > Laurent Pinchart > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Paul, On Wed, Jun 17, 2020 at 06:30:31PM +0900, Paul Elder wrote: > On Wed, Jun 17, 2020 at 04:06:07AM +0300, Laurent Pinchart wrote: > > On Tue, Jun 16, 2020 at 10:12:31PM +0900, Paul Elder wrote: > > > The ioctl handlers currently don't check if arg is null, so if it ever > > > is, it will cause a segfault. Check that arg is null and return -EFAULT > > > in all vidioc ioctl handlers. > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > Can't you check that in V4L2CameraProxy::ioctl() ? > > We can't do that. v4l2-compliance requires that 1) querycap shold not > return EFAULT when arg == NULL, Really ? v4l2-compliance has fail_on_test(doioctl(node, VIDIOC_QUERYCAP, NULL) != EFAULT); Doesn't it mean it expects EFAULT when arg == NULL ? > and 2) invalid ioctl should return > ENOTTY. Because of the second point, we would still have to check at > every ioctl. Indeed, that's an annoying one :-( We could have a static const std::set of supported ioctls (or a switch/case as Jacopo proposed), and use that for the initial checks. That would require keeping the set in sync with the switch/case that dispatches the ioctls, but that would be less error-prone than requiring a manual null check in every handler. std::set<unsigned long> V4L2CameraProxy::supportedIoctls_ = { ... }; ... if (supportedIoctls_.find(request) == supportedIoctls_.end()) return -ENOTTY; if (!arg) return -EFAULT; Now that I've written this, I've looked at the kernel code and at v4l2-compliance. The latter has static int testInvalidIoctls(struct node *node, char type) { unsigned ioc = _IOC(_IOC_NONE, type, 0xff, 0); unsigned char buf[0x4000] = {}; fail_on_test(doioctl(node, ioc, NULL) != ENOTTY); ioc = _IOC(_IOC_NONE, type, 0, 0x3fff); fail_on_test(doioctl(node, ioc, NULL) != ENOTTY); ioc = _IOC(_IOC_READ, type, 0, 0x3fff); fail_on_test(doioctl(node, ioc, buf) != ENOTTY); fail_on_test(check_0(buf, sizeof(buf))); ioc = _IOC(_IOC_WRITE, type, 0, 0x3fff); fail_on_test(doioctl(node, ioc, buf) != ENOTTY); fail_on_test(check_0(buf, sizeof(buf))); ioc = _IOC(_IOC_READ | _IOC_WRITE, type, 0, 0x3fff); fail_on_test(doioctl(node, ioc, buf) != ENOTTY); fail_on_test(check_0(buf, sizeof(buf))); return 0; } Note how the _IOC_WRITE ioctls are not tested with a NULL argument. Those would return -EFAULT. How about the following code ? if (!arg && _IOC_DIR(request) & _IOC_WRITE) return -EFAULT; if (supportedIoctls_.find(request) == supportedIoctls_.end()) return -ENOTTY; if (!arg && _IOC_DIR(request) & _IOC_READ) return -EFAULT; The only ioctl that has neither _IOC_READ nor _IOC_WRITE set is VIDIOC_LOG_STATUS. We don't support it, so the last check could technically be if (!arg) return -EFAULT; Up to you. > > > --- > > > src/v4l2/v4l2_camera_proxy.cpp | 33 +++++++++++++++++++++++++++++++-- > > > 1 file changed, 31 insertions(+), 2 deletions(-) > > > > > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp > > > index 594dd13..5b74b53 100644 > > > --- a/src/v4l2/v4l2_camera_proxy.cpp > > > +++ b/src/v4l2/v4l2_camera_proxy.cpp > > > @@ -238,6 +238,9 @@ int V4L2CameraProxy::vidioc_querycap(struct v4l2_capability *arg) > > > { > > > LOG(V4L2Compat, Debug) << "Servicing vidioc_querycap"; > > > > > > + if (arg == nullptr) > > > + return -EFAULT; > > > + > > > *arg = capabilities_; > > > > > > return 0; > > > @@ -247,6 +250,8 @@ int V4L2CameraProxy::vidioc_enum_fmt(int fd, struct v4l2_fmtdesc *arg) > > > { > > > LOG(V4L2Compat, Debug) << "Servicing vidioc_enum_fmt fd = " << fd; > > > > > > + if (arg == nullptr) > > > + return -EFAULT; > > > > > > if (!validateBufferType(arg->type) || > > > arg->index >= streamConfig_.formats().pixelformats().size()) > > > @@ -264,6 +269,8 @@ int V4L2CameraProxy::vidioc_g_fmt(int fd, struct v4l2_format *arg) > > > { > > > LOG(V4L2Compat, Debug) << "Servicing vidioc_g_fmt fd = " << fd; > > > > > > + if (arg == nullptr) > > > + return -EFAULT; > > > > > > if (!validateBufferType(arg->type)) > > > return -EINVAL; > > > @@ -303,6 +310,9 @@ int V4L2CameraProxy::vidioc_s_fmt(int fd, struct v4l2_format *arg) > > > { > > > LOG(V4L2Compat, Debug) << "Servicing vidioc_s_fmt fd = " << fd; > > > > > > + if (arg == nullptr) > > > + return -EFAULT; > > > + > > > int ret = lock(fd); > > > if (ret < 0) > > > return ret; > > > @@ -334,6 +344,9 @@ int V4L2CameraProxy::vidioc_try_fmt(int fd, struct v4l2_format *arg) > > > { > > > LOG(V4L2Compat, Debug) << "Servicing vidioc_try_fmt fd = " << fd; > > > > > > + if (arg == nullptr) > > > + return -EFAULT; > > > + > > > if (!validateBufferType(arg->type)) > > > return -EINVAL; > > > > > > @@ -361,6 +374,8 @@ int V4L2CameraProxy::vidioc_reqbufs(int fd, struct v4l2_requestbuffers *arg) > > > { > > > LOG(V4L2Compat, Debug) << "Servicing vidioc_reqbufs fd = " << fd; > > > > > > + if (arg == nullptr) > > > + return -EFAULT; > > > > > > int ret = lock(fd); > > > if (ret < 0) > > > @@ -444,6 +459,9 @@ int V4L2CameraProxy::vidioc_querybuf(int fd, struct v4l2_buffer *arg) > > > { > > > LOG(V4L2Compat, Debug) << "Servicing vidioc_querybuf fd = " << fd; > > > > > > + if (arg == nullptr) > > > + return -EFAULT; > > > + > > > int ret = lock(fd); > > > if (ret < 0) > > > return ret; > > > @@ -461,8 +479,10 @@ int V4L2CameraProxy::vidioc_querybuf(int fd, struct v4l2_buffer *arg) > > > > > > int V4L2CameraProxy::vidioc_qbuf(int fd, struct v4l2_buffer *arg) > > > { > > > - LOG(V4L2Compat, Debug) << "Servicing vidioc_qbuf, index = " > > > - << arg->index << " fd = " << fd; > > > + LOG(V4L2Compat, Debug) << "Servicing vidioc_qbuf fd = " << fd; > > > + > > > + if (arg == nullptr) > > > + return -EFAULT; > > > > > > int ret = lock(fd); > > > if (ret < 0) > > > @@ -487,6 +507,9 @@ int V4L2CameraProxy::vidioc_dqbuf(int fd, struct v4l2_buffer *arg) > > > { > > > LOG(V4L2Compat, Debug) << "Servicing vidioc_dqbuf fd = " << fd; > > > > > > + if (arg == nullptr) > > > + return -EFAULT; > > > + > > > int ret = lock(fd); > > > if (ret < 0) > > > return ret; > > > @@ -522,6 +545,9 @@ int V4L2CameraProxy::vidioc_streamon(int fd, int *arg) > > > { > > > LOG(V4L2Compat, Debug) << "Servicing vidioc_streamon fd = " << fd; > > > > > > + if (arg == nullptr) > > > + return -EFAULT; > > > + > > > int ret = lock(fd); > > > if (ret < 0) > > > return ret; > > > @@ -538,6 +564,9 @@ int V4L2CameraProxy::vidioc_streamoff(int fd, int *arg) > > > { > > > LOG(V4L2Compat, Debug) << "Servicing vidioc_streamoff fd = " << fd; > > > > > > + if (arg == nullptr) > > > + return -EFAULT; > > > + > > > int ret = lock(fd); > > > if (ret < 0) > > > return ret;
diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp index 594dd13..5b74b53 100644 --- a/src/v4l2/v4l2_camera_proxy.cpp +++ b/src/v4l2/v4l2_camera_proxy.cpp @@ -238,6 +238,9 @@ int V4L2CameraProxy::vidioc_querycap(struct v4l2_capability *arg) { LOG(V4L2Compat, Debug) << "Servicing vidioc_querycap"; + if (arg == nullptr) + return -EFAULT; + *arg = capabilities_; return 0; @@ -247,6 +250,8 @@ int V4L2CameraProxy::vidioc_enum_fmt(int fd, struct v4l2_fmtdesc *arg) { LOG(V4L2Compat, Debug) << "Servicing vidioc_enum_fmt fd = " << fd; + if (arg == nullptr) + return -EFAULT; if (!validateBufferType(arg->type) || arg->index >= streamConfig_.formats().pixelformats().size()) @@ -264,6 +269,8 @@ int V4L2CameraProxy::vidioc_g_fmt(int fd, struct v4l2_format *arg) { LOG(V4L2Compat, Debug) << "Servicing vidioc_g_fmt fd = " << fd; + if (arg == nullptr) + return -EFAULT; if (!validateBufferType(arg->type)) return -EINVAL; @@ -303,6 +310,9 @@ int V4L2CameraProxy::vidioc_s_fmt(int fd, struct v4l2_format *arg) { LOG(V4L2Compat, Debug) << "Servicing vidioc_s_fmt fd = " << fd; + if (arg == nullptr) + return -EFAULT; + int ret = lock(fd); if (ret < 0) return ret; @@ -334,6 +344,9 @@ int V4L2CameraProxy::vidioc_try_fmt(int fd, struct v4l2_format *arg) { LOG(V4L2Compat, Debug) << "Servicing vidioc_try_fmt fd = " << fd; + if (arg == nullptr) + return -EFAULT; + if (!validateBufferType(arg->type)) return -EINVAL; @@ -361,6 +374,8 @@ int V4L2CameraProxy::vidioc_reqbufs(int fd, struct v4l2_requestbuffers *arg) { LOG(V4L2Compat, Debug) << "Servicing vidioc_reqbufs fd = " << fd; + if (arg == nullptr) + return -EFAULT; int ret = lock(fd); if (ret < 0) @@ -444,6 +459,9 @@ int V4L2CameraProxy::vidioc_querybuf(int fd, struct v4l2_buffer *arg) { LOG(V4L2Compat, Debug) << "Servicing vidioc_querybuf fd = " << fd; + if (arg == nullptr) + return -EFAULT; + int ret = lock(fd); if (ret < 0) return ret; @@ -461,8 +479,10 @@ int V4L2CameraProxy::vidioc_querybuf(int fd, struct v4l2_buffer *arg) int V4L2CameraProxy::vidioc_qbuf(int fd, struct v4l2_buffer *arg) { - LOG(V4L2Compat, Debug) << "Servicing vidioc_qbuf, index = " - << arg->index << " fd = " << fd; + LOG(V4L2Compat, Debug) << "Servicing vidioc_qbuf fd = " << fd; + + if (arg == nullptr) + return -EFAULT; int ret = lock(fd); if (ret < 0) @@ -487,6 +507,9 @@ int V4L2CameraProxy::vidioc_dqbuf(int fd, struct v4l2_buffer *arg) { LOG(V4L2Compat, Debug) << "Servicing vidioc_dqbuf fd = " << fd; + if (arg == nullptr) + return -EFAULT; + int ret = lock(fd); if (ret < 0) return ret; @@ -522,6 +545,9 @@ int V4L2CameraProxy::vidioc_streamon(int fd, int *arg) { LOG(V4L2Compat, Debug) << "Servicing vidioc_streamon fd = " << fd; + if (arg == nullptr) + return -EFAULT; + int ret = lock(fd); if (ret < 0) return ret; @@ -538,6 +564,9 @@ int V4L2CameraProxy::vidioc_streamoff(int fd, int *arg) { LOG(V4L2Compat, Debug) << "Servicing vidioc_streamoff fd = " << fd; + if (arg == nullptr) + return -EFAULT; + int ret = lock(fd); if (ret < 0) return ret;
The ioctl handlers currently don't check if arg is null, so if it ever is, it will cause a segfault. Check that arg is null and return -EFAULT in all vidioc ioctl handlers. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- src/v4l2/v4l2_camera_proxy.cpp | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-)