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

[Igor Paunovic]

> Does this byte reversal using swab32() incorrectly scramble the N and
> CTS values?

No. This is the third review of this finding -- it was raised on v2
and v3 in the be32_to_cpu() spelling and answered both times, and the
comment above the code documents the layout. The premise doesn't
match the ACR subpacket format: CTS is not stored LSB-first from PB1.
Per the HDMI spec, subpacket byte 0 is reserved, CTS[19:16] sits in
PB1[3:0], CTS[15:8] in PB2 and CTS[7:0] in PB3 -- MSB first -- with N
likewise in PB4..PB6. The packet decoder stores PB0 in register bits
[7:0], so reversing the word is precisely what lines the bytes up
before the 20-bit masks. swab32() and the previous
be32_to_cpu()/cpu_to_be32 spelling produce the same value on both
endiannesses; the v4 change only makes the reversal unconditional.

It also remains the path validated on hardware, as noted in the
cover letter: a 44.1 kHz source is detected as 44.1 kHz rather than
falling back to the 48 kHz default, and 44.1/48 kHz switches are
followed. Neither would work if the extraction produced garbage.

> is there a use-after-free risk during device unbind in this driver?
> [video_device embedded in devm-allocated struct, release set to
> video_device_release_empty]

This one looks structurally plausible, but as the report itself
notes it predates this series: it concerns the video_device lifetime
on the V4L2 side and is independent of the audio support added here
(the audio teardown path was exercised on unbind with an active
stream during v3 review, see the v3 thread). Addressing it would be
a separate fix for the existing driver -- happy to leave that call
to Dmitry.

Igor

On Tue, Jul 21, 2026 at 9:00 AM <sashiko-bot@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxx>
> 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@xxxxxxxxx?part=2