Re: [PATCH v4 2/4] media: synopsys: hdmirx: add HDMI audio capture support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The use of `swab32()` when extracting the 20-bit ACR N and CTS values incorrectly scrambles their bits. Because the packet bytes are stored contiguously from the LSB up (PB0 in [7:0], PB1 in [15:8], etc.), reversing the 32-bit word using `swab32()` places the LSB of CTS (PB1) into the MSB of the extracted value, and the MSB of CTS (PB3) into the LSB. Masking this result produces a completely invalid sample rate.

Pre-existing issues:
- [High] The V4L2 `video_device` is embedded in `struct snps_hdmirx_dev`, which is allocated using `devm_kzalloc`. The driver sets `vdev.release = video_device_release_empty`. If a user space process keeps the `/dev/videoX` file descriptor open during device unbind, the `devm` subsystem will free the `snps_hdmirx_dev` memory. When the process later closes the file descriptor, the V4L2 core will access the freed `video_device` to decrement its refcount and call the release callback, causing a use-after-free vulnerability.
--

--- Patch [2]: [PATCH v4 2/4] media: synopsys: hdmirx: add HDMI audio capture support ---
commit 19c9c4db1394f1dc0fc1cb44d51317198a87b078
Author: Igor Paunovic <royalnet026@gmail.com>
Date:   Tue Jul 21 06:41:13 2026 +0000

    media: synopsys: hdmirx: add HDMI audio capture support

This patch adds HDMI audio capture support to the Synopsys DesignWare HDMI RX
controller. It extracts the embedded audio stream and exposes it as an ALSA
capture device via the generic hdmi-codec. It also introduces a periodic
worker to keep the local audio reference clock locked to the source.

> diff --git a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
> index 25f8ca0d6d946..1b4a7fda9e195 100644
> --- a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
> +++ b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c

[ ... ]

> +static u32 hdmirx_audio_fs(struct snps_hdmirx_dev *hdmirx_dev)
> +{
> +	u64 tmds_clk, fs_audio = 0;
> +	u32 acr_cts, acr_n, tmdsqpclk_freq;
> +	u32 acr_pb3_0, acr_pb7_4;
> +
> +	tmdsqpclk_freq = hdmirx_readl(hdmirx_dev, CMU_TMDSQPCLK_FREQ);
> +	hdmirx_readl(hdmirx_dev, PKTDEC_ACR_PH2_1);
> +	acr_pb3_0 = hdmirx_readl(hdmirx_dev, PKTDEC_ACR_PB3_0);
> +	acr_pb7_4 = hdmirx_readl(hdmirx_dev, PKTDEC_ACR_PB7_4);
> +	/*
> +	 * The packet decoder stores the ACR subpacket bytes with packet byte
> +	 * 0 in register bits [7:0], so byte-reverse each word to line the
> +	 * bytes up: CTS is packet bytes 1-3 (PKTDEC_ACR_PB3_0) and N is
> +	 * packet bytes 4-6 (PKTDEC_ACR_PB7_4), 20 bits each. readl()
> +	 * already abstracts the bus endianness, so the reversal is
> +	 * unconditional.
> +	 */
> +	acr_cts = swab32(acr_pb3_0) & 0xfffff;
> +	acr_n = (swab32(acr_pb7_4) & 0x0fffff00) >> 8;

[Severity: High]
Does this byte reversal using swab32() incorrectly scramble the N and CTS
values?

Because the packet bytes are stored contiguously from the LSB up (with packet
byte 0 in [7:0], packet byte 1 in [15:8], etc.), reversing the 32-bit word
using swab32() places the LSB of CTS (from PB1) into the MSB of the extracted
value, and the MSB of CTS (from PB3) into the LSB. 

Will this result in an invalid sample rate being calculated, causing the
driver's fallback logic to silently ignore the broken rate and use 48kHz? This
could cause any audio stream not natively at 48kHz to be played back at the
wrong rate.

> +	tmds_clk = tmdsqpclk_freq * 4 * 1000U;
> +	if (acr_cts != 0) {
> +		fs_audio = div_u64((tmds_clk * acr_n), acr_cts);

[ ... ]

> @@ -2765,6 +3064,10 @@ static int hdmirx_probe(struct platform_device *pdev)
>  						       V4L2_DEBUGFS_IF_AVI, hdmirx_dev,
>  						       hdmirx_debugfs_if_read);
>  
> +	ret = hdmirx_register_audio_device(hdmirx_dev);
> +	if (ret)
> +		dev_warn(dev, "failed to register HDMI audio codec: %d\n", ret);
> +
>  	return 0;

[Severity: High]
This isn't a bug introduced by this patch, but is there a use-after-free risk
during device unbind in this driver?

The V4L2 video_device is embedded in struct snps_hdmirx_dev, which is
allocated using devm_kzalloc() earlier in hdmirx_probe(). However, the driver
sets the release callback to video_device_release_empty in
hdmirx_register_stream_vdev().

If a user space process keeps the /dev/videoX file descriptor open during
device unbind (such as during driver unbind or module removal), the devm
subsystem will free the snps_hdmirx_dev memory. When the process later closes
the file descriptor, will the V4L2 core access the freed video_device memory
to decrement its refcount and call the release callback?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721064115.64809-1-royalnet026@gmail.com?part=2