Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] Memory allocation using GFP_KERNEL in functions designed for atomic context. - [High] Global state used to track per-device control plane engagement. - [High] Integer overflow in `cursor_image` bypasses buffer size validation. - [High] DMA mapping violation by passing stack and `.rodata` buffers to USB transfers. - [High] Premature loop termination in `lockstep_reply` drops valid packets and breaks CP handshake. --
commit ed0df79bff2d18c4e02a6704d484f6b62252ddf5 Author: Mike Lothian <[email protected]> drm/vino: add the AES-CTR/AES-CMAC control-plane seal and arm This commit introduces the AES-CTR/AES-CMAC control-plane for the vino DRM driver. It adds the message builders, cryptographic sealing logic, and a post-SKE setup sequence to engage the dock's control plane. diff --git a/drivers/gpu/drm/vino/cp.rs b/drivers/gpu/drm/vino/cp.rs index 0000000000000..2668931d8500a --- /dev/null +++ b/drivers/gpu/drm/vino/cp.rs [ ... ] > +pub(super) fn cursor_move(counter: u16, head: u8, x: u16, y: u16) -> > Result<KVec<u8>> { > + let mut b = KVec::with_capacity(28, GFP_KERNEL)?; [Severity: High] Is it safe to allocate with GFP_KERNEL here? Functions like cursor_move(), cursor_image(), seal_interactive(), and set_mode() are noted to be used by runtime KMS senders. In the DRM subsystem, cursor updates and plane atomic updates run in atomic context during non-blocking commits, where sleeping is not allowed. Since GFP_KERNEL enables direct reclaim which can sleep, using it in these paths could trigger kernel warnings or deadlocks. > + header(&mut b, 0x1a, 0x43, counter)?; > + pad_to(&mut b, 22)?; > + b.push(head, GFP_KERNEL)?; // off22 head/monitor id > + b.push(1, GFP_KERNEL)?; // off23 flag > + b.extend_from_slice(&x.to_le_bytes(), GFP_KERNEL)?; // off24 > + b.extend_from_slice(&y.to_le_bytes(), GFP_KERNEL)?; // off26 > + Ok(b) > +} > + > +/// cursor image (sec 8.6.1): id=0x1c sub=0x41. Mirrors cursor_create's > header (the > +/// 00 02 00 marker + w,h at off20) and appends the w*h BGRA bitmap. bgra > must be > +/// w*h*4 bytes -- DRM hands the driver a 64x64 ARGB8888 cursor buffer and > the caller swaps > +/// it > +/// to BGRA. The image sub-layout past the create-style header is > capture-unconfirmed (only the > +/// id and the shared header are decoded); re-check against a capture once > CP engages. > +pub(super) fn cursor_image(counter: u16, w: u16, h: u16, bgra: &[u8]) -> > Result<KVec<u8>> { > + if bgra.len() != w as usize * h as usize * 4 { [Severity: High] Can this arithmetic wrap around on 32-bit architectures? The expected buffer size is validated using w as usize * h as usize * 4, where w and h are u16 values. Their maximum product can be ~4.29 billion, which when multiplied by 4 overflows a 32-bit usize. This could allow userspace to specify dimensions like 65535x65535 with a small buffer that matches the wrapped size, bypassing the validation and sending truncated control plane messages. > + return Err(EINVAL); > + } [ ... ] > +pub(super) fn seal_interactive( > + ks: &[u8; 16], > + riv: &[u8; 8], > + id: u16, > + wire_seq: u32, > + content: &[u8], > +) -> Result<KVec<u8>> { > + let body_len = content.len() + 16; // AES-CTR ciphertext + 16-byte > Dl3Cmac > + let size = ((16 + body_len) - 4) as u16; > + let aux = aux_for_id(id, body_len); > + let mut hdr = [0u8; 16]; > + hdr[2..4].copy_from_slice(&size.to_le_bytes()); > + hdr[4..8].copy_from_slice(&4u32.to_le_bytes()); // type=4 > + hdr[8..10].copy_from_slice(&0x24u16.to_le_bytes()); // sub=0x24 > (interactive CP) > + hdr[10..12].copy_from_slice(&aux.to_le_bytes()); > + hdr[12..16].copy_from_slice(&wire_seq.to_le_bytes()); > + seal_livemac(ks, riv, &hdr, content) [Severity: High] Does this wrapper propagate a sleeping allocation into atomic context? seal_interactive() calls seal_livemac(), which allocates KVec buffers using GFP_KERNEL. As this function is also intended for runtime KMS senders, calling it from an atomic commit context could lead to deadlocks. > +} diff --git a/drivers/gpu/drm/vino/vino.rs b/drivers/gpu/drm/vino/vino.rs index db4c38b6dc92f..ef44a625cb70b 100644 --- a/drivers/gpu/drm/vino/vino.rs +++ b/drivers/gpu/drm/vino/vino.rs [ ... ] > +/// Set once the dock has actually engaged the CP cipher (wsub=0x45 acks > > 0). EP08 video is > +/// gated on it: pushing frames at a dock whose CP channel is dead makes it > fault and USB-reset. > +/// NOTE: with the current CP-engagement wall (see the file header) this is > never set on real > +/// 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); [Severity: High] Is it appropriate to use a global static variable for per-device state? The driver tracks the dock's cipher engagement state using the global CP_ENGAGED AtomicBool. Since the kernel needs to support multiple concurrent device instances, sharing this state across devices could create race conditions where one device's successful engagement globally ungates video for all docks, or a failure gates video for an already engaged dock. > + > mod proto; > mod crypto; [ ... ] > + let mut edid_out: Option<KVec<u8>> = None; > + match VinoDriver::send_cp_setup(dev, &session, &mut > edid_out) { > + Ok((n, acks, _wseq_end, _ctr_end)) => { > + dev_info!(cdev, > + "vino: CP setup sent -- {n} messages, > {acks} dock CP acks (wsub=0x45)\n"); > + // CP engagement gates EP08 video (added in > a later patch): until > + // the dock acks, pushing pixels at it > wedges the hub. > + CP_ENGAGED.store(acks > 0, > core::sync::atomic::Ordering::SeqCst); [Severity: High] Does modifying this global static variable here interfere with other concurrent devices? Updating the global CP_ENGAGED state based on a specific device instance's setup outcome could crash other unengaged hubs if video gets incorrectly ungated for them. > + } > + Err(e) => dev_info!(cdev, "vino: CP setup > incomplete ({e:?}) -- WIP\n"), > + } > } [ ... ] > + // Plaintext type=2 sub=0x24+0x45 stream-open arm marker -- the > mandatory gate > + // before the first encrypted frame. > + const STREAM_OPEN: [u8; 64] = [ > + 0x00, 0x00, 0x1c, 0x00, 0x02, 0x00, 0x00, 0x00, // > + 0x24, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, // > + 0x04, 0x00, 0x06, 0x00, 0x00, 0x00, 0x00, 0x00, // > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, // > + 0x00, 0x00, 0x1c, 0x00, 0x02, 0x00, 0x00, 0x00, // > + 0x45, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, // > + 0x05, 0x00, 0x0e, 0x00, 0x00, 0x00, 0x00, 0x00, // > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, // > + ]; [ ... ] > + // Submit the arm marker. Async path: queue it and DO NOT flush -- > leave it in flight so > + // msg0 can be submitted right behind it (the pipelined arm->msg0 > burst DLM does). Sync > + // path: the original blocking send. > + let arm_res = match out_q.as_mut() { > + Some(q) => q.send(&STREAM_OPEN, timeout()), > + None => dev.bulk_send(EP_CTRL_OUT, &STREAM_OPEN, > timeout()).map(|_| ()), > + }; [Severity: High] Does this violate the DMA mapping rules by passing .rodata to a USB transfer? The STREAM_OPEN buffer is a const array located in .rodata. The Linux DMA API forbids using .rodata for DMA transfers because these memory regions cannot be safely mapped when the IOMMU is enabled. > + if let Err(e) = arm_res { > + pr_err!("vino: CP stream-open marker FAILED ({e:?})\n"); > + return Err(e); > + } [ ... ] > + // DLM sends the 0x24 wValue=0 render/commit vendor request right > after msg0. > + match dev.control_send(0x24, 0x40 /* VENDOR_OUT */, 0, 0, &[], > timeout()) { > + Ok(()) => pr_info!("vino: post-msg0 0x24(wValue=0) OK\n"), > + Err(e) => pr_info!("vino: post-msg0 0x24(wValue=0) non-fatal > ({e:?})\n"), > + } > + // DLM then re-reads the 0x22 vendor state (0xc1, wValue=1, > wIndex=0, 28 B) -- its SECOND > + // 0x22 of the session, immediately after the post-msg0 0x24. vino > issued the first 0x22 > + // pre-arm but stopped here, leaving "DLM-ONLY 0x22" in the paired > diff. Issue it > + // unconditionally so the wire matches DLM regardless of whether the > dock acks; it is a > + // harmless vendor IN read. (0xc1 = IN|vendor|INTERFACE recipient, > matching the first 0x22.) > + let mut state2 = [0u8; 28]; > + match dev.control_recv(0x22, 0xc1, 1, 0, &mut state2, timeout()) { [Severity: High] Does this violate the DMA mapping rules by passing a stack array to a USB transfer? The state2 buffer is a local array located on the stack. The Linux DMA API forbids using stack memory for DMA transfers because these regions cannot be safely mapped when CONFIG_VMAP_STACK or the IOMMU are enabled. > + Ok(()) => pr_info!("vino: post-msg0 0x22(wValue=1) OK = > {:02x?}\n", state2), > + Err(e) => pr_info!("vino: post-msg0 0x22(wValue=1) non-fatal > ({e:?})\n"), > + } [ ... ] > + fn lockstep_reply( > + dev: &usb::Device, > + mut q: Option<&mut usb::BulkInQueue>, > + buf: &mut [u8], > + session: &Session, > + ictr: u16, > + edid_out: &mut Option<KVec<u8>>, > + ) -> (usize, usize, bool) { > + const MAX_READS: usize = 8; > + let in_riv = cp::in_riv(&session.riv); > + let mut reads = 0usize; > + let mut acks = 0usize; > + let mut matched = false; > + for _ in 0..MAX_READS { > + match Self::read_ep84(dev, q.as_deref_mut(), buf, > Delta::from_millis(30)) { > + Ok(len) if len > 16 => { > + reads += 1; > + // sec 5 diagnostic: log every frame the dock returns in > the lockstep > + // window -- including the non-0x45 frames we otherwise > skip -- so the > + // divergence point is paired with the dock's actual > reply on the wire. > + Self::log_ep84(session, &buf[..len]); > + if u16::from_le_bytes([buf[8], buf[9]]) != 0x45 { > + continue; > + } > + acks += 1; > + let seq = u32::from_le_bytes([buf[12], buf[13], buf[14], > buf[15]]); > + // Decrypt just the first block to read the inner > counter (off 4). > + let head = &buf[16..len.min(32)]; > + if let Ok(inner) = cp::open_in(&session.ks, &in_riv, > seq, head) { > + if inner.len() >= 6 > + && u16::from_le_bytes([inner[4], inner[5]]) == > ictr > + { > + matched = true; > + } > + } > + // Opportunistically capture the EDID (id=0x94 reply, > off 22). > + if edid_out.is_none() { > + if let Ok(Some(e)) = > + cp::parse_edid_from_reply(&session.ks, > &session.riv, &buf[..len]) > + { > + pr_info!("vino: EDID read from dock ({} > bytes)\n", e.len()); > + *edid_out = Some(e); > + } > + } > + if matched { > + break; > + } > + } > + _ => break, [Severity: High] Does this premature loop termination drop valid packets? If the dock emits a legitimate short packet (such as a ZLP or a small protocol ACK where len <= 16), the loop breaks immediately instead of continuing to read. This drops the polling window and guarantees the driver will miss the subsequent sub=0x45 reply, incorrectly treating the dock's CP initialization as failed. > + } > + } > + (reads, acks, matched) > + } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
