Re: [RFC PATCH 5/7] drm/vino: register a DRM/KMS device and scan out to EP08

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [Critical] Out-of-Bounds Read due to missing plane `atomic_check`
- [High] Sleeping in Atomic Context
- [High] API Contract Violation / Memory Corruption for pitch
- [High] Data Race / Locking Violation in atomic_check
- [High] Workqueue Leak / UAF on Module Unload
- [High] Global Statics for Device State
- [Medium] Endianness Bug in Pixel Conversion
- [Medium] Initialization Ordering Bug causing kernel warnings
- [Medium] Hardware Desync on Cursor Resizing
- [Medium] Cursor Clipping Logic Error
--

commit 8fb2c0d2dd1d854ba4571103d0392a06d8d1e01a
Author: Mike Lothian <mike@fireburn.co.uk>
Add the drm_sink module: register a real struct drm_device with a

This commit adds a DRM/KMS device registration to the vino driver.
It implements a hand-rolled atomic mode-setting pipeline with CRTC,
primary and cursor planes. It handles EDID caching and EP08 video
scanout.

> diff --git a/drivers/gpu/drm/vino/vino.rs b/drivers/gpu/drm/vino/vino.rs
> index e9e6324b717b..1091dcc992c7 100644
> --- a/drivers/gpu/drm/vino/vino.rs
> +++ b/drivers/gpu/drm/vino/vino.rs
> @@ -79,6 +80,24 @@ fn timeout() -> Delta {
>  /// hardware -- the dock runs the whole plaintext handshake but never engages the encrypted CP.
>  static CP_ENGAGED: core::sync::atomic::AtomicBool = core::sync::atomic::AtomicBool::new(false);
>  
> +/// One-shot: clear-halt + prime the video endpoints before the first live-scanout EP08 write.
> +static EP08_SCANOUT_PRIMED: core::sync::atomic::AtomicBool =
> +    core::sync::atomic::AtomicBool::new(false);

[Severity: High]
Will this cause races when multiple docks are connected?

These variables are declared as global statics rather than per-device
state. If one dock successfully finishes bring-up, CP_ENGAGED becomes
globally true. The driver will then incorrectly assume all other
connected docks are ready and push EP08 video frames to them before
their CP is engaged, causing those docks to fault and enter a USB
reset loop.

[ ... ]

> @@ -1608,12 +1713,12 @@ fn probe<'bound>(
>          // exactly like any other failed bring-up step.
> -        match BringUp::new(intf_ref.clone()) {
> +        match BringUp::new(intf_ref.clone(), ddev.clone()) {
>              Ok(work) => {
>                  let _ = workqueue::system().enqueue(work);
>                  dev_info!(cdev, "vino: bring-up queued on system workqueue\n");

[Severity: High]
Does this leak the workqueue task on module unload?

The BringUp work item is enqueued but the handle is discarded. As a
result, VinoDriver::disconnect cannot explicitly cancel the background
task before the driver is unbound.

If the USB device is unplugged and the module is subsequently unloaded
while the work is pending or executing, the system workqueue will
attempt to execute the run function which resides in the freed text
section, causing a kernel oops.

> diff --git a/drivers/gpu/drm/vino/drm_sink.rs b/drivers/gpu/drm/vino/drm_sink.rs
> new file mode 100644
> index 000000000000..afbf883fba36
> --- /dev/null
> +++ b/drivers/gpu/drm/vino/drm_sink.rs
> @@ -0,0 +1,1333 @@

[ ... ]

> +unsafe extern "C" fn vino_atomic_check(
> +    dev: *mut bindings::drm_device,
> +    state: *mut bindings::drm_atomic_commit,
> +) -> i32 {
[ ... ]
> +        let mut cs = unsafe { bindings::drm_atomic_get_new_crtc_state(state, crtc) };
> +        if cs.is_null() {
> +            // SAFETY: `crtc` is initialised; `.state` is its current committed state (or NULL).
> +            cs = unsafe { (*crtc).state };
> +        }

[Severity: High]
Can this cause a data race or use-after-free?

In vino_atomic_check, the driver iterates over all heads and sums the clock
frequencies. Accessing a CRTC's state without acquiring its commit lock
(which drm_atomic_get_crtc_state handles safely) allows a concurrent
non-blocking commit on that other CRTC to modify or free its state while
it is being read here.

[ ... ]

> +unsafe extern "C" fn primary_atomic_update(
> +    plane: *mut bindings::drm_plane,
> +    _state: *mut bindings::drm_atomic_commit,
> +) {
[ ... ]
> +    let (dev_raw, fb, w, h, damage, rotation) = unsafe {
> +        let st = (*plane).state;
> +        if st.is_null() {
> +            return;
> +        }
> +        // Plane destination geometry == the negotiated mode (the compositor sizes the primary
> +        // plane 1:1 with a virtual output), so this drives the dynamic scanout resolution.
> +        let (w, h) = ((*st).crtc_w as usize, (*st).crtc_h as usize);
> +        ((*plane).dev, (*st).fb, w, h, damage_bbox(st), (*st).rotation)
> +    };

[Severity: Critical]
Could this read out-of-bounds memory?

The plane lacks an atomic_check to validate scaling constraints. In
primary_atomic_update, the driver uses the destination dimensions
(crtc_w, crtc_h) to determine the bounds for reading the mapped source
framebuffer.

If userspace attaches a small GEM buffer but requests a large destination
size, encode_and_send will perform pointer arithmetic far beyond the mapped
buffer into unmapped or unrelated kernel memory.

[ ... ]

> +    let gamma = read_gamma_lut(head);
> +    match scanout_one(data, head, fb, w, h, damage, rotation, gamma.as_ref()) {

[Severity: High]
Does this code sleep in atomic context?

The DRM subsystem requires that atomic update callbacks execute in atomic
context during non-blocking commits, where sleeping is strictly forbidden.
Here, scanout_one and encode_and_send perform multiple sleeping operations:
they lock sleeping Mutexes, allocate memory using GFP_KERNEL, map memory
using drm_gem_fb_vmap (which takes blocking dma_resv locks), and invoke
synchronous USB bulk transfers. Executing these will cause scheduling
while atomic panics and system instability.

[ ... ]

> +fn encode_and_send(
[ ... ]
> +        let (sw, sh) = if rotation_swaps_dims(rotation) { (h, w) } else { (w, h) };
> +        let pitch = sw * 4;

[Severity: High]
Does this calculation ignore the framebuffer pitch?

Userspace can allocate dumb buffers or import DMA-BUFs with padded row
pitches. By assuming a tightly packed buffer and ignoring fb->pitches[0],
the driver will read from incorrect memory offsets for all rows after the
first.

[ ... ]

> +                let px =
> +                    unsafe { (vaddr.add(sy * pitch + sx * 4) as *const u32).read_unaligned() };
> +                let (mut r, mut g, mut b) =
> +                    (((px >> 16) & 0xff) as usize, ((px >> 8) & 0xff) as usize, (px & 0xff) as usize);

[Severity: Medium]
Does this handle pixel extraction correctly on big-endian hosts?

The XRGB8888 DRM format specifies a little-endian byte order. When reading
as a native u32 on a big-endian host, the bytes are interpreted in reverse
semantic order. The subsequent hardcoded bitwise shifts will extract the
wrong color channels. The driver must explicitly perform little-endian
reads.

[ ... ]

> +fn cursor_send(
[ ... ]
> +    let (mx, my) = (
> +        cx.clamp(0, u16::MAX as i32) as u16,
> +        cy.clamp(0, u16::MAX as i32) as u16,
> +    );

[Severity: Medium]
Does this clamping prevent the cursor from smoothly transitioning off the
screen edges?

Negative coordinates are clamped to 0 instead of cropping the source sprite,
which forces the cursor to visually stick to the screen edges in its
entirety, creating a user-visible glitch.

[ ... ]

> +    if !head.cursor_primed.swap(true, core::sync::atomic::Ordering::SeqCst) {
> +        data.send_cp(hid, 0x1b, 0, |ctr| super::cp::cursor_create(ctr, w as u16, h as u16))?;
> +    }

[Severity: Medium]
Could this cause a hardware state mismatch if the cursor is resized?

The one-shot flag prevents cursor_create from being sent again even if
the dimensions change later. If userspace dynamically changes the cursor
framebuffer dimensions across atomic updates, the driver will push a
cursor_image with the new size without informing the dock via
cursor_create.

[ ... ]

> +    let pitch = w * 4;

[Severity: High]
Does this stride calculation ignore the cursor framebuffer pitch?

Just as in the primary plane, calculating the stride from the width
ignores fb->pitches[0] and will read from incorrect memory offsets
if the buffer has padding.

[ ... ]

> +        let primary = head.primary.get();
> +        to_result(bindings::drm_universal_plane_init(
> +            raw,
> +            primary,
> +            0,
> +            data.plane_funcs.get(),

[Severity: Medium]
Will this trigger kernel warnings during probe?

Passing 0 for possible_crtcs when initializing the planes right before
attaching them to the CRTC causes the DRM core to emit a warning
that the primary plane is not configured for the CRTC. The mask is patched
later, but the warning has already fired.

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