Re: [PATCH 1/4] drm/amdgpu/sdma4: replace BUG_ON with WARN_ON_ONCE in fence emission

["Christian König" <christian.koenig@amd.com>]

On 4/26/26 23:52, jbmoore wrote:
> From: "John B. Moore" <jbmoore61@gmail.com>
> 
> sdma_v4_0_ring_emit_fence() contains two BUG_ON(addr & 0x3) assertions
> that verify fence writeback addresses are dword-aligned.  These
> assertions can be reached via crafted DRM_IOCTL_AMDGPU_CS submissions
> from unprivileged userspace, causing a fatal kernel panic in a
> scheduler worker thread.
> 
> Replace both BUG_ON() calls with WARN_ON_ONCE() and force-align the
> address by clearing the reserved bits.  This logs the condition once
> per boot and allows the hardware to proceed without crashing the
> kernel.
> 
> On all hardware that amdgpu supports, bits [1:0] of ring buffer
> addresses are reserved (they historically encoded byte-swap mode on
> legacy pre-amdgpu hardware).  A misaligned fence address indicates a
> driver bug, but crashing the kernel is never the correct response.
> 
> Found by a custom amdgpu DRM ioctl fuzzer.
> 
> Fixes: 2130f89ced2c ("drm/amdgpu: add SDMA v4.0 implementation (v2)")
> Signed-off-by: John B. Moore <jbmoore61@gmail.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> index 8a2a4e618..dcb7e4219 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> @@ -889,7 +889,8 @@ static void sdma_v4_0_ring_emit_fence(struct amdgpu_ring *ring, u64 addr, u64 se
>  	/* write the fence */
>  	amdgpu_ring_write(ring, SDMA_PKT_HEADER_OP(SDMA_OP_FENCE));
>  	/* zero in first two bits */
> -	BUG_ON(addr & 0x3);
> +	if (WARN_ON_ONCE(addr & 0x3))
> +		addr &= ~0x3ULL;
>  	amdgpu_ring_write(ring, lower_32_bits(addr));
>  	amdgpu_ring_write(ring, upper_32_bits(addr));
>  	amdgpu_ring_write(ring, lower_32_bits(seq));
> @@ -899,7 +900,8 @@ static void sdma_v4_0_ring_emit_fence(struct amdgpu_ring *ring, u64 addr, u64 se
>  		addr += 4;
>  		amdgpu_ring_write(ring, SDMA_PKT_HEADER_OP(SDMA_OP_FENCE));
>  		/* zero in first two bits */
> -		BUG_ON(addr & 0x3);
> +		if (WARN_ON_ONCE(addr & 0x3))
> +			addr &= ~0x3ULL;

A WARN_ON() should be sufficient here and I don't think we should mask the lower bits.

It is perfectly possible that the lower bits were re-used for some other feature than byte swap.

We should just make sure that the CS IOCTL filters out all invalid submissions since here it is clearly to late to do anything about it.

Regards,
Christian.


>  		amdgpu_ring_write(ring, lower_32_bits(addr));
>  		amdgpu_ring_write(ring, upper_32_bits(addr));
>  		amdgpu_ring_write(ring, upper_32_bits(seq));