Re: [PATCH v2] media: v4l2-ioctl: zero the ext control built for VIDIOC_{G,S}_CTRL

[Sakari Ailus <sakari.ailus@iki.fi>]

Hi Nick,

Thanks for the patch.

On Fri, Sep 25, 2026 at 09:02:06AM +0100, Nick Rogers wrote:
> When a driver implements the extended control ioctls but has no control
> handler, v4l_g_ctrl() and v4l_s_ctrl() pass VIDIOC_G_CTRL and
> VIDIOC_S_CTRL on as a single struct v4l2_ext_control built on the stack.
> Only its id and value are set, and check_ext_ctrls() clears reserved[0]
> and reserved2[0]; the control's size and the rest of both structures are
> left uninitialized.
> 
> A driver that forwards the controls rather than handling them through
> the control framework sees that stack garbage. The virtio-media driver
> under review takes a nonzero size as a payload to copy from userspace,
> so VIDIOC_G_CTRL and VIDIOC_S_CTRL fail with -EINVAL through it whenever
> the stack is dirty. GStreamer's V4L2 encoders set their profile with
> VIDIOC_S_CTRL, and cannot negotiate against such a device.
> 
> Zero-initialize both structures.
> 
> Assisted-by: LLM
> Signed-off-by: Nick Rogers <nick@getfieldwork.ai>
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> ---
> Changes in v2:
> - Assisted-by: LLM, per Documentation/process/coding-assistants.rst
>   (Alexandre)
> - Collected Nicolas's Reviewed-by
> 
> v1: https://lore.kernel.org/all/20260923160936.33445-1-nick@getfieldwork.ai/
> 
> Alexandre asked whether drivers should fill the structure themselves.
> The core builds it and passes it down, and a driver can't tell a
> translated G_CTRL/S_CTRL from a real extended control call, so I think
> it's the core's to zero. He's right that virtio-media will meet kernels
> without this, though, so the driver now guards against it too:
> https://lore.kernel.org/all/20260925080140.44696-1-nick@getfieldwork.ai/
> 
> Found running the virtio-media v9 series [1] in a VMM with a host-side
> stateful encoder: GStreamer's v4l2h264enc fails to negotiate because
> VIDIOC_S_CTRL returns -EINVAL. Tested on 6.18 with that series applied:
> VIDIOC_G_CTRL and VIDIOC_S_CTRL now reach the device intact, and
> v4l2-compliance 1.30.1 reports the same results with and without this
> patch. Build-tested on media.git next (arm64, W=1, no new warnings).
> 
> [1] https://lore.kernel.org/all/20260917171921.2810550-1-briandaniels@google.com/
> 
>  drivers/media/v4l2-core/v4l2-ioctl.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 17ba1ae70..b7d248ab7 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -2357,8 +2357,8 @@ static int v4l_g_ctrl(const struct v4l2_ioctl_ops *ops, struct file *file,
>  	struct video_device *vfd = video_devdata(file);
>  	struct v4l2_control *p = arg;
>  	struct v4l2_fh *vfh = file_to_v4l2_fh(file);
> -	struct v4l2_ext_controls ctrls;
> -	struct v4l2_ext_control ctrl;
> +	struct v4l2_ext_controls ctrls = {};
> +	struct v4l2_ext_control ctrl = {};
>  
>  	if (vfh && vfh->ctrl_handler)
>  		return v4l2_g_ctrl(vfh->ctrl_handler, p);
> @@ -2388,8 +2388,8 @@ static int v4l_s_ctrl(const struct v4l2_ioctl_ops *ops, struct file *file,
>  	struct video_device *vfd = video_devdata(file);
>  	struct v4l2_control *p = arg;
>  	struct v4l2_fh *vfh = file_to_v4l2_fh(file);
> -	struct v4l2_ext_controls ctrls;
> -	struct v4l2_ext_control ctrl;
> +	struct v4l2_ext_controls ctrls = {};
> +	struct v4l2_ext_control ctrl = {};

These are needed exceedingly rarely.

I'd declare them where the struct is filled now and initialise the fields
in declaration, too. That way there's no extra zeroing step and the struct
is only filled with anything when needed.

I wonder what Hans thinks about that.

>  	int ret;
>  
>  	if (vfh && vfh->ctrl_handler)

-- 
Kind regards,

Sakari Ailus