Re: [PATCH v3 7/13] drm/vino: add control-session bring-up

[sashiko-bot@kernel.org]

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 <mike@fireburn.co.uk>

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/20260826163913.7052-1-mike@fireburn.co.uk?part=7