Re: [PATCH AUTOSEL 6.18] drm/amd/ras: reset CPER ring on corrupt entry size

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Using `strcmp` on a non-null-terminated `char[4]` array causes out-of-bounds reads and relies on adjacent zero bytes.
- [High] Out-of-bounds memory read when CPER headers wrap around the end of the ring buffer.
- [High] Incorrect read pointer calculation during CPER ring overflow recovery causes random pointer jumps and can skip valid records.
- [High] Truncation in ring pointer arithmetic causes overlapping memory writes and severe data corruption when encountering corrupted old records.
--

commit f138b5a4c4364a5846a5ccacfcb8f7aaf6be9542
Author: Xiang Liu <xiang.liu@amd.com>

drm/amd/ras: reset CPER ring on corrupt entry size

This commit detects when a corrupt entry size causes the read pointer update
to make no progress during cper ring overflow recovery. Instead of getting
stuck in an infinite loop while holding the ring mutex, it resets the ring
to an empty state.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cper.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cper.c
> index 425a3e5643608..2694facb06c73 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cper.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cper.c
> @@ -465,7 +465,7 @@ static u32 amdgpu_cper_ring_get_ent_sz(struct amdgpu_ring *ring, u64 pos)
>  
>  void amdgpu_cper_ring_write(struct amdgpu_ring *ring, void *src, int count)
>  {
> -	u64 pos, wptr_old, rptr;
> +	u64 pos, wptr_old, rptr, next_rptr;
>  	int rec_cnt_dw = count >> 2;
>  	u32 chunk, ent_sz;

[Severity: High]
This is a pre-existing issue, but could the truncation in the ring pointer
arithmetic result in overlapping memory writes when old, corrupted records
are encountered?

In amdgpu_cper_ring_write(), new data writes are chunked based on the
ent_sz (record size) of the old, to-be-overwritten data:

drivers/gpu/drm/amd/amdgpu/amdgpu_cper.c:amdgpu_cper_ring_write() {
    ...
    ent_sz = amdgpu_cper_ring_get_ent_sz(ring, ring->wptr);
    chunk = umin(ent_sz, count);

    memcpy(&ring->ring[ring->wptr], s, chunk);

    ring->wptr += (chunk >> 2);
    ring->wptr &= ring->ptr_mask;
    ...
}

If the old data is corrupted, ent_sz (and thus chunk) can be an arbitrary
value not aligned to 4 bytes. memcpy() copies chunk bytes, but
ring->wptr += (chunk >> 2) advances the pointer in dwords, truncating the
remainder.

For example, if chunk is 5, wptr advances by 1 dword. Will the next
iteration's memcpy start at the new wptr, overlapping and overwriting the
5th byte that was just written? If chunk < 4, does wptr advance by 0,
repeatedly overwriting the exact same dword?

[ ... ]

> @@ -506,9 +506,19 @@ void amdgpu_cper_ring_write(struct amdgpu_ring *ring, void *src, int count)
>  
>  		do {
>  			ent_sz = amdgpu_cper_ring_get_ent_sz(ring, pos);

[Severity: High]
This is a pre-existing issue, but are out-of-bounds reads possible here
when calling amdgpu_cper_ring_get_ent_sz() and amdgpu_cper_is_hdr()?

When encountering garbage cper data (e.g., during overflow recovery or
reading uninitialized memory), those functions call strcmp() on
chdr->signature:

drivers/gpu/drm/amd/amdgpu/amdgpu_cper.c:amdgpu_cper_is_hdr() {
    ...
    chdr = (struct cper_hdr *)&(ring->ring[pos]);
    return strcmp(chdr->signature, "CPER") ? false : true;
}

Since signature is a char[4] with no null terminator, strcmp() will
continue reading memory. If the struct happens to reside at the very end
of the buffer, could this read into unmapped memory and cause a page
fault?

[Severity: High]
This is a pre-existing issue, but can these direct struct pointer casts
cause out-of-bounds memory reads when cper headers wrap around the end of
the ring buffer?

The writer seamlessly splits records across the ring boundary using
memcpy():

drivers/gpu/drm/amd/amdgpu/amdgpu_cper.c:amdgpu_cper_ring_write() {
    ...
    memcpy(&ring->ring[ring->wptr], s, chunk);
    ...
}

However, amdgpu_cper_ring_get_ent_sz() casts the direct pointer and reads
fields that might be past the end of the buffer:

drivers/gpu/drm/amd/amdgpu/amdgpu_cper.c:amdgpu_cper_ring_get_ent_sz() {
    ...
    chdr = (struct cper_hdr *)&(ring->ring[pos]);
    ...
    if (!strcmp(chdr->signature, "CPER")) {
        rec_len = chdr->record_length;
        goto calc;
    }
}

If pos is very close to the end of the buffer (e.g., < 5 dwords
remaining), does dereferencing chdr->record_length at offset 20 read past
the end of the allocated ring->ring array?

> +			next_rptr = rptr;
> +			if (ent_sz >= sizeof(u32))
> +				next_rptr = (rptr + (ent_sz >> 2)) & ring->ptr_mask;

[Severity: High]
This is a pre-existing issue, but might this logic incorrectly add a
size/distance offset derived from pos to the unrelated and stale pointer
rptr?

When the cper ring overflows, the recovery loop sets pos to locate the
oldest un-overwritten data, and gets its size via ent_sz.

However, instead of advancing pos by this size, it adds ent_sz to rptr
(the original read pointer, which is completely unrelated to pos and
points somewhere in the middle of the overwritten region).

Does this mathematically meaningless addition corrupt the read pointer and
cause valid records to be skipped?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831133314.4125787-317-sashal@kernel.org?part=1