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 <[email protected]> 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/[email protected]?part=10
