Re: drm/bridge: tc358767: AUX read uses an unclamped device length

[Doug Anderson]

Hi,

On Wed, Jun 24, 2026 at 3:53 AM Maoyi Xie <maoyixie.tju@xxxxxxxxx> wrote:
>
> Hi all,
>
> I think tc_aux_transfer() in drivers/gpu/drm/bridge/tc358767.c can write
> out of bounds on an AUX read. It uses the byte count the hardware reports
> as the copy length, without clamping it back to the requested size. I
> would appreciate it if you could take a look.
>
> tc_aux_transfer() first clamps the request to the AUX payload limit:
>
>         size_t size = min_t(size_t, DP_AUX_MAX_PAYLOAD_BYTES - 1, msg->size);
>
> After the transfer it overwrites size with the count the hardware reports:
>
>         if (size)
>                 size = FIELD_GET(AUX_BYTES, auxstatus);
>
> AUX_BYTES is GENMASK(15, 8). It can be up to 255. Nothing clamps it again
> before the read:
>
>         if (size)
>                 return tc_aux_read_data(tc, msg->buffer, size);
>
> tc_aux_read_data() reads that many bytes into a fixed stack buffer and
> then copies them into the caller buffer:
>
>         u32 auxrdata[DP_AUX_MAX_PAYLOAD_BYTES / sizeof(u32)];
>         int ret, count = ALIGN(size, sizeof(u32));
>
>         ret = regmap_raw_read(tc->regmap, DP0_AUXRDATA(0), auxrdata, count);
>         ...
>         memcpy(data, auxrdata, size);
>
> auxrdata is 16 bytes. AUX_BYTES can be as high as 255, which makes count
> 256. regmap_raw_read then writes past auxrdata on the stack, and the memcpy
> writes past msg->buffer. AUX_BYTES reflects the downstream DP sink
> response. tc_aux_transfer is the registered drm_dp_aux transfer handler. It
> is reached through DPCD and EDID reads, or from userspace through
> /dev/drm_dp_auxN. So a malicious or buggy sink is the source.
>
> ti-sn65dsi86 had the same pattern. It was fixed in aca58eac52b8
> ("drm/bridge: ti-sn65dsi86: Never store more than msg->size bytes in AUX
> xfer"), which clamps the hardware length back to the request. tc358767
> fetches the count from the device the same way but does not clamp.
>
> I do not have the hardware. A test of the read path with an oversized
> count trips the stack protector.
>
> Does this look like a real overflow to you? Is clamping the reported size
> back to the request the right fix, like ti-sn65dsi86 did? I am happy to
> send a patch with the reproducer once you confirm.

I'm not an expert on this driver and I didn't spend a massive amount
of time digging, but my quick thoughts:

If I understand correctly, we're reading "size" from the DP controller
registers after we do a transfer. We told it to transfer "size" bytes
of data. The controller should never try to transfer more than "size"
bytes of data, but it could transfer fewer. When we read how many
bytes it transferred, it should always be <= the "size" we told it to
transfer. So assuming we trust the DP controller, we should be safe.
Specifically, I don't believe that a malicious panel/DP display should
be able to craft something that causes the controller to report a
large "size".

That said, adding a bounds check here (like we did with ti-sn65dsi86)
seems wise. Even if the controller _shouldn't_ ever give us bad data,
the data we get from it should still be only "lightly trusted" IMO.
...and a bounds check here is cheap.

...so IMO, yeah, you should submit a patch to fix this. ...but it
won't be a "drop everything and get this fixed right away" security
hole, but just an extra layer of defense.

-Doug