[PATCH v2] drm/drv: Add buffer bounds check in drm_dev_wedged_event()

[PATCH v2] drm/drv: Add buffer bounds check in drm_dev_wedged_event()

[Mallesh Koujalagi]

event_string[] has a fixed size of WEDGE_STR_LEN (32) bytes.
scnprintf(buf, size, "%s,", recovery) writes strlen(recovery)+1
content bytes (token + comma) and requires one additional byte
for the NUL terminator within 'size'.

Without a bounds check, if the remaining space is insufficient,
scnprintf() silently truncates the recovery token mid-name,
producing a malformed uevent payload (e.g. "WEDGED=bus-reset,vendor-spe").

Add a pre-flight drm_WARN_ON() guard:

  len + strlen(recovery) + 1 >= WEDGE_STR_LEN

Fixes: b7cf9f4ac1b8 ("drm: Introduce device wedged event")
Signed-off-by: Mallesh Koujalagi <mallesh.koujalagi@intel.com>
---
v2:
- Add proper logic to handle recovery string. (Raag)
---
 drivers/gpu/drm/drm_drv.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index e51ed959da89..e48b22ca38bd 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -586,6 +586,20 @@ int drm_dev_wedged_event(struct drm_device *dev, unsigned long method,
 		if (drm_WARN_ONCE(dev, !recovery, "invalid recovery method %u\n", opt))
 			break;
 
+		/*
+		 * scnprintf(buf, size, "%s,", recovery) writes strlen(recovery)+1
+		 * content bytes (token + comma) and reserves 1 byte for NUL within
+		 * 'size'.  It fits without truncation when:
+		 *   len + strlen(recovery) + 1 <= WEDGE_STR_LEN - 1  (safe)
+		 * so warn and bail when:
+		 *   len + strlen(recovery) + 1 >= WEDGE_STR_LEN       (would truncate)
+		 * Note: ">= WEDGE_STR_LEN - 1" would be off-by-one — it would
+		 * incorrectly reject the exact-fit case where NUL lands on the last
+		 * valid byte (index WEDGE_STR_LEN - 1).
+		 */
+		if (drm_WARN_ON(dev, len + strlen(recovery) + 1 >= WEDGE_STR_LEN))
+			break;
+
 		len += scnprintf(event_string + len, sizeof(event_string) - len, "%s,", recovery);
 	}
 
-- 
2.48.1

Re: [PATCH v2] drm/drv: Add buffer bounds check in drm_dev_wedged_event()

[sashiko-bot]

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

New issues:
- [High] Improper Use of WARN_ON for Valid Input Due to Insufficient Buffer Size

Pre-existing issues:
- [High] NULL Pointer Dereference on Compute Accelerator Devices
--

--- Patch [1]: [PATCH v2] drm/drv: Add buffer bounds check in drm_dev_wedged_event() ---
commit 97688eec142901f13da2ba3b0c76b902728202be
Author: Mallesh Koujalagi <mallesh.koujalagi@intel.com>
Subject: drm/drv: Add buffer bounds check in drm_dev_wedged_event()

This commit adds a bounds check in drm_dev_wedged_event() to prevent
scnprintf() from silently truncating the recovery token. It adds a
drm_WARN_ON() guard to catch cases where the buffer size WEDGE_STR_LEN
is exceeded.

> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 1ff0bf7cba6aa..09175d0e4a5a2 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -578,6 +578,20 @@ int drm_dev_wedged_event(struct drm_device *dev, unsigned long method,
>  		if (drm_WARN_ONCE(dev, !recovery, "invalid recovery method %u\n", opt))
>  			break;
>  
> +		/*
> +		 * scnprintf(buf, size, "%s,", recovery) writes strlen(recovery)+1
> +		 * content bytes (token + comma) and reserves 1 byte for NUL within
> +		 * 'size'.  It fits without truncation when:
> +		 *   len + strlen(recovery) + 1 <= WEDGE_STR_LEN - 1  (safe)
> +		 * so warn and bail when:
> +		 *   len + strlen(recovery) + 1 >= WEDGE_STR_LEN       (would truncate)
> +		 * Note: ">= WEDGE_STR_LEN - 1" would be off-by-one — it would
> +		 * incorrectly reject the exact-fit case where NUL lands on the last
> +		 * valid byte (index WEDGE_STR_LEN - 1).
> +		 */
> +		if (drm_WARN_ON(dev, len + strlen(recovery) + 1 >= WEDGE_STR_LEN))
                ^^^^

[Severity: High]
Does this WARN_ON() trigger on perfectly valid API usage?

In drivers/gpu/drm/drm_drv.c:drm_dev_wedged_event(), the exported API supports
multiple recovery methods in a bitmask. For example, if a driver passes 
DRM_WEDGE_RECOVERY_BUS_RESET | DRM_WEDGE_RECOVERY_VENDOR, the combined string 
length will reach 33 bytes (7 for WEDGED= + 9 for bus-reset + 1 for comma +
15 for vendor-specific + 1 for comma). 

Since WEDGE_STR_LEN is hardcoded to 32 bytes, the condition 33 >= 32 will 
evaluate to true and trigger the newly added drm_WARN_ON(). On systems with 
panic_on_warn enabled, this will cause a kernel panic, effectively penalizing
the driver for using a valid API bitmask because of an artificially small
internal buffer size.

> +			break;
> +
>  		len += scnprintf(event_string + len, sizeof(event_string) - len, "%s,", recovery);
>  	}

[Severity: High]
This is a pre-existing issue and was not introduced by the patch under review, 
but does the end of drm_dev_wedged_event() unconditionally dereference 
dev->primary?

drivers/gpu/drm/drm_drv.c:drm_dev_wedged_event() {
...
	if (info && (info->comm[0] != '\0') && (info->pid >= 0)) {
		snprintf(pid_string, sizeof(pid_string), "PID=%u", info->pid);
		snprintf(comm_string, sizeof(comm_string), "TASK=%s", info->comm);
		envp[1] = pid_string;
		envp[2] = comm_string;
	}

	return kobject_uevent_env(&dev->primary->kdev->kobj, KOBJ_CHANGE, envp);
                                   ^^^^
}

Compute accelerator drivers (configured with DRIVER_COMPUTE_ACCEL) only
allocate an accel minor (DRM_MINOR_ACCEL) during initialization and
intentionally leave dev->primary as NULL.

If a compute accelerator driver calls drm_dev_wedged_event() to report a 
hardware wedge state, this will result in a NULL pointer dereference and a
kernel panic.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724063005.703631-2-mallesh.koujalagi@intel.com?part=1