Re: [PATCH v3] drm/appletbdrm: Add suspend and resume support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm/tiny/appletbdrm: memory leak of info buffer on appletbdrm_send_msg error
- [Medium] drm/appletbdrm: coordinate underflow on resume if device dimensions shrink
--

commit e4d6246ec3eb92963562077b451503c3ef907475
Author: Vasilij Markin <vasilij.kernel@tototo.si>
Subject: drm/appletbdrm: Add suspend and resume support

The driver has no USB power management callbacks, so the USB core unbinds
it before system suspend and binds it again on resume. This commit
implements suspend and resume with drm_mode_config_helper_suspend() and
drm_mode_config_helper_resume() so the device stays registered and the
last committed state is restored on resume.

> diff --git a/drivers/gpu/drm/tiny/appletbdrm.c b/drivers/gpu/drm/tiny/appletbdrm.c
> index 4297d9c1125e4..350dc598c9e7c 100644
> --- a/drivers/gpu/drm/tiny/appletbdrm.c
> +++ b/drivers/gpu/drm/tiny/appletbdrm.c
[ ... ]
> @@ -821,6 +822,48 @@ static void appletbdrm_shutdown(struct usb_interface *intf)
>  	drm_atomic_helper_shutdown(&adev->drm);
>  }
>  
[ ... ]
> +static int appletbdrm_reset_resume(struct usb_interface *intf)
> +{
> +	struct appletbdrm_device *adev = usb_get_intfdata(intf);
> +	struct drm_device *drm = &adev->drm;
> +	int ret, resume_ret;
> +
> +	/*
> +	 * A reset puts the device back into its initial state, where it
> +	 * expects the same handshake as during probe. Resume the DRM side
> +	 * even if the handshake fails, so that the state saved on suspend
> +	 * is released.
> +	 */
> +	ret = appletbdrm_get_information(adev);

[Severity: Medium]
This problem wasn't introduced by this patch, but calling
appletbdrm_get_information() here on every system resume can trigger a memory
leak if the USB device fails to handshake correctly:

drivers/gpu/drm/tiny/appletbdrm.c:appletbdrm_get_information() {
	...
	info = kzalloc_obj(*info);
	if (!info)
		return -ENOMEM;

	ret = appletbdrm_send_msg(adev, APPLETBDRM_MSG_GET_INFORMATION);
	if (ret)
		return ret; /* Leaks info */
	...
}

Can this cause repeated memory leaks if the device frequently fails to wake?

[Severity: Medium]
If a buggy or malicious USB device returns smaller display dimensions during
this handshake, appletbdrm_get_information() updates the device dimensions:

drivers/gpu/drm/tiny/appletbdrm.c:appletbdrm_get_information() {
	...
	adev->width = get_unaligned_le32(&info->width);
	adev->height = get_unaligned_le32(&info->height);
	...
}

When the DRM core subsequently commits the suspended atomic state, it uses the
old damage rectangles which are clamped to the pre-suspend mode bounds. This
can cause an integer underflow in appletbdrm_flush_damage():

drivers/gpu/drm/tiny/appletbdrm.c:appletbdrm_flush_damage() {
	...
	frame->begin_y = cpu_to_le16(adev->height - damage.x2);
	...
}

Because damage.x2 can now be larger than the newly updated adev->height, this
could underflow and send huge out-of-bounds coordinates to the USB device.
Should the new dimensions be validated against the existing mode bounds before
applying them?

> +	if (ret) {
> +		drm_err(drm, "Failed to get display information\n");
> +		goto resume;
> +	}
> +
> +	ret = appletbdrm_signal_readiness(adev);
> +	if (ret)
> +		drm_err(drm, "Failed to signal readiness\n");
> +
> +resume:
> +	resume_ret = drm_mode_config_helper_resume(drm);
> +
> +	return ret ? ret : resume_ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-appletbdrm-pm-v3-1-47377176619a@tototo.si?part=1