| Message ID | 20230227203947.3964157-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
On Mon, Feb 27, 2023 at 08:39:47PM +0000, 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. > > We can easily identify if an application has mistakenly sign-extended an > ioctl command request and mask out those bits. > > Do so and report the error loudly, but only once that the application > should be fixed. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > > This is a pain. It's not 'libcamera's' fault. But we can do something > about it. Welp :/ Yeah we can do something about it. I see a future where the error gets ignored because it "still works" though. Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > See > - https://github.com/kbingham/libcamera/issues/69 > and > - https://github.com/Motion-Project/motion/discussions/1636 > > > > src/v4l2/v4l2_camera_proxy.cpp | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp > index 55ff62cdb430..75c53aedf2fa 100644 > --- a/src/v4l2/v4l2_camera_proxy.cpp > +++ b/src/v4l2/v4l2_camera_proxy.cpp > @@ -782,6 +782,24 @@ int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long request, void *ar > { > MutexLocker locker(proxyMutex_); > > + /* > + * Applications may easily find themselves incorrectly sign-extending > + * ioctls on 64-bit systems which can cause hard to diagnose failures > + * with the v4l2-adatation layer. Identify this failure, and work > + * around it - but report the issue as it should be fixed in the > + * application. > + */ > + if (request & 0xffffffff00000000) { > + static bool warnOnce = true; > + > + if (warnOnce) { > + LOG(V4L2Compat, Error) << "ioctl sign extension detected."; > + LOG(V4L2Compat, Error) << "Please fix your application."; > + warnOnce = false; > + } > + request &= 0xffffffff; > + } > + > if (!arg && (_IOC_DIR(request) & _IOC_WRITE)) { > errno = EFAULT; > return -1; > -- > 2.34.1 >
Hi Kieran, Thank you for the patch. On Mon, Feb 27, 2023 at 08:39:47PM +0000, 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. > > We can easily identify if an application has mistakenly sign-extended an > ioctl command request and mask out those bits. > > Do so and report the error loudly, but only once that the application > should be fixed. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > > This is a pain. It's not 'libcamera's' fault. But we can do something > about it. > > See > - https://github.com/kbingham/libcamera/issues/69 > and > - https://github.com/Motion-Project/motion/discussions/1636 > > src/v4l2/v4l2_camera_proxy.cpp | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp > index 55ff62cdb430..75c53aedf2fa 100644 > --- a/src/v4l2/v4l2_camera_proxy.cpp > +++ b/src/v4l2/v4l2_camera_proxy.cpp > @@ -782,6 +782,24 @@ int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long request, void *ar > { > MutexLocker locker(proxyMutex_); > > + /* > + * Applications may easily find themselves incorrectly sign-extending > + * ioctls on 64-bit systems which can cause hard to diagnose failures > + * with the v4l2-adatation layer. Identify this failure, and work s/adatation/adaptation/ > + * around it - but report the issue as it should be fixed in the > + * application. As far as I understand, the kernel drops the upper 32 bits (see fs/ioctl.c which defines the ioctl syscall handler with an unsigned int cmd argument). I suppose it's nice to hint that sign extension isn't a great idea, but I'm not sure this can be considered an application error. You may want to adjust the comment accordingly (and keep in mind I may well be wrong about the whole thing :-)). > + */ > + if (request & 0xffffffff00000000) { > + static bool warnOnce = true; > + > + if (warnOnce) { > + LOG(V4L2Compat, Error) << "ioctl sign extension detected."; > + LOG(V4L2Compat, Error) << "Please fix your application."; You could write this in a single message. > + warnOnce = false; > + } > + request &= 0xffffffff; > + } > + > if (!arg && (_IOC_DIR(request) & _IOC_WRITE)) { > errno = EFAULT; > return -1;
Hi Laurent, On 28/02/2023 23:46, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Mon, Feb 27, 2023 at 08:39:47PM +0000, 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. >> >> We can easily identify if an application has mistakenly sign-extended an >> ioctl command request and mask out those bits. >> >> Do so and report the error loudly, but only once that the application >> should be fixed. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> >> This is a pain. It's not 'libcamera's' fault. But we can do something >> about it. >> >> See >> - https://github.com/kbingham/libcamera/issues/69 >> and >> - https://github.com/Motion-Project/motion/discussions/1636 >> >> src/v4l2/v4l2_camera_proxy.cpp | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> >> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp >> index 55ff62cdb430..75c53aedf2fa 100644 >> --- a/src/v4l2/v4l2_camera_proxy.cpp >> +++ b/src/v4l2/v4l2_camera_proxy.cpp >> @@ -782,6 +782,24 @@ int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long request, void *ar >> { >> MutexLocker locker(proxyMutex_); >> >> + /* >> + * Applications may easily find themselves incorrectly sign-extending >> + * ioctls on 64-bit systems which can cause hard to diagnose failures >> + * with the v4l2-adatation layer. Identify this failure, and work > > s/adatation/adaptation/ > >> + * around it - but report the issue as it should be fixed in the >> + * application. > > As far as I understand, the kernel drops the upper 32 bits (see > fs/ioctl.c which defines the ioctl syscall handler with an unsigned int > cmd argument). I suppose it's nice to hint that sign extension isn't a > great idea, but I'm not sure this can be considered an application > error. You may want to adjust the comment accordingly (and keep in mind > I may well be wrong about the whole thing :-)). It's confusing, and I was looking for somewhere explicitly masking out the bits - I'd missed that it just gets ignored/masked by the compiler/interfaces because that's the type on the argument. man ioctl, definitely states that ioctl() takes an unsigned long... so we can't change our args. Maybe even silently masking is fine. But I'm happier in this small case to report it >> + */ >> + if (request & 0xffffffff00000000) { I fear that being invalid or inefficient on 32 bit platforms though. /o\ And we can probably simplify all of this with something more like: -int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long request, void *arg) +int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long longReq, 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 = longReq; + >> + static bool warnOnce = true; >> + >> + if (warnOnce) { >> + LOG(V4L2Compat, Error) << "ioctl sign extension detected."; >> + LOG(V4L2Compat, Error) << "Please fix your application."; > > You could write this in a single message. I did that intentionally, but I'll keep the first line, and drop the second. Or move to the unsigned int request type and remove the warning all together... > >> + warnOnce = false; >> + } >> + request &= 0xffffffff; >> + } >> + >> 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..75c53aedf2fa 100644 --- a/src/v4l2/v4l2_camera_proxy.cpp +++ b/src/v4l2/v4l2_camera_proxy.cpp @@ -782,6 +782,24 @@ int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long request, void *ar { MutexLocker locker(proxyMutex_); + /* + * Applications may easily find themselves incorrectly sign-extending + * ioctls on 64-bit systems which can cause hard to diagnose failures + * with the v4l2-adatation layer. Identify this failure, and work + * around it - but report the issue as it should be fixed in the + * application. + */ + if (request & 0xffffffff00000000) { + static bool warnOnce = true; + + if (warnOnce) { + LOG(V4L2Compat, Error) << "ioctl sign extension detected."; + LOG(V4L2Compat, Error) << "Please fix your application."; + warnOnce = false; + } + request &= 0xffffffff; + } + 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. We can easily identify if an application has mistakenly sign-extended an ioctl command request and mask out those bits. Do so and report the error loudly, but only once that the application should be fixed. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- This is a pain. It's not 'libcamera's' fault. But we can do something about it. See - https://github.com/kbingham/libcamera/issues/69 and - https://github.com/Motion-Project/motion/discussions/1636 src/v4l2/v4l2_camera_proxy.cpp | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)