Re: [RFC PATCH v2 05/10] drm/vino: add the DRM/KMS sink, built on the safe KMS mode-object layer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Sleeping in atomic context within KMS atomic callbacks (Subsystem Guide Violation).
- [High] Out-of-bounds kernel memory read in framebuffer scanout due to unvalidated destination dimensions.
- [High] Integer overflow in scanout buffer size calculation on 32-bit architectures leads to out-of-bounds writes.
- [High] Device-specific initialization and recovery states are tracked in global variables, breaking multi-device support.
- [Medium] Incorrect pixel color extraction on big-endian architectures due to native-endian memory reads.
--

commit 717e075a35f786d4dc6a24d62c35ac5c6ff9090a
Author: Mike Lothian <mike@fireburn.co.uk>

drm/vino: add the DRM/KMS sink, built on the safe KMS mode-object layer

This commit introduces the DRM/KMS sink for the vino driver using the safe
KMS mode-object layer. It wires up a CRTC, a primary plane, a virtual
encoder, and a connector to support mode-setting and scanning out
framebuffers to the dock.

> diff --git a/drivers/gpu/drm/vino/drm_sink.rs b/drivers/gpu/drm/vino/drm_sink.rs
> new file mode 100644
> index 0000000000000..450c7c84a0677
> --- /dev/null
> +++ b/drivers/gpu/drm/vino/drm_sink.rs

[ ... ]

> +    fn atomic_enable(commit: CrtcAtomicCommit<'_, Self>) {
> +        let data: &VinoDrmData = commit.crtc().drm_dev();
> +        let new = commit.take_new_state();
> +        let timing = super::cp::timing_from_drm_mode(new.mode());
> +        pr_info!(
> +            "vino: KMS CRTC enable -- display ON, mode {}x{}@{} (scanout begins)\n",
> +            timing.hactive,
> +            timing.vactive,
> +            timing.refresh_hz
> +        );
> +        if let Err(e) = data.send_cp(0x48, 16, |ctr| super::cp::set_mode(ctr, &timing)) {

[Severity: High]
Does send_cp() perform sleeping operations?

According to DRM subsystem guidelines, CRTC atomic callbacks execute in an
atomic context where sleeping is strictly forbidden.

Since send_cp() acquires a Mutex and performs a blocking USB transfer with
GFP_KERNEL, could this cause a "scheduling while atomic" issue during
display enable?

> +            pr_warn!("vino: runtime mode-set send failed ({e:?})\n");
> +        }
> +        let _ = data.set_vcp(super::cp::VCP_POWER_MODE, super::cp::POWER_ON);
> +    }

[ ... ]

> +    fn atomic_update(commit: PlaneAtomicCommit<'_, Self>) {

[Severity: High]
Are we performing sleeping operations in an atomic context here?

The Plane atomic update callback synchronously invokes the frame conversion
and USB scanout pipeline.

This function eventually calls encode_and_send(), which locks a Mutex,
allocates memory with GFP_KERNEL, and blocks on USB bulk transfers. Will
this lead to system instability during mode-sets or page-flips?

> +        if !super::CP_ENGAGED.load(core::sync::atomic::Ordering::SeqCst) {
> +            return;
> +        }
> +        let plane = commit.plane();
> +        let data: &VinoDrmData = plane.drm_dev();
> +        let new = commit.take_new_state();
> +        let Some(fb) = new.framebuffer::<VinoDrmDriver>() else { return };
> +        // The plane's destination geometry mirrors the negotiated mode (the compositor sizes the
> +        // primary plane 1:1 with the virtual output), so this drives the dynamic scanout
> +        // resolution.
> +        let (w, h) = (new.crtc_w() as usize, new.crtc_h() as usize);

[Severity: High]
Could these destination dimensions exceed the bounds of the underlying
source framebuffer?

These values come from the userspace-controlled plane destination size.
Because there is no atomic_check to validate them against the attached
framebuffer size, could the pixel extraction loops further down read out of
bounds memory?

> +
> +        use core::sync::atomic::Ordering::Relaxed;
> +        // Throttle: while scanout is failing (dock NAKing because CP isn't engaged), skip the
> +        // upcoming pageflips set by the backoff below instead of converting+encoding+sending a
> +        // frame the dock will just drop.
> +        let skip = super::SCANOUT_SKIP.load(Relaxed);

[Severity: High]
Can tracking frame backoffs in a global static cause interference between
multiple connected devices?

If multiple docks are in use, they would share these error recovery states
and potentially affect each other's scanout rate.

[ ... ]

> +fn prime_video_eps(dev: &super::usb::Interface<kernel::device::Bound>) {
> +    if !super::EP08_SCANOUT_PRIMED.swap(true, core::sync::atomic::Ordering::SeqCst) {

[Severity: High]
Does using a global static here prevent multiple connected docks from
initializing properly?

If a second dock is connected, it appears it would see this global flag as
true and skip clearing the halts on its video endpoints. Could this state
be tracked per-device instead?

> +        for ep in [0x08u8, 0x0a, 0x0b, 0x0c] {
> +            let _ = dev.clear_halt(ep);
> +        }

[ ... ]

> +fn encode_and_send(
> +    data: &VinoDrmData,
> +    plane: &plane::Plane<VinoPlane>,
> +    vaddr: *const u8,
> +    pitch: usize,
> +    w: usize,
> +    h: usize,
> +) -> Result {

[ ... ]

> +        let first = st.enc.is_none() || st.dims != (w, h);
> +        if first {
> +            st.enc = Some(super::video::Encoder::new(w, h, super::video::Mode::Rle)?);
> +            st.cur = VVec::from_elem(0u16, w * h, GFP_KERNEL)?;

[Severity: High]
Can this size calculation silently overflow on 32-bit architectures?

If userspace passes large values for crtc_w and crtc_h, w * h could wrap
around the 32-bit usize limit.

This would result in an undersized allocation, causing the nested loops
below to write out-of-bounds to the heap.

> +            st.dims = (w, h);
> +            st.hint = 0;
> +        }
> +        let ScanoutState { enc, cur, seq, hint, dims: _ } = &mut *st;
> +        for dy in 0..h {
> +            for dx in 0..w {
> +                // SAFETY: `dy*pitch + dx*4 + 3` is within the mapped source framebuffer
> +                // (`pitch*h` bytes); `dx < w <= pitch/4`, `dy < h`.
> +                let px = unsafe { (vaddr.add(dy * pitch + dx * 4) as *const u32).read_unaligned() };
> +                let (r, g, b) = ((px >> 16) & 0xff, (px >> 8) & 0xff, px & 0xff);

[Severity: Medium]
Will this color channel extraction work correctly on big-endian
architectures?

XRGB8888 has a fixed byte order in memory. By reading it as a native u32
and applying bitwise shifts, the bytes will be loaded in reverse order on a 
big-endian system, which might cause the wrong color channels to be
extracted.

> +                cur[dy * w + dx] = (((r >> 3) << 11) | ((g >> 2) << 5) | (b >> 3)) as u16;
> +            }
> +        }

[ ... ]

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