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

[Umang Jain]

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

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