Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] The driver encrypts `stream_open` messages for unoccupied connectors using a randomly generated video key that is never transmitted to the dock, permanently breaking hotplug for those connectors. - [High] EDID crosstalk in Navarro discovery due to missing wait loop for asynchronous replies, causing one connector to claim another's EDID. - [High] AES-CTR keystream reuse and control protocol desynchronization when an EDID transmission times out. --
commit fa0367d27511540d9b322089cdf1062994527dcc Author: Mike Lothian <[email protected]> drm/vino: add control-session bring-up Bringing a dock up runs once per bind, in order: a plaintext initialisation preamble, the link AKE, then the sealed setup burst that authenticates each downstream connector, transitions the video engine and reads back what the dock will answer. > diff --git a/drivers/gpu/drm/vino/session/setup.rs > b/drivers/gpu/drm/vino/session/setup.rs > new file mode 100644 > index 0000000000000..b278ebcf4b4e5 > --- /dev/null > +++ b/drivers/gpu/drm/vino/session/setup.rs [ ... ] > + if i >= 3 && !rrx_applied { > + let Some(rrx_h) = fresh_rrx else { > + // No `rrx` means this connector never began a > downstream authentication, [ ... ] > + vino_debug!( > + "vino: socket {socket} has no downstream sink > (no AKE_Send_Rrx); skipping its authentication\n" > + ); > + Self::announce_stream(link, &mut out_q, profile, > connector)?; > + continue 'per_head; > + }; [ ... ] > + for connector in 0..connector_count { > + > settle_polls!(profile.protocol.setup_polls.before_open(connector as u8)); > + let stream_id = profile.geometry().stream_id(connector as > u8); > + let mut vkey = kernel::crypto::Secret::<16>::zeroed(); > + vkey.copy_from_slice(&video_keys[connector][..16]); > + let mut vnonce = [0u8; 8]; > + vnonce.copy_from_slice(&video_keys[connector][16..24]); > + let content = > cp::stream_open(profile.protocol.stream_marker_kind); > + let open = cp::seal_video_arm(&vkey, &vnonce, stream_id, > 0x000a, 0, &content)?; > + link.ctrl_send(&open, timeout(), GFP_KERNEL)?; [Severity: High] Will this break hotplug functionality for unoccupied connectors on DL-3x00 docks? When initializing a dock with an unplugged connector (where video_on_ctrl_pipe is true), the per_head loop aborts early via continue 'per_head. This means SKE_Send_Eks is skipped and the generated video key is never sent to the dock. Later in send_cp_setup(), the driver attempts to open the stream using this same unsent, randomly generated key to encrypt the stream_open message. The dock won't be able to decrypt this message, causing the stream to fail to open, which leads to permanent black screens if a monitor is subsequently plugged in. [ ... ] > + for connector in 0..connector_count { [ ... ] > + let fetch = cp::get_edid_req(cp_ctr, hu8)?; > + navarro_send!(0x15, fetch); > + // The fetch drain carries the asynchronous EDID in the > working DLM cadence. > + edid_connectors[connector] = edid_out.take(); > + discovery_deferred[connector] = > edid_connectors[connector].is_none(); > + } [Severity: High] Is there a risk of EDID crosstalk here if the downstream panel's DDC read takes longer than the 10ms timeout used in the navarro_send! macro? During Navarro EDID discovery, if the fetch times out (yielding no EDID), the loop advances to the next connector and issues a readiness kick. While draining responses for the new connector's commands, the delayed EDID reply from the previous connector could arrive. Because drain_ep84() unconditionally assigns any parsed EDID to edid_out without checking the connector ID, the current connector might mistakenly claim the delayed EDID from the previous one, causing monitors to be identified incorrectly. [ ... ] > + for connector in 0..connector_count { [ ... ] > + 'discovery: { > + macro_rules! edid_send { > + ($ep:expr, $body:expr, $tag:expr) => {{ > + match Self::send_live_cp( > + link, > + session, > + ep84_q.as_mut(), > + &mut resp, > + edid_out, > + $ep, > + wseq, > + &$body, > + ) { > + Ok(e) => { [ ... ] > + } > + Err(e) => { > + transport_error = Some(e); > + break 'discovery; > + } > + } > + }}; > + } [Severity: High] Does this error path lead to AES-CTR keystream reuse and control protocol desynchronization? If send_live_cp() fails (e.g., due to a USB timeout) during Ridge EDID discovery, the edid_send! macro sets transport_error and breaks out of the 'discovery block. This skips the increments for both the AES-CTR block counter (wseq) and the message counter (cp_ctr). When the outer loop advances to the next connector, it encrypts the new request using the exact same wseq and cp_ctr. This reuse breaks cryptographic confidentiality. Additionally, if the dock successfully received the initial message but the host timed out waiting for the ACK, the dock will reject the next message due to the reused sequence numbers, permanently breaking the session. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
