[libcamera-devel] [PATCH 02/15] v4l2: v4l2_camera_proxy: Check for null arg values in ioctl handlers
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jun 17 17:21:54 CEST 2020
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 at 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;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list