public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH] drm/msm/adreno: fix userspace-triggered crash on a2xx-a4xx
@ 2026-04-06 22:14 Dmitry Baryshkov
  2026-04-06 22:24 ` Rob Clark
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Dmitry Baryshkov @ 2026-04-06 22:14 UTC (permalink / raw)
  To: Rob Clark, Sean Paul, Konrad Dybcio, Akhil P Oommen,
	Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang, Marijn Suijten,
	David Airlie, Simona Vetter
  Cc: linux-arm-msm, dri-devel, freedreno, linux-kernel

Before a5xx Adreno driver will not try fetching UBWC params (because
those generations didn't support UBWC anyway), however it's still
possible to query UBWC-related params from the userspace, triggering
possible NULL pointer dereference. Check for UBWC config in
adreno_get_param() and return sane defaults if there is none.

Fixes: a452510aad53 ("drm/msm/adreno: Switch to the common UBWC config struct")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index d5fe6f6f0dec..7dc95c0a17f7 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -426,16 +426,25 @@ int adreno_get_param(struct msm_gpu *gpu, struct msm_context *ctx,
 		*value = vm->mm_range;
 		return 0;
 	case MSM_PARAM_HIGHEST_BANK_BIT:
-		*value = adreno_gpu->ubwc_config->highest_bank_bit;
+		if (!adreno_gpu->ubwc_config)
+			*value = 13;
+		else
+			*value = adreno_gpu->ubwc_config->highest_bank_bit;
 		return 0;
 	case MSM_PARAM_RAYTRACING:
 		*value = adreno_gpu->has_ray_tracing;
 		return 0;
 	case MSM_PARAM_UBWC_SWIZZLE:
-		*value = adreno_gpu->ubwc_config->ubwc_swizzle;
+		if (!adreno_gpu->ubwc_config)
+			*value = 0;
+		else
+			*value = adreno_gpu->ubwc_config->ubwc_swizzle;
 		return 0;
 	case MSM_PARAM_MACROTILE_MODE:
-		*value = adreno_gpu->ubwc_config->macrotile_mode;
+		if (!adreno_gpu->ubwc_config)
+			*value = 0;
+		else
+			*value = adreno_gpu->ubwc_config->macrotile_mode;
 		return 0;
 	case MSM_PARAM_UCHE_TRAP_BASE:
 		*value = adreno_gpu->uche_trap_base;

---
base-commit: 36ece9697e89016181e5ae87510e40fb31d86f2b
change-id: 20260407-adreno-fix-ubwc-6a2564710e21

Best regards,
--  
With best wishes
Dmitry


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] drm/msm/adreno: fix userspace-triggered crash on a2xx-a4xx
  2026-04-06 22:14 [PATCH] drm/msm/adreno: fix userspace-triggered crash on a2xx-a4xx Dmitry Baryshkov
@ 2026-04-06 22:24 ` Rob Clark
  2026-04-06 23:43   ` Dmitry Baryshkov
  2026-04-12  4:17 ` Claude review: " Claude Code Review Bot
  2026-04-12  4:17 ` Claude Code Review Bot
  2 siblings, 1 reply; 5+ messages in thread
From: Rob Clark @ 2026-04-06 22:24 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Sean Paul, Konrad Dybcio, Akhil P Oommen, Dmitry Baryshkov,
	Abhinav Kumar, Jessica Zhang, Marijn Suijten, David Airlie,
	Simona Vetter, linux-arm-msm, dri-devel, freedreno, linux-kernel

On Mon, Apr 6, 2026 at 3:14 PM Dmitry Baryshkov
<dmitry.baryshkov@oss.qualcomm.com> wrote:
>
> Before a5xx Adreno driver will not try fetching UBWC params (because
> those generations didn't support UBWC anyway), however it's still
> possible to query UBWC-related params from the userspace, triggering
> possible NULL pointer dereference. Check for UBWC config in
> adreno_get_param() and return sane defaults if there is none.
>
> Fixes: a452510aad53 ("drm/msm/adreno: Switch to the common UBWC config struct")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> ---
>  drivers/gpu/drm/msm/adreno/adreno_gpu.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index d5fe6f6f0dec..7dc95c0a17f7 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -426,16 +426,25 @@ int adreno_get_param(struct msm_gpu *gpu, struct msm_context *ctx,
>                 *value = vm->mm_range;
>                 return 0;
>         case MSM_PARAM_HIGHEST_BANK_BIT:
> -               *value = adreno_gpu->ubwc_config->highest_bank_bit;
> +               if (!adreno_gpu->ubwc_config)
> +                       *value = 13;
> +               else
> +                       *value = adreno_gpu->ubwc_config->highest_bank_bit;

These three params are only used by userspace on a6xx, so tbh it would
be ok to return -ENOENT for platforms that have no ubwc config.  That
might be better than returning imaginary values.

BR,
-R

>                 return 0;
>         case MSM_PARAM_RAYTRACING:
>                 *value = adreno_gpu->has_ray_tracing;
>                 return 0;
>         case MSM_PARAM_UBWC_SWIZZLE:
> -               *value = adreno_gpu->ubwc_config->ubwc_swizzle;
> +               if (!adreno_gpu->ubwc_config)
> +                       *value = 0;
> +               else
> +                       *value = adreno_gpu->ubwc_config->ubwc_swizzle;
>                 return 0;
>         case MSM_PARAM_MACROTILE_MODE:
> -               *value = adreno_gpu->ubwc_config->macrotile_mode;
> +               if (!adreno_gpu->ubwc_config)
> +                       *value = 0;
> +               else
> +                       *value = adreno_gpu->ubwc_config->macrotile_mode;
>                 return 0;
>         case MSM_PARAM_UCHE_TRAP_BASE:
>                 *value = adreno_gpu->uche_trap_base;
>
> ---
> base-commit: 36ece9697e89016181e5ae87510e40fb31d86f2b
> change-id: 20260407-adreno-fix-ubwc-6a2564710e21
>
> Best regards,
> --
> With best wishes
> Dmitry
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] drm/msm/adreno: fix userspace-triggered crash on a2xx-a4xx
  2026-04-06 22:24 ` Rob Clark
@ 2026-04-06 23:43   ` Dmitry Baryshkov
  0 siblings, 0 replies; 5+ messages in thread
From: Dmitry Baryshkov @ 2026-04-06 23:43 UTC (permalink / raw)
  To: Rob Clark
  Cc: Sean Paul, Konrad Dybcio, Akhil P Oommen, Dmitry Baryshkov,
	Abhinav Kumar, Jessica Zhang, Marijn Suijten, David Airlie,
	Simona Vetter, linux-arm-msm, dri-devel, freedreno, linux-kernel

On Mon, Apr 06, 2026 at 03:24:43PM -0700, Rob Clark wrote:
> On Mon, Apr 6, 2026 at 3:14 PM Dmitry Baryshkov
> <dmitry.baryshkov@oss.qualcomm.com> wrote:
> >
> > Before a5xx Adreno driver will not try fetching UBWC params (because
> > those generations didn't support UBWC anyway), however it's still
> > possible to query UBWC-related params from the userspace, triggering
> > possible NULL pointer dereference. Check for UBWC config in
> > adreno_get_param() and return sane defaults if there is none.
> >
> > Fixes: a452510aad53 ("drm/msm/adreno: Switch to the common UBWC config struct")
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> > ---
> >  drivers/gpu/drm/msm/adreno/adreno_gpu.c | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > index d5fe6f6f0dec..7dc95c0a17f7 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > @@ -426,16 +426,25 @@ int adreno_get_param(struct msm_gpu *gpu, struct msm_context *ctx,
> >                 *value = vm->mm_range;
> >                 return 0;
> >         case MSM_PARAM_HIGHEST_BANK_BIT:
> > -               *value = adreno_gpu->ubwc_config->highest_bank_bit;
> > +               if (!adreno_gpu->ubwc_config)
> > +                       *value = 13;
> > +               else
> > +                       *value = adreno_gpu->ubwc_config->highest_bank_bit;
> 
> These three params are only used by userspace on a6xx, so tbh it would
> be ok to return -ENOENT for platforms that have no ubwc config.  That
> might be better than returning imaginary values.

I'd say, those are defaults. But I agree, -ENOENT might be better.

> 
> BR,
> -R
> 
> >                 return 0;
> >         case MSM_PARAM_RAYTRACING:
> >                 *value = adreno_gpu->has_ray_tracing;
> >                 return 0;
> >         case MSM_PARAM_UBWC_SWIZZLE:
> > -               *value = adreno_gpu->ubwc_config->ubwc_swizzle;
> > +               if (!adreno_gpu->ubwc_config)
> > +                       *value = 0;
> > +               else
> > +                       *value = adreno_gpu->ubwc_config->ubwc_swizzle;
> >                 return 0;
> >         case MSM_PARAM_MACROTILE_MODE:
> > -               *value = adreno_gpu->ubwc_config->macrotile_mode;
> > +               if (!adreno_gpu->ubwc_config)
> > +                       *value = 0;
> > +               else
> > +                       *value = adreno_gpu->ubwc_config->macrotile_mode;
> >                 return 0;
> >         case MSM_PARAM_UCHE_TRAP_BASE:
> >                 *value = adreno_gpu->uche_trap_base;
> >
> > ---
> > base-commit: 36ece9697e89016181e5ae87510e40fb31d86f2b
> > change-id: 20260407-adreno-fix-ubwc-6a2564710e21
> >
> > Best regards,
> > --
> > With best wishes
> > Dmitry
> >

-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Claude review: drm/msm/adreno: fix userspace-triggered crash on a2xx-a4xx
  2026-04-06 22:14 [PATCH] drm/msm/adreno: fix userspace-triggered crash on a2xx-a4xx Dmitry Baryshkov
  2026-04-06 22:24 ` Rob Clark
@ 2026-04-12  4:17 ` Claude Code Review Bot
  2026-04-12  4:17 ` Claude Code Review Bot
  2 siblings, 0 replies; 5+ messages in thread
From: Claude Code Review Bot @ 2026-04-12  4:17 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: drm/msm/adreno: fix userspace-triggered crash on a2xx-a4xx
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Patches: 3
Reviewed: 2026-04-12T14:17:19.494675

---

This is a single patch fixing a real NULL pointer dereference bug in the Adreno GPU driver. The problem is genuine: the commit `a452510aad53` ("drm/msm/adreno: Switch to the common UBWC config struct") introduced unconditional dereferences of `adreno_gpu->ubwc_config` in `adreno_get_param()`, but on a2xx through a4xx hardware, `ubwc_config` is never initialized (confirmed: none of a2xx_gpu.c, a3xx_gpu.c, or a4xx_gpu.c ever set `ubwc_config`). Any userspace process can trigger this crash by issuing an `MSM_PARAM_HIGHEST_BANK_BIT`, `MSM_PARAM_UBWC_SWIZZLE`, or `MSM_PARAM_MACROTILE_MODE` ioctl query on those older GPUs.

The fix is correct in that it prevents the crash, but the approach is debatable.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Claude review: drm/msm/adreno: fix userspace-triggered crash on a2xx-a4xx
  2026-04-06 22:14 [PATCH] drm/msm/adreno: fix userspace-triggered crash on a2xx-a4xx Dmitry Baryshkov
  2026-04-06 22:24 ` Rob Clark
  2026-04-12  4:17 ` Claude review: " Claude Code Review Bot
@ 2026-04-12  4:17 ` Claude Code Review Bot
  2 siblings, 0 replies; 5+ messages in thread
From: Claude Code Review Bot @ 2026-04-12  4:17 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**The bug is real and the fix prevents the crash.** The NULL dereference is userspace-triggerable via the `MSM_GET_PARAM` ioctl, making this a local DoS / kernel oops from unprivileged userspace. The `Fixes:` tag is appropriate.

**Design concern: returning fabricated defaults vs returning -EINVAL.**

The patch returns hardcoded values (`13` for `HIGHEST_BANK_BIT`, `0` for the others) when `ubwc_config` is NULL:

```c
	case MSM_PARAM_HIGHEST_BANK_BIT:
		if (!adreno_gpu->ubwc_config)
			*value = 13;
		else
			*value = adreno_gpu->ubwc_config->highest_bank_bit;
```

While `13` is a reasonable default for `highest_bank_bit` (it matches the value used in `a5xx_gpu.c:838` where `BUG_ON(adreno_gpu->ubwc_config->highest_bank_bit < 13)` shows 13 is the baseline), returning fabricated UBWC parameters for hardware that **doesn't support UBWC at all** is semantically questionable. These params are meaningless on a2xx-a4xx.

An alternative would be to return `-EINVAL` for these three params when `ubwc_config` is NULL, similar to how `MSM_PARAM_TIMESTAMP` returns `-EINVAL` when the GPU doesn't support timestamps. This would clearly communicate to userspace "this GPU does not have UBWC" rather than silently returning plausible-looking but meaningless values. If existing userspace (e.g., freedreno in Mesa) depends on getting a success return from these params without checking the GPU generation first, that would justify the current approach — but a comment explaining why defaults were chosen over `-EINVAL` would be helpful.

**Minor style observation: repetitive NULL checks.**

The three cases repeat the same `if (!adreno_gpu->ubwc_config)` pattern. A small refactor could consolidate:

```c
	case MSM_PARAM_HIGHEST_BANK_BIT:
	case MSM_PARAM_UBWC_SWIZZLE:
	case MSM_PARAM_MACROTILE_MODE:
		if (!adreno_gpu->ubwc_config) {
			*value = (param == MSM_PARAM_HIGHEST_BANK_BIT) ? 13 : 0;
			return 0;
		}
		/* fall through to individual handling */
```

But this is a matter of taste for a simple bugfix, and the current form is perfectly readable.

**Missing Cc: stable.** Given this is a userspace-triggerable NULL pointer dereference, this should probably carry `Cc: stable@vger.kernel.org` to get backported to affected stable kernels (those carrying `a452510aad53`).

**Overall verdict:** The patch fixes a real crash. The only substantive question is whether returning `-EINVAL` would be more appropriate than returning dummy values for hardware that has no UBWC support. If the maintainers are happy with the default-value approach (and Mesa handles it correctly), the patch is ready to merge with the possible addition of a `Cc: stable` tag.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2026-04-12  4:17 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-06 22:14 [PATCH] drm/msm/adreno: fix userspace-triggered crash on a2xx-a4xx Dmitry Baryshkov
2026-04-06 22:24 ` Rob Clark
2026-04-06 23:43   ` Dmitry Baryshkov
2026-04-12  4:17 ` Claude review: " Claude Code Review Bot
2026-04-12  4:17 ` Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox