Re: [PATCH v3 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:
- [High] Incorrect byte-swapping logic completely scrambles the extraction of Audio Clock Regeneration (ACR) CTS and N values, breaking sample rate recovery on little-endian architectures.
- [High] The `audio_work` delayed work is not explicitly cancelled in `hdmirx_remove()`, leading to a Use-After-Free if the driver is unbound while an audio stream is active.
--

--- Patch [2]: [PATCH v3 2/4] media: synopsys: hdmirx: add HDMI audio capture support ---
commit b04bcd22f40b424aac824100715c46f764531875
Author: Igor Paunovic <royalnet026@gmail.com>

media: synopsys: hdmirx: add HDMI audio capture support

The Synopsys DesignWare HDMI RX controller extracts the audio stream
embedded in the incoming HDMI signal and feeds it to an on-SoC I2S
controller. Expose it as an ALSA capture device by registering the
generic hdmi-codec as a child of the controller.

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

[ ... ]

> +/* Recover the incoming audio sample rate from the ACR N/CTS + TMDS clock. */
> +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-swap 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.
> +	 */
> +	acr_cts = be32_to_cpu((__force __be32)acr_pb3_0) & 0xfffff;
> +	acr_n = (be32_to_cpu((__force __be32)acr_pb7_4) & 0x0fffff00) >> 8;

[Severity: High]
Does this byte-swapping logic scramble the extraction of the CTS and N values
on little-endian architectures?

Since the HDMI ACR packet transmits the 20-bit CTS value across PB0, PB1, and
PB2, and because it is little-endian, applying a bitmask like
acr_pb3_0 & 0xfffff natively yields the correct CTS value without any swaps.

By byte-swapping the 32-bit register, the bytes are completely reversed.
The subsequent bitmask extracts the lower 20 bits of the swapped word, which
correspond to the lower nibble of PB1, all of PB2, and all of PB3, discarding
PB0 entirely.

The same applies to acr_pb7_4 to extract N. Will this regression result in
garbage values for both acr_cts and acr_n, forcing the driver to blindly
fall back to the default sample rate?

[ ... ]

> @@ -2784,6 +3052,9 @@ static void hdmirx_remove(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct snps_hdmirx_dev *hdmirx_dev = dev_get_drvdata(dev);
>  
> +	if (hdmirx_dev->audio_pdev)
> +		platform_device_unregister(hdmirx_dev->audio_pdev);
> +

[Severity: High]
Can this lead to a use-after-free if the driver is unbound while an audio
stream is active?

The v3 changelog notes that the synchronous cancel for the audio_work
delayed work was removed here under the assumption that audio_shutdown()
handles it.

However, ALSA does not synchronously invoke the PCM .shutdown callback
when a device is unregistered if a userspace application holds the file
descriptor open.

Because hdmirx_dev is allocated using devm_kzalloc, it is freed immediately
after hdmirx_remove() returns. Will the un-cancelled audio_work delayed work
subsequently fire and access the freed hdmirx_dev memory?

>  	v4l2_debugfs_if_free(hdmirx_dev->infoframes);
>  	debugfs_remove_recursive(hdmirx_dev->debugfs_dir);

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