* Re: [PATCH v2] drm/drv: Add buffer bounds check in drm_dev_wedged_event()
2026-07-24 6:30 [PATCH v2] drm/drv: Add buffer bounds check in drm_dev_wedged_event() Mallesh Koujalagi
@ 2026-07-24 6:45 ` sashiko-bot
2026-07-28 6:14 ` Raag Jadav
2026-07-28 10:59 ` Jani Nikula
2 siblings, 0 replies; 5+ messages in thread
From: sashiko-bot @ 2026-07-24 6:45 UTC (permalink / raw)
To: Mallesh Koujalagi; +Cc: dri-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] drm/drv: Add buffer bounds check in drm_dev_wedged_event()
2026-07-24 6:30 [PATCH v2] drm/drv: Add buffer bounds check in drm_dev_wedged_event() Mallesh Koujalagi
2026-07-24 6:45 ` sashiko-bot
@ 2026-07-28 6:14 ` Raag Jadav
2026-07-28 10:59 ` Jani Nikula
2 siblings, 0 replies; 5+ messages in thread
From: Raag Jadav @ 2026-07-28 6:14 UTC (permalink / raw)
To: Mallesh Koujalagi
Cc: dri-devel, rodrigo.vivi, andrealmeid, christian.koenig, airlied,
simona.vetter, mripard, maarten.lankhorst, tzimmermann,
anshuman.gupta, badal.nilawar, riana.tauro, karthik.poosa,
sk.anirban
On Fri, Jul 24, 2026 at 12:00:06PM +0530, Mallesh Koujalagi wrote:
> 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).
This looks a bit verbose, can we shorten it up a bit?
> + */
> + if (drm_WARN_ON(dev, len + strlen(recovery) + 1 >= WEDGE_STR_LEN))
I'd use drm_WARN_ON_ONCE() and avoid spamming logs.
Raag
> + break;
> +
> len += scnprintf(event_string + len, sizeof(event_string) - len, "%s,", recovery);
> }
>
> --
> 2.48.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] drm/drv: Add buffer bounds check in drm_dev_wedged_event()
2026-07-24 6:30 [PATCH v2] drm/drv: Add buffer bounds check in drm_dev_wedged_event() Mallesh Koujalagi
2026-07-24 6:45 ` sashiko-bot
2026-07-28 6:14 ` Raag Jadav
@ 2026-07-28 10:59 ` Jani Nikula
2026-07-28 11:22 ` Raag Jadav
2 siblings, 1 reply; 5+ messages in thread
From: Jani Nikula @ 2026-07-28 10:59 UTC (permalink / raw)
To: Mallesh Koujalagi, dri-devel, rodrigo.vivi
Cc: andrealmeid, christian.koenig, airlied, simona.vetter, mripard,
maarten.lankhorst, tzimmermann, anshuman.gupta, badal.nilawar,
riana.tauro, karthik.poosa, sk.anirban, raag.jadav,
Mallesh Koujalagi
On Fri, 24 Jul 2026, Mallesh Koujalagi <mallesh.koujalagi@intel.com> wrote:
> 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
The kernel has seq_buf precisely to avoid manually rolling tedious and
error prone bounds checks everywhere. Please consider converting the
whole thing to it instead.
BR,
Jani.
>
> 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);
> }
--
Jani Nikula, Intel
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] drm/drv: Add buffer bounds check in drm_dev_wedged_event()
2026-07-28 10:59 ` Jani Nikula
@ 2026-07-28 11:22 ` Raag Jadav
0 siblings, 0 replies; 5+ messages in thread
From: Raag Jadav @ 2026-07-28 11:22 UTC (permalink / raw)
To: Jani Nikula
Cc: Mallesh Koujalagi, dri-devel, rodrigo.vivi, andrealmeid,
christian.koenig, airlied, simona.vetter, mripard,
maarten.lankhorst, tzimmermann, anshuman.gupta, badal.nilawar,
riana.tauro, karthik.poosa, sk.anirban
On Tue, Jul 28, 2026 at 01:59:44PM +0300, Jani Nikula wrote:
> On Fri, 24 Jul 2026, Mallesh Koujalagi <mallesh.koujalagi@intel.com> wrote:
> > 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
>
> The kernel has seq_buf precisely to avoid manually rolling tedious and
> error prone bounds checks everywhere. Please consider converting the
> whole thing to it instead.
+1, much better.
Raag
> > 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);
> > }
>
> --
> Jani Nikula, Intel
^ permalink raw reply [flat|nested] 5+ messages in thread