| Message ID | 20230704232248.3861085-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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 ? > 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. > > Signed-off-by: Kieran Bingham <kieran.bingham@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 > + * 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 >
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@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 >> + * 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 >>
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@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 > >>
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@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@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 > > >>
Hi Kieran On 7/5/23 4:52 AM, 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, > 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. > > Signed-off-by: Kieran Bingham <kieran.bingham@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) It would be worth to add also why we can't accept int32_t request in the function parameter itself. Other than that, Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > { > 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 > + * 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;
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 + * 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;
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, 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. Signed-off-by: Kieran Bingham <kieran.bingham@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(-)