[{"id":5280,"web_url":"https://patchwork.libcamera.org/comment/5280/","msgid":"<20200619102230.ymc2gir3ssr4ccbv@uno.localdomain>","date":"2020-06-19T10:22:30","subject":"Re: [libcamera-devel] [PATCH v2 03/17] v4l2: v4l2_camera_proxy:\n\tCheck for null arg values in main ioctl handler","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n\nOn Fri, Jun 19, 2020 at 02:41:09PM +0900, Paul Elder wrote:\n> The ioctl handlers currently don't check if arg is null, so if it ever\n> is, it will cause a segfault. Check that arg is null and return -EFAULT\n> in the main vidioc ioctl handler.\n>\n> Signed-off-by: Paul Elder \n>\n> ---\n> Changes in v2:\n> - moved !arg check to main ioctl handler, and added a set of supported\n> ioctls\n> - use !arg instead of arg == nullptr\n> ---\n> src/v4l2/v4l2_camera_proxy.cpp | 27 +++++++++++++++++++++++++--\n> src/v4l2/v4l2_camera_proxy.h | 3 +++\n> 2 files changed, 28 insertions(+), 2 deletions(-)\n>\n> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> index f06f58d..cff6562 100644\n> --- a/src/v4l2/v4l2_camera_proxy.cpp\n> +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> @@ -11,6 +11,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -238,7 +239,6 @@ int V4L2CameraProxy::vidioc_enum_fmt(V4L2CameraFile *cf, struct v4l2_fmtdesc *ar\n> {\n> \tLOG(V4L2Compat, Debug) << \"Servicing vidioc_enum_fmt fd = \" << cf->efd();\n>\n> -\n\nWhere does these spurious empty lines come from ?\n\n> \tif (!validateBufferType(arg->type) ||\n> \t arg->index >= streamConfig_.formats().pixelformats().size())\n> \t\treturn -EINVAL;\n> @@ -255,7 +255,6 @@ int V4L2CameraProxy::vidioc_g_fmt(V4L2CameraFile *cf, struct v4l2_format *arg)\n> {\n> \tLOG(V4L2Compat, Debug) << \"Servicing vidioc_g_fmt fd = \" << cf->efd();\n>\n> -\n> \tif (!validateBufferType(arg->type))\n> \t\treturn -EINVAL;\n>\n> @@ -543,8 +542,32 @@ int V4L2CameraProxy::vidioc_streamoff(V4L2CameraFile *cf, int *arg)\n> \treturn ret;\n> }\n>\n> +std::set V4L2CameraProxy::supportedIoctls_ = {\n> +\tVIDIOC_QUERYCAP,\n> +\tVIDIOC_ENUM_FMT,\n> +\tVIDIOC_G_FMT,\n> +\tVIDIOC_S_FMT,\n> +\tVIDIOC_TRY_FMT,\n> +\tVIDIOC_REQBUFS,\n> +\tVIDIOC_QUERYBUF,\n> +\tVIDIOC_QBUF,\n> +\tVIDIOC_DQBUF,\n> +\tVIDIOC_STREAMON,\n> +\tVIDIOC_STREAMOFF,\n> +};\n\nI understand why an std::set(), as it provides a ::find() function\nwhich ease lookup. I'm wondering if it's worth it though, as a\nfunction in an anonymous namespace which simply switch on the\nsupported requests might be more efficient.\n\nAnyway, have you considered having this in an anonymous namespace\ninstead of making a static class member out of it ? Is it accessed\nfrom any other class (don't think so, as it's a private static class\nmember)\n\n> +\n> int V4L2CameraProxy::ioctl(V4L2CameraFile *cf, unsigned long request, void *arg)\n> {\n> +\tif (supportedIoctls_.find(request) == supportedIoctls_.end()) {\n> +\t\terrno = ENOTTY;\n> +\t\treturn -1;\n> +\t}\n> +\n> +\tif (!arg) {\n> +\t\terrno = EFAULT;\n> +\t\treturn -1;\n> +\t}\n> +\n> \tint ret;\n> \tswitch (request) {\n> \tcase VIDIOC_QUERYCAP:\n> diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h\n> index 43290ca..dd7e793 100644\n> --- a/src/v4l2/v4l2_camera_proxy.h\n> +++ b/src/v4l2/v4l2_camera_proxy.h\n> @@ -11,6 +11,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -67,6 +68,8 @@ private:\n> \tstatic PixelFormat v4l2ToDrm(uint32_t format);\n> \tstatic uint32_t drmToV4L2(const PixelFormat &format);\n>\n> +\tstatic std::set supportedIoctls_;\n> +\n> \tunsigned int refcount_;\n> \tunsigned int index_;\n>\n> --\n> 2.27.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A35F760103\n\tfor ;\n\tFri, 19 Jun 2020 12:19:05 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id 213AA240009;\n\tFri, 19 Jun 2020 10:19:04 +0000 (UTC)"],"Date":"Fri, 19 Jun 2020 12:22:30 +0200","From":"Jacopo Mondi ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200619102230.ymc2gir3ssr4ccbv@uno.localdomain>","References":"<20200619054123.19052-1-paul.elder@ideasonboard.com>\n\t<20200619054123.19052-4-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200619054123.19052-4-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 03/17] v4l2: v4l2_camera_proxy:\n\tCheck for null arg values in main ioctl handler","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 19 Jun 2020 10:19:05 -0000"}},{"id":5293,"web_url":"https://patchwork.libcamera.org/comment/5293/","msgid":"<20200620011217.GQ5823@pendragon.ideasonboard.com>","date":"2020-06-20T01:12:17","subject":"Re: [libcamera-devel] [PATCH v2 03/17] v4l2: v4l2_camera_proxy:\n\tCheck for null arg values in main ioctl handler","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Fri, Jun 19, 2020 at 12:22:30PM +0200, Jacopo Mondi wrote:\n> On Fri, Jun 19, 2020 at 02:41:09PM +0900, Paul Elder wrote:\n> > The ioctl handlers currently don't check if arg is null, so if it ever\n> > is, it will cause a segfault. Check that arg is null and return -EFAULT\n> > in the main vidioc ioctl handler.\n> >\n> > Signed-off-by: Paul Elder \n> >\n> > ---\n> > Changes in v2:\n> > - moved !arg check to main ioctl handler, and added a set of supported\n> > ioctls\n> > - use !arg instead of arg == nullptr\n> > ---\n> > src/v4l2/v4l2_camera_proxy.cpp | 27 +++++++++++++++++++++++++--\n> > src/v4l2/v4l2_camera_proxy.h | 3 +++\n> > 2 files changed, 28 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> > index f06f58d..cff6562 100644\n> > --- a/src/v4l2/v4l2_camera_proxy.cpp\n> > +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> > @@ -11,6 +11,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -238,7 +239,6 @@ int V4L2CameraProxy::vidioc_enum_fmt(V4L2CameraFile *cf, struct v4l2_fmtdesc *ar\n> > {\n> > \tLOG(V4L2Compat, Debug) << \"Servicing vidioc_enum_fmt fd = \" << cf->efd();\n> >\n> > -\n> \n> Where does these spurious empty lines come from ?\n\nI suspect a rebase mistake :-) It should indeed be fixed in the previous\npatch.\n\n> > \tif (!validateBufferType(arg->type) ||\n> > \t arg->index >= streamConfig_.formats().pixelformats().size())\n> > \t\treturn -EINVAL;\n> > @@ -255,7 +255,6 @@ int V4L2CameraProxy::vidioc_g_fmt(V4L2CameraFile *cf, struct v4l2_format *arg)\n> > {\n> > \tLOG(V4L2Compat, Debug) << \"Servicing vidioc_g_fmt fd = \" << cf->efd();\n> >\n> > -\n> > \tif (!validateBufferType(arg->type))\n> > \t\treturn -EINVAL;\n> >\n> > @@ -543,8 +542,32 @@ int V4L2CameraProxy::vidioc_streamoff(V4L2CameraFile *cf, int *arg)\n> > \treturn ret;\n> > }\n> >\n> > +std::set V4L2CameraProxy::supportedIoctls_ = {\n\nThis should be const.\n\n> > +\tVIDIOC_QUERYCAP,\n> > +\tVIDIOC_ENUM_FMT,\n> > +\tVIDIOC_G_FMT,\n> > +\tVIDIOC_S_FMT,\n> > +\tVIDIOC_TRY_FMT,\n> > +\tVIDIOC_REQBUFS,\n> > +\tVIDIOC_QUERYBUF,\n> > +\tVIDIOC_QBUF,\n> > +\tVIDIOC_DQBUF,\n> > +\tVIDIOC_STREAMON,\n> > +\tVIDIOC_STREAMOFF,\n> > +};\n> \n> I understand why an std::set(), as it provides a ::find() function\n> which ease lookup. I'm wondering if it's worth it though, as a\n> function in an anonymous namespace which simply switch on the\n> supported requests might be more efficient.\n\nWould it ? It's a honest question, I'm not sure how the compiler would\noptimize that.\n\n> Anyway, have you considered having this in an anonymous namespace\n> instead of making a static class member out of it ? Is it accessed\n> from any other class (don't think so, as it's a private static class\n> member)\n\nI have a slight preference for a set here. As for storing it in the\nclass or as a global variable in an anonymous namespace, I don't mind\nmuch either way, especially as V4L2CameraProxy is an internal class. I'd\ncertainly go for the latter if the class was part of the public API.\n\n> > +\n> > int V4L2CameraProxy::ioctl(V4L2CameraFile *cf, unsigned long request, void *arg)\n> > {\n\nIf we want to match V4L2, should we here add\n\n\tif (!arg && (_IOC_DIR(cmd) & _IOC_WRITE)) {\n\t\terrno = EFAULT;\n\t\treturn -1;\n\t}\n\n> > +\tif (supportedIoctls_.find(request) == supportedIoctls_.end()) {\n> > +\t\terrno = ENOTTY;\n> > +\t\treturn -1;\n> > +\t}\n> > +\n> > +\tif (!arg) {\n\n\tif (!arg && (_IOC_DIR(cmd) & _IOC_READ)) {\n\nWith the small issues addressed,\n\nReviewed-by: Laurent Pinchart \n\n> > +\t\terrno = EFAULT;\n> > +\t\treturn -1;\n> > +\t}\n> > +\n> > \tint ret;\n> > \tswitch (request) {\n> > \tcase VIDIOC_QUERYCAP:\n> > diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h\n> > index 43290ca..dd7e793 100644\n> > --- a/src/v4l2/v4l2_camera_proxy.h\n> > +++ b/src/v4l2/v4l2_camera_proxy.h\n> > @@ -11,6 +11,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -67,6 +68,8 @@ private:\n> > \tstatic PixelFormat v4l2ToDrm(uint32_t format);\n> > \tstatic uint32_t drmToV4L2(const PixelFormat &format);\n> >\n> > +\tstatic std::set supportedIoctls_;\n> > +\n> > \tunsigned int refcount_;\n> > \tunsigned int index_;\n> >","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F079760710\n\tfor ;\n\tSat, 20 Jun 2020 03:12:41 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 528C2552;\n\tSat, 20 Jun 2020 03:12:41 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"HoGqndwH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1592615561;\n\tbh=qXCrIpV31Fw3d6msHZLnZ74No9QiiL2jswi5uoJFI/E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HoGqndwHEOdVgnvUUb1KiniIdQVc9GL+2hik8ahVIG0qbq/uGluLhMby6UxKc0I8A\n\tbyoGrGVy1mEC+IfBMkNTtyh+aOTLf1FL5tgAEdcdqqGX5MmA5tVSDID1rw1lTLobtL\n\tvYsqrg5FaR4MuAWPmhq5r3c1TvqkY/TPxivNgwnc=","Date":"Sat, 20 Jun 2020 04:12:17 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20200620011217.GQ5823@pendragon.ideasonboard.com>","References":"<20200619054123.19052-1-paul.elder@ideasonboard.com>\n\t<20200619054123.19052-4-paul.elder@ideasonboard.com>\n\t<20200619102230.ymc2gir3ssr4ccbv@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200619102230.ymc2gir3ssr4ccbv@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 03/17] v4l2: v4l2_camera_proxy:\n\tCheck for null arg values in main ioctl handler","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sat, 20 Jun 2020 01:12:42 -0000"}}]