Re: [PATCH] drm/gud: NUL-terminate TV mode names read from the device

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

Pre-existing issues:
- [High] A malicious or misconfigured USB device can cause a NULL pointer dereference and kernel panic in `drm_object_attach_property` by returning `GUD_PROPERTY_TV_MODE` after any other non-margin TV property (such as `GUD_PROPERTY_TV_BRIGHTNESS` or `GUD_PROPERTY_TV_HUE`).
--

--- Patch [1]: [PATCH] drm/gud: NUL-terminate TV mode names read from the device ---
commit 789868bcbf6cc6ddf0d3b74feab3a22914863ec8
Author: Deepanshu Kartikey <[email protected]>

drm/gud: NUL-terminate TV mode names read from the device

This commit fixes a potential out-of-bounds read by explicitly NUL-terminating
TV mode names read from the USB device. This ensures strlen() won't run past
the end of the allocation when passed to drm_mode_create_tv_properties_legacy().

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

> diff --git a/drivers/gpu/drm/gud/gud_connector.c b/drivers/gpu/drm/gud/gud_connector.c
> index ea0cca58b7c89..5c0065c876a77 100644
> --- a/drivers/gpu/drm/gud/gud_connector.c
> +++ b/drivers/gpu/drm/gud/gud_connector.c
> @@ -396,8 +396,13 @@ static int gud_connector_add_tv_mode(struct gud_device *gdrm, struct drm_connect
>  	}
>  
>  	num_modes = ret / GUD_CONNECTOR_TV_MODE_NAME_LEN;
> -	for (i = 0; i < num_modes; i++)
> -		modes[i] = &buf[i * GUD_CONNECTOR_TV_MODE_NAME_LEN];
> +	for (i = 0; i < num_modes; i++) {
> +		char *mode = &buf[i * GUD_CONNECTOR_TV_MODE_NAME_LEN];
> +
> +		/* The device is not trusted to NUL-terminate the name */
> +		mode[GUD_CONNECTOR_TV_MODE_NAME_LEN - 1] = '\0';
> +		modes[i] = mode;
> +	}
>  
>  	ret = drm_mode_create_tv_properties_legacy(connector->dev, num_modes, modes);

[Severity: High]
This is a pre-existing issue, but could a malicious or misconfigured USB device
cause a NULL pointer dereference and kernel panic by returning
GUD_PROPERTY_TV_MODE after another non-margin TV property?

In gud_connector_add_properties(), properties from the USB device are processed
sequentially. If a property like GUD_PROPERTY_TV_BRIGHTNESS is processed first,
drm_mode_create_tv_properties_legacy() creates the baseline TV properties but
leaves legacy_tv_mode_property as NULL since num_modes is 0.

If GUD_PROPERTY_TV_MODE is processed next, it calls gud_connector_add_tv_mode()
which then calls drm_mode_create_tv_properties_legacy() here:

drivers/gpu/drm/drm_connector.c:drm_mode_create_tv_properties_legacy() {
    ...
	if (dev->mode_config.tv_select_subconnector_property)
		return 0;
    ...
}

This early return prevents dev->mode_config.legacy_tv_mode_property from
being created.

Then in gud_connector_add_properties(), gud_connector_property_lookup()
will return NULL for legacy_tv_mode_property:

drivers/gpu/drm/gud/gud_connector.c:gud_connector_add_properties() {
    ...
		property = gud_connector_property_lookup(connector, prop);
		if (drm_WARN_ON(drm, IS_ERR(property)))
			continue;
    ...
}

Since IS_ERR() ignores NULL, the NULL property pointer bypasses this check and
is passed to drm_object_attach_property(), which unconditionally dereferences
it:

drivers/gpu/drm/drm_mode_object.c:drm_object_attach_property() {
    ...
	struct drm_device *dev = property->dev;
    ...
}

Could this lead to a local denial of service?

>  free:
>  	kfree(buf);
>  	if (ret < 0)

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