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

[Paul Elder]

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