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

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart