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

[Jacopo Mondi]

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