| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CANiDSCtyMvbSMod3afjwR0aP+ZaG2tLugLTiC5q8tQyHL1k9pw@mail.gmail.com>
Date: Wed, 19 Nov 2025 23:09:46 +0100
From: Ricardo Ribalda <ribalda@...omium.org>
To: Gergo Koteles <soyer@....hu>
Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>, Hans de Goede <hansg@...nel.org>,
Mauro Carvalho Chehab <mchehab@...nel.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-usb@...r.kernel.org
Subject: Re: [PATCH v2 5/6] media: uvcvideo: Introduce allow_privacy_override param
Hi Gergo
Thanks for the review (and the discussion :) )
On Wed, 19 Nov 2025 at 22:54, Gergo Koteles <soyer@....hu> wrote:
>
> Hi Ricardo,
>
> On Wed, 2025-11-19 at 19:37 +0000, Ricardo Ribalda wrote:
> > Some camera modules have XU controls that can configure the behaviour of
> > the privacy LED.
> >
> > Block mapping of those controls, unless the module is configured with
> > a new parameter: allow_privacy_override.
> >
> > This is just an interim solution. Based on the users feedback, we will
> > either put the privacy controls behind a CONFIG option, or completely
> > block them.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> > ---
> > drivers/media/usb/uvc/uvc_ctrl.c | 38 ++++++++++++++++++++++++++++++++++++++
> > drivers/media/usb/uvc/uvc_driver.c | 20 ++++++++++++++++++++
> > drivers/media/usb/uvc/uvc_v4l2.c | 7 +++++++
> > drivers/media/usb/uvc/uvcvideo.h | 2 ++
> > include/linux/usb/uvc.h | 4 ++++
> > 5 files changed, 71 insertions(+)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index 57ce486f22bbc404a1f127539eb2d12373431631..d9cbb942f798dc7138608982a5d3e3ef9f8141f6 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -2951,6 +2951,35 @@ static int uvc_ctrl_init_xu_ctrl(struct uvc_device *dev,
> > return ret;
> > }
> >
> > +bool uvc_ctrl_is_privacy_control(u8 entity[16], u8 selector)
> > +{
> > + /*
> > + * This list is not exhaustive, it is a best effort to block access to
> > + * non documented controls that can affect user's privacy.
> > + */
> > + struct privacy_control {
> > + u8 entity[16];
> > + u8 selector;
> > + } privacy_control[] = {
> > + {
> > + .entity = UVC_GUID_LOGITECH_USER_HW_CONTROL_V1,
> > + .selector = 1,
> > + },
> > + {
> > + .entity = UVC_GUID_LOGITECH_PERIPHERAL,
> > + .selector = 9,
> > + },
> > + };
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(privacy_control); i++)
> > + if (!memcmp(entity, privacy_control[i].entity, 16) &&
> > + selector == privacy_control[i].selector)
> > + return true;
> > +
> > + return false;
> > +}
> > +
> > int uvc_xu_ctrl_query(struct uvc_video_chain *chain,
> > struct uvc_xu_control_query *xqry)
> > {
> > @@ -2995,6 +3024,15 @@ int uvc_xu_ctrl_query(struct uvc_video_chain *chain,
> > return -ENOENT;
> > }
> >
> > + if (uvc_ctrl_is_privacy_control(entity->guid, xqry->selector) &&
> > + !uvc_allow_privacy_override_param) {
> > + dev_warn_once(&chain->dev->intf->dev,
> > + "Privacy related controls can only be accessed if param allow_privacy_override is true\n");
> > + uvc_dbg(chain->dev, CONTROL, "Blocking access to privacy related Control %pUl/%u\n",
> > + entity->guid, xqry->selector);
> > + return -EACCES;
> > + }
> > +
>
> What if this only applied to UVC_SET_CUR?
>
> > if (mutex_lock_interruptible(&chain->ctrl_mutex))
> > return -ERESTARTSYS;
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index 71563d8f4bcf581694ccd4b665ff52b629caa0b6..c292bf8b6f57e9fdacee726285f5b46e638fd317 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -35,6 +35,7 @@ unsigned int uvc_hw_timestamps_param;
> > static unsigned int uvc_quirks_param = -1;
> > unsigned int uvc_dbg_param;
> > unsigned int uvc_timeout_param = UVC_CTRL_STREAMING_TIMEOUT;
> > +bool uvc_allow_privacy_override_param;
> >
> > static struct usb_driver uvc_driver;
> >
> > @@ -2474,6 +2475,25 @@ MODULE_PARM_DESC(trace, "Trace level bitmask");
> > module_param_named(timeout, uvc_timeout_param, uint, 0644);
> > MODULE_PARM_DESC(timeout, "Streaming control requests timeout");
> >
> > +static int param_set_privacy(const char *val, const struct kernel_param *kp)
> > +{
> > + pr_warn_once("uvcvideo: " DEPRECATED
> > + "allow_privacy_override parameter will be eventually removed.\n");
> > + return param_set_bool(val, kp);
> > +}
> > +
> > +static const struct kernel_param_ops param_ops_privacy = {
> > + .set = param_set_privacy,
> > + .get = param_get_bool,
> > +};
> > +
> > +param_check_bool(allow_privacy_override, &uvc_allow_privacy_override_param);
> > +module_param_cb(allow_privacy_override, ¶m_ops_privacy,
> > + &uvc_allow_privacy_override_param, 0644);
> > +__MODULE_PARM_TYPE(allow_privacy_override, "bool");
> > +MODULE_PARM_DESC(allow_privacy_override,
> > + "Allow access to privacy related controls");
> > +
> > /* ------------------------------------------------------------------------
> > * Driver initialization and cleanup
> > */
> > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> > index 03c64b5698bf4331fed8437fa6e9c726a07450bd..510cf47c86a62ba7fe3c7fa51be82c996cf37f9f 100644
> > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > @@ -133,6 +133,13 @@ static int uvc_ioctl_xu_ctrl_map(struct uvc_video_chain *chain,
> > return -EINVAL;
> > }
> >
> > + if (uvc_ctrl_is_privacy_control(xmap->entity, xmap->selector) &&
> > + !uvc_allow_privacy_override_param) {
> > + dev_warn_once(&chain->dev->intf->dev,
> > + "Privacy related controls can only be mapped if param allow_privacy_override is true\n");
> > + return -EACCES;
> > + }
> > +
>
> Would a better solution be to be able to map and query, but not set?
IMO it is less confusing if the controls are fully disabled.
Maybe it is worth it to land the patches that we do not have any
disagreement with (1-4) . And then make a new patchset thread with 5
and have a proper discussion there about usecases, mechanism to bypass
block and future plans.
Best regards
>
>
> Regards,
> Gergo
--
Ricardo Ribalda
Powered by blists - more mailing lists