[PATCH v2 0/2] drm/amdgpu: reject misaligned IB addresses in CS parser

[PATCH v2 0/2] drm/amdgpu: reject misaligned IB addresses in CS parser

[John B. Moore]

Userspace can submit command streams with IB addresses whose low two
bits are set.  On all hardware that amdgpu supports, those bits are
reserved (they encoded byte-swap mode on pre-amdgpu legacy HW).
Today these addresses pass through the CS parser unchecked and hit
BUG_ON(addr & 0x3) assertions in ring emission callbacks across
gfx_v9 through gfx_v12 and sdma_v4 through sdma_v7 (35 call sites),
crashing the kernel.

Patch 1 adds an early -EINVAL rejection in the CS parser before the
IB is allocated, plus a defense-in-depth WARN_ON_ONCE in
amdgpu_ib_schedule() to catch any that slip through from other code
paths.

Patch 2 is a trivial cleanup: removing a dead BUG_ON(!bo_va) in
amdgpu_cs_vm_handling() that is unreachable due to the NULL check on
the line above.

A follow-up series could convert the 35 downstream BUG_ON(addr & 0x3)
assertions in the ring emit_ib callbacks to WARN_ON_ONCE, but that is
a larger change and is not included here.

v2:
 - Rebased onto amd-staging-drm-next (was incorrectly based on a
   local branch in v1 — thanks Christian for catching this)
 - Split the dead-code BUG_ON removal into a separate patch
 - Moved the check before amdgpu_ib_get() to avoid unnecessary
   IB allocation on bad input
 - Added Fixes: tag and Cc: stable

John B. Moore (2):
  drm/amdgpu: reject IB addresses with reserved byte-swap bits
  drm/amdgpu: remove superfluous BUG_ON in amdgpu_cs_vm_handling

 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c |  9 ++++++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c | 10 ++++++++++
 2 files changed, 18 insertions(+), 1 deletion(-)

-- 
2.43.0

[PATCH v2 1/2] drm/amdgpu: reject IB addresses with reserved byte-swap bits

[John B. Moore]

Reject IB GPU addresses with bits [1:0] set early in the CS parser,
before they reach ring emission callbacks. On legacy AMD hardware
(pre-amdgpu era), these two bits encoded byte-swap mode for IB memory
fetches. That feature was dropped on all hardware that amdgpu supports,
but the ring emission paths still contain BUG_ON(addr & 0x3) assertions
that crash the kernel if userspace submits a misaligned IB address.

Add an early check in amdgpu_cs_p2_ib() to reject such submissions
with -EINVAL before the IB is allocated, and a defense-in-depth
WARN_ON_ONCE in amdgpu_ib_schedule() to catch any that slip through
from other code paths.

Fixes: b0635e808290 ("drm/amdgpu: implement GFX 9.0 support (v2)")
Cc: stable@vger.kernel.org
Signed-off-by: John B. Moore <jbmoore61@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c |  8 ++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c | 10 ++++++++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 10d8dcc3a..53f537f3e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -379,6 +379,14 @@ static int amdgpu_cs_p2_ib(struct amdgpu_cs_parser *p,
 	if (chunk_ib->flags & AMDGPU_IB_FLAG_PREAMBLE)
 		job->preamble_status |= AMDGPU_PREAMBLE_IB_PRESENT;
 
+	/* Reject IB addresses with reserved byte-swap bits set.
+	 * On legacy HW (pre-amdgpu), bits [1:0] encoded byte-swap mode
+	 * for IB fetches. That feature is deprecated on all HW that
+	 * amdgpu supports, so these bits must be zero.
+	 */
+	if (chunk_ib->va_start & 0x3)
+		return -EINVAL;
+
 	r =  amdgpu_ib_get(p->adev, vm, ring->funcs->parse_cs ?
 			   chunk_ib->ib_bytes : 0,
 			   AMDGPU_IB_POOL_DELAYED, ib);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
index f1ed4a436..3111d2c7e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
@@ -272,6 +272,16 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned int num_ibs,
 	for (i = 0; i < num_ibs; ++i) {
 		ib = &ibs[i];
 
+		/* Defense-in-depth: the CS parser rejects misaligned IB
+		 * addresses, but catch any that slip through before they
+		 * hit BUG_ON(addr & 0x3) in ring emission callbacks.
+		 */
+		if (WARN_ON_ONCE(ib->gpu_addr & 0x3)) {
+			r = -EINVAL;
+			amdgpu_ring_undo(ring);
+			goto free_fence;
+		}
+
 		if (job && ring->funcs->emit_frame_cntl) {
 			if (secure != !!(ib->flags & AMDGPU_IB_FLAGS_SECURE)) {
 				amdgpu_ring_emit_frame_cntl(ring, false, secure);
-- 
2.43.0

[PATCH v2 2/2] drm/amdgpu: remove superfluous BUG_ON in amdgpu_cs_vm_handling

[John B. Moore]

Remove the BUG_ON(!bo_va) in amdgpu_cs_vm_handling() that is
unreachable: bo_va is assigned from fpriv->csa_va on the line
directly above, inside an if (fpriv->csa_va) block, so it is
guaranteed to be non-NULL at that point.

Signed-off-by: John B. Moore <jbmoore61@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 53f537f3e..556c62948 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -1137,7 +1137,6 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
 
 	if (fpriv->csa_va) {
 		bo_va = fpriv->csa_va;
-		BUG_ON(!bo_va);
 		r = amdgpu_vm_bo_update(adev, bo_va, false);
 		if (r)
 			return r;
-- 
2.43.0

Re: [PATCH v2 1/2] drm/amdgpu: reject IB addresses with reserved byte-swap bits

[Christian König]

On 4/24/26 16:08, John B. Moore wrote:
> Reject IB GPU addresses with bits [1:0] set early in the CS parser,
> before they reach ring emission callbacks. On legacy AMD hardware
> (pre-amdgpu era), these two bits encoded byte-swap mode for IB memory
> fetches. That feature was dropped on all hardware that amdgpu supports,
> but the ring emission paths still contain BUG_ON(addr & 0x3) assertions
> that crash the kernel if userspace submits a misaligned IB address.
> 
> Add an early check in amdgpu_cs_p2_ib() to reject such submissions
> with -EINVAL before the IB is allocated, and a defense-in-depth
> WARN_ON_ONCE in amdgpu_ib_schedule() to catch any that slip through
> from other code paths.
> 
> Fixes: b0635e808290 ("drm/amdgpu: implement GFX 9.0 support (v2)")
> Cc: stable@vger.kernel.org
> Signed-off-by: John B. Moore <jbmoore61@gmail.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c |  8 ++++++++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c | 10 ++++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 10d8dcc3a..53f537f3e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -379,6 +379,14 @@ static int amdgpu_cs_p2_ib(struct amdgpu_cs_parser *p,
>  	if (chunk_ib->flags & AMDGPU_IB_FLAG_PREAMBLE)
>  		job->preamble_status |= AMDGPU_PREAMBLE_IB_PRESENT;
>  
> +	/* Reject IB addresses with reserved byte-swap bits set.
> +	 * On legacy HW (pre-amdgpu), bits [1:0] encoded byte-swap mode
> +	 * for IB fetches. That feature is deprecated on all HW that
> +	 * amdgpu supports, so these bits must be zero.
> +	 */
> +	if (chunk_ib->va_start & 0x3)
> +		return -EINVAL;
> +
>  	r =  amdgpu_ib_get(p->adev, vm, ring->funcs->parse_cs ?
>  			   chunk_ib->ib_bytes : 0,
>  			   AMDGPU_IB_POOL_DELAYED, ib);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> index f1ed4a436..3111d2c7e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> @@ -272,6 +272,16 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned int num_ibs,
>  	for (i = 0; i < num_ibs; ++i) {
>  		ib = &ibs[i];
>  
> +		/* Defense-in-depth: the CS parser rejects misaligned IB
> +		 * addresses, but catch any that slip through before they
> +		 * hit BUG_ON(addr & 0x3) in ring emission callbacks.
> +		 */
> +		if (WARN_ON_ONCE(ib->gpu_addr & 0x3)) {
> +			r = -EINVAL;
> +			amdgpu_ring_undo(ring);
> +			goto free_fence;
> +		}
> +

Please drop that chunk. Apart from that the patch looks good to me.

Regards,
Christian.

>  		if (job && ring->funcs->emit_frame_cntl) {
>  			if (secure != !!(ib->flags & AMDGPU_IB_FLAGS_SECURE)) {
>  				amdgpu_ring_emit_frame_cntl(ring, false, secure);