Re: [PATCH v13 1/4] drm: Add DRM_WEDGE_RECOVERY_COLD_RESET recovery method

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

Pre-existing issues:
- [High] The `event_string` buffer used for wedged uevents is limited to 32 bytes (`WEDGE_STR_LEN`), which is insufficient to hold multiple combined recovery methods. This leads to silent string truncation and a malformed UAPI payload.
--

--- Patch [1]: [PATCH v13 1/4] drm: Add DRM_WEDGE_RECOVERY_COLD_RESET recovery method ---
commit 091feacf4c7aa58c5985f7e23f67de90d3d5760f
Author: Mallesh Koujalagi <[email protected]>

drm: Add DRM_WEDGE_RECOVERY_COLD_RESET recovery method

Introduce DRM_WEDGE_RECOVERY_COLD_RESET (BIT(4)) recovery method to handle
scenarios requiring device power cycle. This method addresses cases where
other recovery mechanisms are insufficient to restore device functionality.

> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index e51ed959da89f..8519e97ef5d31 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -545,6 +545,8 @@ static const char *drm_get_wedge_recovery(unsigned int opt)
>  		return "bus-reset";
>  	case DRM_WEDGE_RECOVERY_VENDOR:
>  		return "vendor-specific";
> +	case DRM_WEDGE_RECOVERY_COLD_RESET:
> +		return "cold-reset";

[Severity: High]
This is a pre-existing issue, but will adding the "cold-reset" recovery
method cause the event_string buffer to overflow and silently truncate?

Looking at drm_dev_wedged_event() in drivers/gpu/drm/drm_drv.c, the
event_string buffer is limited to 32 bytes (WEDGE_STR_LEN). If multiple
recovery methods are combined, their appended strings can exceed this limit:

drivers/gpu/drm/drm_drv.c:drm_dev_wedged_event() {
	char event_string[WEDGE_STR_LEN]; ...
	...
	len = scnprintf(event_string, sizeof(event_string), "%s", "WEDGED=");

	for_each_set_bit(opt, &method, BITS_PER_TYPE(method)) {
		recovery = drm_get_wedge_recovery(opt);
		...
		len += scnprintf(event_string + len,
				 sizeof(event_string) - len, "%s,", recovery);
	}

	if (recovery)
		/* Get rid of trailing comma */
		event_string[len - 1] = '\0';
...
}

When the string is truncated by scnprintf() to fit the 32-byte buffer, could
the trailing comma removal blindly overwrite the last character of the
truncated word (e.g. chopping "cold-reset" down to "cold-r")? It appears this
might result in a malformed UAPI payload being sent to userspace when
multiple recovery methods are combined.

>  	default:
>  		return NULL;
>  	}

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