[libcamera-devel] [PATCH v2 03/17] v4l2: v4l2_camera_proxy: Check for null arg values in main ioctl handler

[Laurent Pinchart]

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 at 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 at 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_;
> >

-- 
Regards,

Laurent Pinchart