* Re: [PATCH] media: v4l2-ctrls: document expected validation scope
2026-07-01 8:11 [PATCH] media: v4l2-ctrls: document expected validation scope Linus Walleij
@ 2026-07-01 8:50 ` Jacopo Mondi
2026-07-01 9:03 ` Hans Verkuil
2026-07-01 15:05 ` Vincenzo Frascino
2 siblings, 0 replies; 6+ messages in thread
From: Jacopo Mondi @ 2026-07-01 8:50 UTC (permalink / raw)
To: Linus Walleij
Cc: Laurent Pinchart, Vincenzo Frascino, Jacopo Mondi, Nayden Kanchev,
Konstantin Babin, Daniel Scally, Mauro Carvalho Chehab,
linux-media
Hi Linus
On Wed, Jul 01, 2026 at 10:11:20AM +0200, Linus Walleij wrote:
> After discussion on the mailing list it became clear that these
> validations are not expected to be exhaustive.
>
> Link: https://lore.kernel.org/linux-media/20260629133209.GG3054459@killaraus.ideasonboard.com/
The above linked discussion is about parameters buffers, which can
indeed contain large sets of data.
I'm not sure the same applies to extended controls ?
> Signed-off-by: Linus Walleij <linusw@kernel.org>
> ---
> Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> index b8698b85bd80..58184b6e386f 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> @@ -118,6 +118,18 @@ correct. This prevents the situation where only some of the controls
> were set/get. Only low-level errors (e. g. a failed i2c command) can
> still cause this situation.
>
> +The validation of controls is not expected to be exhaustive, for example
> +custom controls under ``V4L2_CTRL_CLASS_USER`` would become very voluminous.
> +The expected behaviour is to reject settings that could:
> +
> +1. Pose stability issues, such as turning the hardware unresponsive requiring
> + a hard reset, such controls must be rejected.
> +
> +2. Pose security issues, such as compromising unrelated kernel-contained
> + data to userspace.
> +
> +Extended validation of control values should be performed in userspace.
> +
> .. tabularcolumns:: |p{6.8cm}|p{4.0cm}|p{6.5cm}|
>
> .. c:type:: v4l2_ext_control
>
> ---
> base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482
> change-id: 20260701-v4l2-doc-c60195d38d00
>
> Best regards,
> --
> Linus Walleij <linusw@kernel.org>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] media: v4l2-ctrls: document expected validation scope
2026-07-01 8:11 [PATCH] media: v4l2-ctrls: document expected validation scope Linus Walleij
2026-07-01 8:50 ` Jacopo Mondi
@ 2026-07-01 9:03 ` Hans Verkuil
2026-07-01 10:59 ` Linus Walleij
2026-07-01 15:05 ` Vincenzo Frascino
2 siblings, 1 reply; 6+ messages in thread
From: Hans Verkuil @ 2026-07-01 9:03 UTC (permalink / raw)
To: Linus Walleij, Laurent Pinchart, Vincenzo Frascino, Jacopo Mondi,
Nayden Kanchev, Konstantin Babin, Daniel Scally,
Mauro Carvalho Chehab
Cc: linux-media
On 01/07/2026 10:11, Linus Walleij wrote:
> After discussion on the mailing list it became clear that these
> validations are not expected to be exhaustive.
>
> Link: https://lore.kernel.org/linux-media/20260629133209.GG3054459@killaraus.ideasonboard.com/
> Signed-off-by: Linus Walleij <linusw@kernel.org>
> ---
> Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> index b8698b85bd80..58184b6e386f 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> @@ -118,6 +118,18 @@ correct. This prevents the situation where only some of the controls
> were set/get. Only low-level errors (e. g. a failed i2c command) can
> still cause this situation.
>
> +The validation of controls is not expected to be exhaustive, for example
> +custom controls under ``V4L2_CTRL_CLASS_USER`` would become very voluminous.
CLASS_USER has nothing to do with this. I'd rephrase this as:
"...compound controls can be very complex."
> +The expected behaviour is to reject settings that could:
> +
> +1. Pose stability issues, such as turning the hardware unresponsive requiring
> + a hard reset, such controls must be rejected.
> +
> +2. Pose security issues, such as compromising unrelated kernel-contained
> + data to userspace.
> +
> +Extended validation of control values should be performed in userspace.
> +
I don't like this. If drivers do not validate controls well enough, and so allow
stability or security issues, then that's simply a bug. Ideally that check is done
when the controls are set, but this is not always possible. Sometimes this is only
possible when e.g. streaming is in progress.
And what "Extended validation" actually means is rather vague.
I would actually prefer to make some changes elsewhere in this file:
diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
index b8698b85bd80..d78328152b75 100644
--- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
+++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
@@ -72,12 +72,6 @@ partial array, all elements have to be set or retrieved. The total size
is calculated as ``elems`` * ``elem_size``. These values can be obtained
by calling :ref:`VIDIOC_QUERY_EXT_CTRL <VIDIOC_QUERYCTRL>`.
-To change the value of a set of controls applications initialize the
-``id``, ``size``, ``reserved2`` and ``value/value64/string/ptr`` fields
-of each struct :c:type:`v4l2_ext_control` and call
-the :ref:`VIDIOC_S_EXT_CTRLS <VIDIOC_G_EXT_CTRLS>` ioctl. The controls will only be set if *all*
-control values are valid.
-
To check if a set of controls have correct values applications
initialize the ``id``, ``size``, ``reserved2`` and
``value/value64/string/ptr`` fields of each struct
@@ -86,6 +80,15 @@ initialize the ``id``, ``size``, ``reserved2`` and
values are automatically adjusted to a valid value or if an error is
returned.
+To change the value of a set of controls applications initialize the
+``id``, ``size``, ``reserved2`` and ``value/value64/string/ptr`` fields
+of each struct :c:type:`v4l2_ext_control` and call
+the :ref:`VIDIOC_S_EXT_CTRLS <VIDIOC_G_EXT_CTRLS>` ioctl.
+:ref:`VIDIOC_S_EXT_CTRLS <VIDIOC_G_EXT_CTRLS>` will implicitly call
+:ref:`VIDIOC_TRY_EXT_CTRLS <VIDIOC_G_EXT_CTRLS>` first and return an
+error if that fails. So the controls will only be set if *all*
+control values are valid.
+
When the ``id`` or ``which`` is invalid drivers return an ``EINVAL`` error
code. When the value is out of bounds drivers can choose to take the
closest valid value or return an ``ERANGE`` error code, whatever seems more
The main change here is that it explicitly states that S_EXT_CTRLS implies
a call to TRY_EXT_CTRLS is called. Something that was never clearly stated
before, but it's important to know this. Because of this change the TRY_EXT_CTRLS
and S_EXT_CTRLS paragraphs are swapped since S_EXT_CTRLS now refers to
TRY_EXT_CTRLS.
And the TRY_EXT_CTRLS documentation already says that: "It is up to the driver
whether wrong values are automatically adjusted to a valid value or if an error is
returned."
I just saw Jacopo's reply that the discussion was for parameter buffers, not
for extended controls, but the reasoning is the same: the driver must validate
at some point (ideally when the buffer is queued up so userspace gets the error
immediately), and ensure there are no security or stability issues. But if there
are, then that's just a driver bug.
Regards,
Hans
> .. tabularcolumns:: |p{6.8cm}|p{4.0cm}|p{6.5cm}|
>
> .. c:type:: v4l2_ext_control
>
> ---
> base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482
> change-id: 20260701-v4l2-doc-c60195d38d00
>
> Best regards,
> --
> Linus Walleij <linusw@kernel.org>
>
>
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] media: v4l2-ctrls: document expected validation scope
2026-07-01 8:11 [PATCH] media: v4l2-ctrls: document expected validation scope Linus Walleij
2026-07-01 8:50 ` Jacopo Mondi
2026-07-01 9:03 ` Hans Verkuil
@ 2026-07-01 15:05 ` Vincenzo Frascino
2 siblings, 0 replies; 6+ messages in thread
From: Vincenzo Frascino @ 2026-07-01 15:05 UTC (permalink / raw)
To: Linus Walleij, Laurent Pinchart, Jacopo Mondi, Nayden Kanchev,
Konstantin Babin, Daniel Scally, Mauro Carvalho Chehab
Cc: linux-media
On 01/07/2026 09:11, Linus Walleij wrote:
> After discussion on the mailing list it became clear that these
> validations are not expected to be exhaustive.
>
> Link: https://lore.kernel.org/linux-media/20260629133209.GG3054459@killaraus.ideasonboard.com/
> Signed-off-by: Linus Walleij <linusw@kernel.org>
Thank you Linus for posting this. With the suggested additions would have
certainly helped my understanding.
With this:
Acked-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> ---
> Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> index b8698b85bd80..58184b6e386f 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> @@ -118,6 +118,18 @@ correct. This prevents the situation where only some of the controls
> were set/get. Only low-level errors (e. g. a failed i2c command) can
> still cause this situation.
>
> +The validation of controls is not expected to be exhaustive, for example
> +custom controls under ``V4L2_CTRL_CLASS_USER`` would become very voluminous.
> +The expected behaviour is to reject settings that could:
> +
> +1. Pose stability issues, such as turning the hardware unresponsive requiring
> + a hard reset, such controls must be rejected.
> +
> +2. Pose security issues, such as compromising unrelated kernel-contained
> + data to userspace.
> +
> +Extended validation of control values should be performed in userspace.
> +
> .. tabularcolumns:: |p{6.8cm}|p{4.0cm}|p{6.5cm}|
>
> .. c:type:: v4l2_ext_control
>
> ---
> base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482
> change-id: 20260701-v4l2-doc-c60195d38d00
>
> Best regards,
> --
> Linus Walleij <linusw@kernel.org>
>
--
Regards,
Vincenzo
^ permalink raw reply [flat|nested] 6+ messages in thread