Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Use of sleeping `GFP_KERNEL` memory allocations and blocking USB operations in atomic KMS callbacks. - [High] Constant mismatch in `aux_for_id` causes fallback misparsing for `cursor_image` messages. - [Medium] The `seal` function fails to strip the stale MAC tag and append a fresh one, contradicting its explicit documentation. --
commit 399a257fc41428dee57be4f93ca9bcdacd1d9785 Author: Mike Lothian <[email protected]> drm/vino: add the AES-CTR/AES-CMAC control-plane seal and arm sequence This commit implements the post-AKE control-plane wire format for the vino driver. It includes the AES-CTR seal, the Dl3Cmac trailer, the stream-open arm marker, and message builders for mode-set, cursor, and DDC/CI. > diff --git a/drivers/gpu/drm/vino/cp.rs b/drivers/gpu/drm/vino/cp.rs > new file mode 100644 > index 0000000000000..4d55b96999b02 > --- /dev/null > +++ b/drivers/gpu/drm/vino/cp.rs [ ... ] > +pub(super) fn cursor_image( > + counter: u16, > + head: u8, > + w: u16, > + h: u16, > + bgra: &[u8], > +) -> Result<KVec<u8>> { > + // w*h*4 can wrap a 32-bit usize (max ~1.7e10 > u32::MAX), which would > let an > + // undersized bgra pass the check; compute it with checked arithmetic so > an > + // overflow is rejected as a mismatch rather than silently bypassing > validation. > + let expected = (w as usize) > + .checked_mul(h as usize) > + .and_then(|n| n.checked_mul(4)); > + if expected != Some(bgra.len()) { > + return Err(EINVAL); > + } > + let mut b = KVec::with_capacity(32 + bgra.len(), GFP_KERNEL)?; [Severity: High] Does this allocate memory using a sleeping flag (GFP_KERNEL) while in an atomic context? When the compositor requests a frame flip or a cursor update, the DRM core invokes the VinoPlane::atomic_update callback (which runs in an atomic context). This atomic callback executes cp::cursor_image, which allocates memory dynamically using GFP_KERNEL. > + cursor_header(&mut b, 0x401c, 0x41, counter, head)?; [Severity: High] Will the 0x40 high-byte flag on this cursor image ID cause a mismatch in aux_for_id()? In aux_for_id(), we check for 0x1c for the cursor image: 0x1a => 0x04, // cursor move 0x1b => 0x03, // cursor create 0x1c => 0x02, // cursor image 0x1f => 0x0f, Because aux_for_id() strictly checks for 0x1c instead of 0x401c, it will fall through to the default arm and incorrectly return (body_len / 4) as u16, rather than the expected 0x02 length marker. > + pad_to(&mut b, 32)?; // off24..31 zero (no w/h here) > + b.extend_from_slice(bgra, GFP_KERNEL)?; // bitmap @ off32 > + Ok(b) > +} [ ... ] > +pub(super) fn seal( > + ks: &[u8; 16], > + riv: &[u8; 8], > + seq: u32, > + inner: &[u8], > +) -> Result<KVec<u8>> { > + // The interactive CP stream: session ks, wire sub 0x24. > + seal_stream(ks, riv, 0x24, seq, inner) [Severity: Medium] Is the stale MAC tag properly stripped here? The documentation for seal() explicitly mentions that we should encrypt only content = inner[..len-16] and append a fresh tag. However, the implementation delegates to a generic stream encrypter (seal_stream) on the entire inner buffer, which ignores the requirement to slice off the stale tag and append a live MAC. > +} [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
