| Message ID | 20200619054123.19052-4-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, On Fri, Jun 19, 2020 at 02:41:09PM +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 the main vidioc ioctl handler. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > Changes in v2: > - moved !arg check to main ioctl handler, and added a set of supported > ioctls > - use !arg instead of arg == nullptr > --- > src/v4l2/v4l2_camera_proxy.cpp | 27 +++++++++++++++++++++++++-- > src/v4l2/v4l2_camera_proxy.h | 3 +++ > 2 files changed, 28 insertions(+), 2 deletions(-) > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp > index f06f58d..cff6562 100644 > --- a/src/v4l2/v4l2_camera_proxy.cpp > +++ b/src/v4l2/v4l2_camera_proxy.cpp > @@ -11,6 +11,7 @@ > #include <array> > #include <errno.h> > #include <linux/videodev2.h> > +#include <set> > #include <string.h> > #include <sys/mman.h> > #include <unistd.h> > @@ -238,7 +239,6 @@ int V4L2CameraProxy::vidioc_enum_fmt(V4L2CameraFile *cf, struct v4l2_fmtdesc *ar > { > LOG(V4L2Compat, Debug) << "Servicing vidioc_enum_fmt fd = " << cf->efd(); > > - Where does these spurious empty lines come from ? > if (!validateBufferType(arg->type) || > arg->index >= streamConfig_.formats().pixelformats().size()) > return -EINVAL; > @@ -255,7 +255,6 @@ int V4L2CameraProxy::vidioc_g_fmt(V4L2CameraFile *cf, struct v4l2_format *arg) > { > LOG(V4L2Compat, Debug) << "Servicing vidioc_g_fmt fd = " << cf->efd(); > > - > if (!validateBufferType(arg->type)) > return -EINVAL; > > @@ -543,8 +542,32 @@ int V4L2CameraProxy::vidioc_streamoff(V4L2CameraFile *cf, int *arg) > return ret; > } > > +std::set<unsigned long> V4L2CameraProxy::supportedIoctls_ = { > + VIDIOC_QUERYCAP, > + VIDIOC_ENUM_FMT, > + VIDIOC_G_FMT, > + VIDIOC_S_FMT, > + VIDIOC_TRY_FMT, > + VIDIOC_REQBUFS, > + VIDIOC_QUERYBUF, > + VIDIOC_QBUF, > + VIDIOC_DQBUF, > + VIDIOC_STREAMON, > + VIDIOC_STREAMOFF, > +}; I understand why an std::set(), as it provides a ::find() function which ease lookup. I'm wondering if it's worth it though, as a function in an anonymous namespace which simply switch on the supported requests might be more efficient. Anyway, have you considered having this in an anonymous namespace instead of making a static class member out of it ? Is it accessed from any other class (don't think so, as it's a private static class member) > + > int V4L2CameraProxy::ioctl(V4L2CameraFile *cf, unsigned long request, void *arg) > { > + if (supportedIoctls_.find(request) == supportedIoctls_.end()) { > + errno = ENOTTY; > + return -1; > + } > + > + if (!arg) { > + errno = EFAULT; > + return -1; > + } > + > int ret; > switch (request) { > case VIDIOC_QUERYCAP: > diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h > index 43290ca..dd7e793 100644 > --- a/src/v4l2/v4l2_camera_proxy.h > +++ b/src/v4l2/v4l2_camera_proxy.h > @@ -11,6 +11,7 @@ > #include <linux/videodev2.h> > #include <map> > #include <memory> > +#include <set> > #include <sys/mman.h> > #include <sys/types.h> > #include <vector> > @@ -67,6 +68,8 @@ private: > static PixelFormat v4l2ToDrm(uint32_t format); > static uint32_t drmToV4L2(const PixelFormat &format); > > + static std::set<unsigned long> supportedIoctls_; > + > unsigned int refcount_; > unsigned int index_; > > -- > 2.27.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hello, On Fri, Jun 19, 2020 at 12:22:30PM +0200, Jacopo Mondi wrote: > On Fri, Jun 19, 2020 at 02:41:09PM +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 the main vidioc ioctl handler. > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > --- > > Changes in v2: > > - moved !arg check to main ioctl handler, and added a set of supported > > ioctls > > - use !arg instead of arg == nullptr > > --- > > src/v4l2/v4l2_camera_proxy.cpp | 27 +++++++++++++++++++++++++-- > > src/v4l2/v4l2_camera_proxy.h | 3 +++ > > 2 files changed, 28 insertions(+), 2 deletions(-) > > > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp > > index f06f58d..cff6562 100644 > > --- a/src/v4l2/v4l2_camera_proxy.cpp > > +++ b/src/v4l2/v4l2_camera_proxy.cpp > > @@ -11,6 +11,7 @@ > > #include <array> > > #include <errno.h> > > #include <linux/videodev2.h> > > +#include <set> > > #include <string.h> > > #include <sys/mman.h> > > #include <unistd.h> > > @@ -238,7 +239,6 @@ int V4L2CameraProxy::vidioc_enum_fmt(V4L2CameraFile *cf, struct v4l2_fmtdesc *ar > > { > > LOG(V4L2Compat, Debug) << "Servicing vidioc_enum_fmt fd = " << cf->efd(); > > > > - > > Where does these spurious empty lines come from ? I suspect a rebase mistake :-) It should indeed be fixed in the previous patch. > > if (!validateBufferType(arg->type) || > > arg->index >= streamConfig_.formats().pixelformats().size()) > > return -EINVAL; > > @@ -255,7 +255,6 @@ int V4L2CameraProxy::vidioc_g_fmt(V4L2CameraFile *cf, struct v4l2_format *arg) > > { > > LOG(V4L2Compat, Debug) << "Servicing vidioc_g_fmt fd = " << cf->efd(); > > > > - > > if (!validateBufferType(arg->type)) > > return -EINVAL; > > > > @@ -543,8 +542,32 @@ int V4L2CameraProxy::vidioc_streamoff(V4L2CameraFile *cf, int *arg) > > return ret; > > } > > > > +std::set<unsigned long> V4L2CameraProxy::supportedIoctls_ = { This should be const. > > + VIDIOC_QUERYCAP, > > + VIDIOC_ENUM_FMT, > > + VIDIOC_G_FMT, > > + VIDIOC_S_FMT, > > + VIDIOC_TRY_FMT, > > + VIDIOC_REQBUFS, > > + VIDIOC_QUERYBUF, > > + VIDIOC_QBUF, > > + VIDIOC_DQBUF, > > + VIDIOC_STREAMON, > > + VIDIOC_STREAMOFF, > > +}; > > I understand why an std::set(), as it provides a ::find() function > which ease lookup. I'm wondering if it's worth it though, as a > function in an anonymous namespace which simply switch on the > supported requests might be more efficient. Would it ? It's a honest question, I'm not sure how the compiler would optimize that. > Anyway, have you considered having this in an anonymous namespace > instead of making a static class member out of it ? Is it accessed > from any other class (don't think so, as it's a private static class > member) I have a slight preference for a set here. As for storing it in the class or as a global variable in an anonymous namespace, I don't mind much either way, especially as V4L2CameraProxy is an internal class. I'd certainly go for the latter if the class was part of the public API. > > + > > int V4L2CameraProxy::ioctl(V4L2CameraFile *cf, unsigned long request, void *arg) > > { If we want to match V4L2, should we here add if (!arg && (_IOC_DIR(cmd) & _IOC_WRITE)) { errno = EFAULT; return -1; } > > + if (supportedIoctls_.find(request) == supportedIoctls_.end()) { > > + errno = ENOTTY; > > + return -1; > > + } > > + > > + if (!arg) { if (!arg && (_IOC_DIR(cmd) & _IOC_READ)) { With the small issues addressed, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > + errno = EFAULT; > > + return -1; > > + } > > + > > int ret; > > switch (request) { > > case VIDIOC_QUERYCAP: > > diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h > > index 43290ca..dd7e793 100644 > > --- a/src/v4l2/v4l2_camera_proxy.h > > +++ b/src/v4l2/v4l2_camera_proxy.h > > @@ -11,6 +11,7 @@ > > #include <linux/videodev2.h> > > #include <map> > > #include <memory> > > +#include <set> > > #include <sys/mman.h> > > #include <sys/types.h> > > #include <vector> > > @@ -67,6 +68,8 @@ private: > > static PixelFormat v4l2ToDrm(uint32_t format); > > static uint32_t drmToV4L2(const PixelFormat &format); > > > > + static std::set<unsigned long> supportedIoctls_; > > + > > unsigned int refcount_; > > unsigned int index_; > >
diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp index f06f58d..cff6562 100644 --- a/src/v4l2/v4l2_camera_proxy.cpp +++ b/src/v4l2/v4l2_camera_proxy.cpp @@ -11,6 +11,7 @@ #include <array> #include <errno.h> #include <linux/videodev2.h> +#include <set> #include <string.h> #include <sys/mman.h> #include <unistd.h> @@ -238,7 +239,6 @@ int V4L2CameraProxy::vidioc_enum_fmt(V4L2CameraFile *cf, struct v4l2_fmtdesc *ar { LOG(V4L2Compat, Debug) << "Servicing vidioc_enum_fmt fd = " << cf->efd(); - if (!validateBufferType(arg->type) || arg->index >= streamConfig_.formats().pixelformats().size()) return -EINVAL; @@ -255,7 +255,6 @@ int V4L2CameraProxy::vidioc_g_fmt(V4L2CameraFile *cf, struct v4l2_format *arg) { LOG(V4L2Compat, Debug) << "Servicing vidioc_g_fmt fd = " << cf->efd(); - if (!validateBufferType(arg->type)) return -EINVAL; @@ -543,8 +542,32 @@ int V4L2CameraProxy::vidioc_streamoff(V4L2CameraFile *cf, int *arg) return ret; } +std::set<unsigned long> V4L2CameraProxy::supportedIoctls_ = { + VIDIOC_QUERYCAP, + VIDIOC_ENUM_FMT, + VIDIOC_G_FMT, + VIDIOC_S_FMT, + VIDIOC_TRY_FMT, + VIDIOC_REQBUFS, + VIDIOC_QUERYBUF, + VIDIOC_QBUF, + VIDIOC_DQBUF, + VIDIOC_STREAMON, + VIDIOC_STREAMOFF, +}; + int V4L2CameraProxy::ioctl(V4L2CameraFile *cf, unsigned long request, void *arg) { + if (supportedIoctls_.find(request) == supportedIoctls_.end()) { + errno = ENOTTY; + return -1; + } + + if (!arg) { + errno = EFAULT; + return -1; + } + int ret; switch (request) { case VIDIOC_QUERYCAP: diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h index 43290ca..dd7e793 100644 --- a/src/v4l2/v4l2_camera_proxy.h +++ b/src/v4l2/v4l2_camera_proxy.h @@ -11,6 +11,7 @@ #include <linux/videodev2.h> #include <map> #include <memory> +#include <set> #include <sys/mman.h> #include <sys/types.h> #include <vector> @@ -67,6 +68,8 @@ private: static PixelFormat v4l2ToDrm(uint32_t format); static uint32_t drmToV4L2(const PixelFormat &format); + static std::set<unsigned long> supportedIoctls_; + unsigned int refcount_; unsigned int index_;
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 the main vidioc ioctl handler. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- Changes in v2: - moved !arg check to main ioctl handler, and added a set of supported ioctls - use !arg instead of arg == nullptr --- src/v4l2/v4l2_camera_proxy.cpp | 27 +++++++++++++++++++++++++-- src/v4l2/v4l2_camera_proxy.h | 3 +++ 2 files changed, 28 insertions(+), 2 deletions(-)