Re: [RFC PATCH v2 03/10] drm/vino: add the AES-CTR/AES-CMAC control-plane seal and arm sequence

[sashiko-bot@kernel.org]

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

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