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