[libcamera-devel] [PATCH] v4l2: v4l2_camera_proxy: Prevent ioctl sign-extensions

[Jacopo Mondi]

Hi Kieran

On Tue, Jul 11, 2023 at 11:26:56AM +0100, Kieran Bingham wrote:
> Quoting Kieran Bingham (2023-07-11 10:20:14)
> > Hi Jacopo,
> >
> > On 11/07/2023 10:06, Jacopo Mondi wrote:
> > > Hi Kieran
> > >
> > > On Wed, Jul 05, 2023 at 12:22:48AM +0100, Kieran Bingham via libcamera-devel wrote:
> > >> Handling ioctl's within applications is often wrapped with a helper such
> > >> as xioctl. Unfortunately, there are many instances of xioctl which
> > >> incorrectly handle the correct size of the ioctl request.
> > >>
> > >> This leads to incorrect sign-extension of ioctl's which have bit-31 set,
> > >
> > > I would expect this if the function arguments were signed, but does
> > > sign-extension happens with unsigned arguments as well ?
> >
> > The issue is in the caller - not this function. This patch aims to
> > handle things in the same way the kernel does (which only processes 32
> > bits of the data).
> >
> > For instance, the linux kernel documentation has a sample implementation
> > of xioctl that would cause the fault that this patch fixes:
> >
> > https://www.kernel.org/doc/html/v4.11/media/uapi/v4l/capture.c.html
> >
> > static int xioctl(int fh, int request, void *arg)

Shouldn't this be a long ?

> > {
> >          int r;
> >
> >          do {
> >                  r = ioctl(fh, request, arg);
> >          } while (-1 == r && EINTR == errno);
> >
> >          return r;
> > }
> >
> > If an application uses that xioctl, with the 'int request' and calls
> >
> >    if (xioctl(vid_source, VIDIOC_QUERYCAP, &vid_source->cap) < 0) {
> >    }
> >
> > The VIDIOC_QUERYCAP (0x80685600) gets sign extended into
> > (0xffffffff80685600) and then doesn't match the parsing in the V4L2
> > adaptation layer.
> >
> >
> > >
> > >> and can cause values to be passed into the libcamera's v4l2 adaptation
> > >> layer which no longer represent the true IOCTL code.
> > >>
> > >> Match the implementation of the Linux kernel and ensure that only 32
> > >> bits of the ioctl request are used by assigning to an unsigned int.
> > >>
> >
> > I'll add:
> >
> > Link: https://github.com/Motion-Project/motion/discussions/1636
> >
> > >> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > >> ---
> > >> This is a (simpler) rework of the previous patch [0]:
> > >>   v4l2: v4l2_camera_proxy: Detect ioctl sign-extensions
> > >>
> > >> [0] https://patchwork.libcamera.org/patch/18311/
> > >>
> > >>   src/v4l2/v4l2_camera_proxy.cpp | 12 +++++++++++-
> > >>   1 file changed, 11 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> > >> index 55ff62cdb430..dfb672080219 100644
> > >> --- a/src/v4l2/v4l2_camera_proxy.cpp
> > >> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> > >> @@ -778,10 +778,20 @@ const std::set<unsigned long> V4L2CameraProxy::supportedIoctls_ = {
> > >>      VIDIOC_STREAMOFF,
> > >>   };
> > >>
> > >> -int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long request, void *arg)
> > >> +int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long longRequest, void *arg)
> > >>   {
> > >>      MutexLocker locker(proxyMutex_);
> > >>
> > >> +    /*
> > >> +     * The Linux Kernel only processes 32 bits of an IOCTL.
> > >> +     *
> > >> +     * Prevent unexpected sign-extensions that could occur if applications
> > >> +     * use an unsigned int for the ioctl request, which would sign-extend
>
> Aha, perhaps your comment was here and this should actually read:
>
> 	"if applications use a signed int for the ioctl request"
>
> I'll s/unsigned int/signed int/
>

Thanks, also this bit was confusing

Anyway, the patch looks good
Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>

Thanks
  j

> --
> Kieran
>
>
> > >> +     * to an incorrect value for unsigned longs on 64 bit architectures by
> > >> +     * explicitly casting as an unsigned int here.
> > >> +     */
> > >> +    unsigned int request = longRequest;
> > >> +
> > >>      if (!arg && (_IOC_DIR(request) & _IOC_WRITE)) {
> > >>              errno = EFAULT;
> > >>              return -1;
> > >> --
> > >> 2.34.1
> > >>