Re: [PATCH] drm/gud: validate GUD_ROTATION_0 is present in supported rotations

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

Pre-existing issues:
- [Critical] An out-of-bounds heap write occurs in `gud_plane_atomic_check` due to an incorrect array indexing bug. When populating `req->properties` with plane properties, the driver uses `req->properties[num_properties + i]` while simultaneously incrementing `num_properties` inside the loop. This causes the effective index to be `initial_num_properties + 2 * i`, which skips elements and ultimately writes past the end of the allocated array. A malicious USB device can trigger this by advertising multiple plane and connector properties (up to 32 of each), causing the loop to write up to index 94 in an array allocated for 64 elements.
--

--- Patch [1]: [PATCH] drm/gud: validate GUD_ROTATION_0 is present in supported rotations ---
commit 63f4e25c2ff8da9667835a1e92cc53b147ae5f56
Author: Sajal Gupta <[email protected]>

drm/gud: validate GUD_ROTATION_0 is present in supported rotations

The rotation argument to drm_plane_create_rotation_property() is set to
DRM_MODE_ROTATE_0, and the device reported rotation bitmask is used as
the supported_rotations argument. The driver never validates that
GUD_ROTATION_0 is present, so a device that omits it from its
GUD_PROPERTY_ROTATION triggers a warning. Fix this by skipping the creation
of rotation property if the device doesn't have the GUD_ROTATION_0 bit.

Fixes: 40e1a70b4aed ("drm: Add GUD USB Display driver")
Reported-by: [email protected]
Closes: https://syzkaller.appspot.com/bug?extid=efe2810681f1b065d3a8
Tested-by: [email protected]
Signed-off-by: Sajal Gupta <[email protected]>

> diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c
> index 89bd6ca36003f..3a1b9e2a2eaa1 100644
> --- a/drivers/gpu/drm/gud/gud_drv.c
> +++ b/drivers/gpu/drm/gud/gud_drv.c
[ ... ]

[Severity: Critical]
This is a pre-existing issue, but can this loop write out of bounds?

Looking at gud_plane_atomic_check() in drivers/gpu/drm/gud/gud_pipe.c:

	num_properties = ret;
	for (i = 0; i < gdrm->num_properties; i++) {
		...
		req->properties[num_properties + i].prop = cpu_to_le16(prop);
		req->properties[num_properties + i].val = cpu_to_le64(val);
		num_properties++;
	}

Since num_properties is incremented inside the loop, the index
num_properties + i actually increments by 2 on each iteration.

If a USB device advertises multiple plane and connector properties,
would this skip array elements and eventually write past the end of the
req->properties allocation, causing a heap buffer overflow?

Could this be fixed by just using num_properties as the index instead of
num_properties + i?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1