Re: [PATCH] drm: Optimized by adding the TV modes attribute

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unconditional recreation of DRM TV properties breaks atomic uAPI property updates and causes memory leaks of `drm_property` objects.

Pre-existing issues:
- [High] Out-of-bounds read in TV mode name processing via `strlen()` due to missing null-termination guarantees.
--

--- Patch [1]: [PATCH] drm: Optimized by adding the TV modes attribute ---
commit 5de13986f93b84f8b4c689b55a18d298dfc7431d
Author: Edward Adam Davis <eadavis@qq.com>

drm: Optimized by adding the TV modes attribute

When adding gud properties for drm connector within the function
gud_connector_add_properties(), if the TV modes property is not added
first, drm_mode_create_tv_properties_legacy() would fail to add the TV
modes property because the tv_select_subconnector_property has already
been added.

Closes: https://syzkaller.appspot.com/bug?extid=1944765c3659f63d3777

> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 8b4baed060f3a..7728c2941a701 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -2201,8 +2201,12 @@ int drm_mode_create_tv_properties_legacy(struct drm_device *dev,
>  	struct drm_property *tv_subconnector;
>  	unsigned int i;
>  
> -	if (dev->mode_config.tv_select_subconnector_property)
> +	if (dev->mode_config.tv_select_subconnector_property) {
> +		if (num_modes && !dev->mode_config.legacy_tv_mode_property)
> +			goto other;
> +
>  		return 0;
> +	}
>  
>  	/*
>  	 * Basic connector properties

[ ... ]

> @@ -2225,6 +2229,7 @@ int drm_mode_create_tv_properties_legacy(struct drm_device *dev,
>  		goto nomem;
>  	dev->mode_config.tv_subconnector_property = tv_subconnector;
>  
> +other:
>  	/*
>  	 * Other, TV specific properties: margins & TV modes.
>  	 */

[Severity: High]
Does this goto path unconditionally recreate properties like
tv_brightness_property?

If a GUD USB device exposes multiple TV properties, the driver calls
drm_mode_create_tv_properties_legacy() multiple times. For subsequent
properties, the goto other path is hit, but it proceeds to unconditionally
call drm_property_create_range() for brightness, contrast, etc.

Would this overwrite the pointers in dev->mode_config and leak the orphaned
drm_property objects?

Furthermore, during a userspace atomic commit to update the brightness
property on the first connector, wouldn't the pointer mismatch cause the
kernel to reject the valid property update with -EINVAL?

drivers/gpu/drm/drm_atomic_uapi.c:drm_atomic_connector_set_property() {
	...
	} else if (property == config->tv_brightness_property) {
		state->tv.brightness = val;
	...
}

[Severity: High]
This isn't a bug introduced by this patch, but does the TV mode name
processing in gud_connector_add_tv_mode() have an out-of-bounds read?

Untrusted data is read from the USB device into buf without enforcing
null-termination:

drivers/gpu/drm/gud/gud_connector.c:gud_connector_add_tv_mode() {
	...
	ret = gud_usb_get(gdrm, GUD_REQ_GET_CONNECTOR_TV_MODE_VALUES,
			  connector->index, buf, buf_len);
	...
	for (i = 0; i < num_modes; i++)
		modes[i] = &buf[i * GUD_CONNECTOR_TV_MODE_NAME_LEN];
	...
}

If the USB device payload contains full 16-byte chunks without any null
terminators, would the strlen() in drm_property_add_enum() read past the
end of the kmalloced buffer?

drivers/gpu/drm/drm_property.c:drm_property_add_enum() {
	...
	if (WARN_ON(strlen(name) >= DRM_PROP_NAME_LEN))
		return -EINVAL;
	...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_C6A625E0BAF4BB77334C9E0F1B244B7B1B08@qq.com?part=1