Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
New issues: - [High] The patch introduces a `gamma` field protected by a `kernel::sync::Mutex` and locks it from within DRM atomic commit callbacks (`atomic_enable`, `atomic_disable`, `atomic_update`), which execute in atomic context where sleeping is forbidden. - [High] The DDC/CI payload length is silently truncated if it exceeds 255 bytes, leading to malformed CP packets. - [Medium] Dynamic `GAMMA_LUT` property updates are silently ignored while the display is active. - [Medium] The rotation property is missing on the cursor plane and the rotation is completely ignored when reading cursor pixels. - [Medium] The DDC/CI I2C adapter's `master_xfer` incorrectly skips unsupported messages and transmission failures instead of returning an error, breaking sequential processing guarantees. - [Low] The import `i2c` does not follow the kernel vertical import style. Pre-existing issues: - [Critical] Missing plane scaling rejection allows userspace to trigger out-of-bounds kernel memory reads during scanout. - [High] The `ddc_forward` function allocates memory using `GFP_KERNEL` when called from the `atomic_disable` DRM callback, violating atomic context rules. -- commit efd9ec1e849fe00fb25b5f1bc3faedd48ed3e0f5 Author: Mike Lothian <[email protected]> drm/vino: wire CRTC gamma, plane rotation and DDC/CI monitor controls This commit consumes the new gamma, rotation, and i2c bindings for the vino driver. It enables a 256-entry GAMMA_LUT, adds a rotation property on the primary/cursor plane, and registers a virtual I2C adapter for DDC/CI controls. > diff --git a/drivers/gpu/drm/vino/cp.rs b/drivers/gpu/drm/vino/cp.rs > --- a/drivers/gpu/drm/vino/cp.rs > +++ b/drivers/gpu/drm/vino/cp.rs [ ... ] > @@ -151,14 +151,24 @@ pub(super) fn set_mode(counter: u16, t: &Timing) -> > Result<KVec<u8>> { > /// the CP wall), so re-check against a capture once CP engages. The DDC/CI > bytes themselves > /// ([`ddc_ci_set_vcp`]) are standard and verified. > pub(super) fn ddc_set_vcp(counter: u16, vcp: u8, value: u16) -> > Result<KVec<u8>> { > - let payload = ddc_ci_set_vcp(vcp, value); > - let mut b = KVec::with_capacity(32, GFP_KERNEL)?; > + ddc_forward(counter, DDCCI_I2C_ADDR, &ddc_ci_set_vcp(vcp, value)) > +} > + > +/// The DDC/CI I2C slave address on the monitor bus. > +pub(super) const DDCCI_I2C_ADDR: u8 = 0x37; > + > +/// CP message tunnelling a raw DDC/CI transaction (`payload`, the bytes a > host writes after the > +/// I2C write-address) to the downstream monitor's I2C slave `addr`. Same > `id=0x15 sub=0x22` > +/// framing as [`ddc_set_vcp`], with the slave + payload length at off20 and > the payload at off22. > +/// This is what the vino I2C adapter forwards for a userspace DDC/CI write > (e.g. `ddcutil`). > +pub(super) fn ddc_forward(counter: u16, addr: u8, payload: &[u8]) -> > Result<KVec<u8>> { > + let mut b = KVec::with_capacity(32 + payload.len(), GFP_KERNEL)?; [Severity: High] This is a pre-existing issue, but can this GFP_KERNEL allocation cause a scheduling while atomic panic? The CRTC atomic_disable callback runs in atomic context where sleeping is forbidden. It calls set_vcp(), which delegates to cp::ddc_set_vcp() and subsequently cp::ddc_forward(). Allocating with GFP_KERNEL here can sleep. Could this allocation use GFP_ATOMIC instead? > header(&mut b, 0x15, 0x22, counter)?; > pad_to(&mut b, 20)?; > - // off20: monitor DDC/CI I2C slave (0x37) + DDC/CI payload length. > - b.extend_from_slice(&[0x37, payload.len() as u8], GFP_KERNEL)?; > - // off22: the DDC/CI Set-VCP bytes (same off22 convention as the EDID > payload). > - b.extend_from_slice(&payload, GFP_KERNEL)?; > + // off20: monitor DDC/CI I2C slave + DDC/CI payload length. > + b.extend_from_slice(&[addr, payload.len() as u8], GFP_KERNEL)?; [Severity: High] Can a payload larger than 255 bytes cause malformed CP packets here? The payload.len() is cast to u8 without bounds checking, meaning the protocol packet header receives a truncated length, but the full unbounded payload is appended below. Could this cause the downstream dock's CP parser to read misaligned framing, potentially hanging the hardware or corrupting the stream? > + // off22: the DDC/CI bytes (same off22 convention as the EDID payload). > + b.extend_from_slice(payload, GFP_KERNEL)?; > pad_to(&mut b, 32)?; > Ok(b) > } > diff --git a/drivers/gpu/drm/vino/drm_sink.rs > b/drivers/gpu/drm/vino/drm_sink.rs > --- a/drivers/gpu/drm/vino/drm_sink.rs > +++ b/drivers/gpu/drm/vino/drm_sink.rs [ ... ] > @@ -41,6 +42,7 @@ > plane::{self, PlaneAtomicCommit, RawPlaneState as _}, > KmsDriver, ModeConfigGuard, ModeConfigInfo, ModeObject as _, > NewKmsDevice, Probing, > }, > + i2c, > + error::code::EINVAL, [Severity: Low] Does this import follow the kernel vertical import style for Rust? The i2c import is placed on the same line as other items instead of being vertically aligned. [ ... ] > @@ -125,6 +127,12 @@ pub(super) struct VinoDrmData { > /// can reach its cached-EDID slot without needing DRM's mode-object > list. Written once, > /// during single-threaded probe, before the device is registered; > read-only thereafter. > connector: core::sync::atomic::AtomicPtr<VinoConnector>, > + /// The CRTC's gamma ramp cached from the atomic hook as three 256-entry > 8-bit LUTs > + /// (`[r; 256] ++ [g; 256] ++ [b; 256]`), or `None` for identity. Cached > here (not read from > + /// the CRTC state) because scanout runs in the plane path; it is > `Copy`, so the scanout > + /// snapshots it under the lock and applies it without holding the lock > in the pixel loop. > + #[pin] > + gamma: Mutex<Option<[u8; 768]>>, > } > > impl VinoDrmData { [Severity: High] Does the use of a sleeping Mutex here violate atomic context rules? The update_gamma() and gamma_snapshot() functions lock this Mutex, but they are invoked synchronously during the CRTC atomic_enable, atomic_disable, and plane atomic_update callbacks. According to DRM subsystem guidelines, these callbacks run in atomic context during non-blocking commits where sleeping is strictly forbidden. Should this be a spinlock instead to avoid scheduling while atomic panics? [ ... ] > @@ -302,6 +333,16 @@ fn probe(dev: &NewKmsDevice<'_, Self, Probing>) -> > Result { > None, > false, > )?; > + // Advertise the rotations vino's re-encode handles 1:1 > (dimension-preserving): the > + // scanout applies them per source pixel via `rot_src`. 90/270 > (which swap width/height) > + // are intentionally omitted. > + primary.create_rotation_property( > + bindings::DRM_MODE_ROTATE_0, > + bindings::DRM_MODE_ROTATE_0 > + | bindings::DRM_MODE_ROTATE_180 > + | bindings::DRM_MODE_REFLECT_X > + | bindings::DRM_MODE_REFLECT_Y, > + )?; > let cursor = plane::UnregisteredPlane::<VinoPlane>::new( > dev, > 1, > &CURSOR_FORMATS, > None, > plane::Type::Cursor, > None, > PlaneArgs { head: 0, is_cursor: true }, > )?; [Severity: Medium] Is the cursor plane missing the rotation property? The commit message mentions wiring rotation for the primary/cursor plane, but create_rotation_property() is not called for the cursor plane during probe. Additionally, read_cursor_bgra() maps and reads the cursor framebuffer without applying any rotation transformation. [ ... ] > @@ -372,6 +415,8 @@ fn new(_device: &drm::Device<Self::Driver, drm::Uninit>, > _args: &()) -> impl Pin > fn atomic_enable(commit: CrtcAtomicCommit<'_, Self>) { > let data: &VinoDrmData = commit.crtc().drm_dev(); > let new = commit.take_new_state(); > + // Cache the gamma ramp for the scanout to apply. > + data.update_gamma(new.gamma_lut()); [Severity: Medium] Are dynamic GAMMA_LUT property updates silently ignored while the display is active? The driver caches the GAMMA_LUT value only in the atomic_enable and atomic_disable CRTC hooks. If a compositor dynamically updates the gamma LUT, it will trigger atomic_flush or atomic_update, but skip the enable hook when the CRTC is already active, potentially missing color calibration changes until a full modeset occurs. [ ... ] > @@ -489,6 +535,8 @@ fn atomic_update(commit: PlaneAtomicCommit<'_, Self>) { > // 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: Critical] This isn't a bug introduced by this patch, but does the lack of plane scaling rejection allow userspace to trigger out-of-bounds kernel memory reads? VinoPlane lacks an atomic_check function to reject scaling (e.g., using drm_atomic_helper_check_plane_state with DRM_PLANE_NO_SCALING). In atomic_update(), the scanout loop uses the destination CRTC geometry (new.crtc_w(), new.crtc_h()) as the bounds instead of the source framebuffer dimensions. If unprivileged userspace issues an atomic commit with crtc_h greater than fb.height(), the scanout loop will read past the end of the mapped GEM buffer. [ ... ] > @@ -748,13 +816,62 @@ fn get_modes<'a>( > } > } > > +// ---- DDC/CI I2C adapter > ----------------------------------------------------- > + > +/// vino's DDC/CI virtual I2C bus: a userspace monitor-control tool > (`ddcutil`, the desktop > +/// brightness slider via the I2C DDC path) writes a DDC/CI transaction to > the monitor's I2C slave > +/// on this adapter, and vino tunnels it to the downstream monitor over the > dock's CP channel > +/// (`cp::ddc_forward`, `id=0x15 sub=0x22`) -- the same monitor-I2C bridge > vino's DPMS-power VCP > +/// uses. Writes only for now (Get-VCP reads need the CP reply path); a > no-op until CP engages. > +pub(super) struct VinoI2c; > + > +impl i2c::BusController for VinoI2c { > + type Context = ARef<VinoDrmDevice>; > + > + fn master_xfer(dev: &ARef<VinoDrmDevice>, msgs: &mut [i2c::Msg]) -> > Result<usize> { > + let data: &VinoDrmData = dev; > + let mut transferred = 0usize; > + for msg in msgs.iter() { > + if msg.addr() != super::cp::DDCCI_I2C_ADDR as u16 { > + continue; > + } > + if msg.is_read() { > + // DDC/CI reads (Get-VCP) require decoding the dock's CP > reply -- not wired yet. > + continue; > + } [Severity: Medium] Should master_xfer() return an error when encountering unsupported messages rather than skipping them? If master_xfer() receives an array of messages where an early message is unsupported (e.g., a read) and a later message succeeds, it uses continue to skip the error and returns a positive transferred count. This violates the I2C subsystem API contract and causes userspace tools like ddcutil to assume operations succeeded when they actually failed. > + if data > + .send_cp(0x15, 0, |ctr| { > + super::cp::ddc_forward(ctr, super::cp::DDCCI_I2C_ADDR, > msg.buf()) > + }) > + .is_ok() > + { > + transferred += 1; > + } > + } > + Ok(transferred) > + } [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=8
