Re: [PATCH v3 10/13] drm/vino: read the dock's firmware version, and update it over DFU

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The automatic update path in `update_if_newer` fails to verify that the firmware package matches the dock's hardware family, allowing a mismatched firmware file to permanently brick the dock.
- [Medium] Infinite update loop and data race due to a single shared non-atomic rate-limit slot (`UPDATE_ATTEMPTS`).
--

commit 872c2fc3f8d6d143ff1f9c6026fbab419e5a21e3
Author: Mike Lothian <mike@fireburn.co.uk>
Subject: drm/vino: read the dock's firmware version, and update it over DFU

A DisplayLink dock does not advance bcdDevice when its firmware changes. This
patch adds parsers for the dock's identity descriptor and shipped firmware
package, and applies updates automatically over USB DFU if the packaged
version is strictly newer. It also provides a manual upload interface.

> diff --git a/drivers/gpu/drm/vino/firmware.rs b/drivers/gpu/drm/vino/firmware.rs
> new file mode 100644
> index 0000000000000..13c9cff55ae48
> --- /dev/null
> +++ b/drivers/gpu/drm/vino/firmware.rs
[ ... ]
> +fn claim_update_attempt(dev: &Device) -> bool {
> +    let key = attempt_key(dev);
> +    let slot = UPDATE_ATTEMPTS.load(Ordering::Acquire);
> +    let count = if slot & ATTEMPT_KEY_MASK == key {
> +        slot as u32
> +    } else {
> +        0
> +    };
> +    if count >= MAX_UPDATE_ATTEMPTS {
> +        return false;
> +    }
> +    UPDATE_ATTEMPTS.store(key | u64::from(count + 1), Ordering::Release);
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Medium]
Could this non-atomic read-modify-write on the shared global UPDATE_ATTEMPTS
variable cause an infinite update loop?

If two docks are connected that require an update, and both fail to apply it
successfully, they might re-enumerate and probe continuously. Because
claim_update_attempt() loads and then stores on a single global slot, it looks
like the two docks might continuously evict each other's attempt counts.

This could prevent either dock from reaching MAX_UPDATE_ATTEMPTS to break
the loop, which might lead to constant USB bus resets and flash memory wear-out.

Similarly, clear_update_attempts() performs a non-atomic check-and-clear
that might race with this logic and clear another device's freshly recorded
attempts:

    if UPDATE_ATTEMPTS.load(Ordering::Acquire) & ATTEMPT_KEY_MASK == key {
        UPDATE_ATTEMPTS.store(0, Ordering::Release);
    }

[ ... ]
> +pub(crate) fn update_if_newer(
> +    io: &usb::Io<'_>,
> +    dev: &Device,
> +    identity: &Identity,
> +    iface: u16,
> +    force: bool,
> +) -> Result {
[ ... ]
> +    let Some(packaged) = package_version(fw.data()) else {
> +        dev_warn!(dev, "{} carries no version tag; ignoring it\n", name);
> +        return Ok(());
> +    };
> +    if packaged <= identity.version && !force {
> +        clear_update_attempts(dev);
> +        vino_dev_debug!(dev, "firmware is current ({} offers {})\n", name, packaged);
> +        return Ok(());
> +    }
[ ... ]
> +    dev_info!(
> +        dev,
> +        "updating dock firmware {} -> {}{}\n",
> +        identity.version,
> +        packaged,
> +        if force { " (forced)" } else { "" }
> +    );
> +    flash(io, iface, fw.data())
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Should this automatic update path verify that the firmware package matches
the dock's hardware family before flashing?

The manual upload path (Upload::prepare) uses package_family(image) and
refuses to flash if the family does not match the hardware. However,
update_if_newer() only checks the version and relies on the filename.

If a mismatched firmware file is inadvertently placed in /lib/firmware/vino/,
it appears this would silently flash the incompatible image and permanently
render the dock unusable, circumventing the safety check implemented for
manual uploads.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826163913.7052-1-mike@fireburn.co.uk?part=10