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

[Kieran Bingham]

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)
> {
>          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/

--
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
> >>