Re: [PATCH AUTOSEL 6.18] drm/amd/ras: Fix CPER ring debugfs read overflow

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] start_dw calculation truncates loff_t offset to u32.

Pre-existing issues:
- [High] Return of large positive value on put_user() failure due to u32 cast.
- [High] Out-of-bounds read and logic failure in CPER header parsing due to strcmp on non-null-terminated string.
--

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

drm/amd/ras: Fix CPER ring debugfs read overflow

The legacy CPER debugfs reader can reach the payload path without a valid
pointer snapshot, and treats user byte count as ring occupancy in dwords.
This commit fixes the overflow by taking the CPER lock before sampling
pointers, properly bounding the payload copy, and advancing the file position.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> index 5ec5c3ff22bb0..f3a891124250a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c

[ ... ]

> @@ -546,13 +547,24 @@ static ssize_t amdgpu_debugfs_ring_read(struct file *f, char __user *buf,
>  			*pos += 4;
>  		}
>  	} else {
> +		early[0] = amdgpu_ring_get_rptr(ring) & ring->buf_mask;
> +		early[1] = amdgpu_ring_get_wptr(ring) & ring->buf_mask;
> +
>  		p = early[0];
>  		if (early[0] <= early[1])
> -			size = (early[1] - early[0]);
> +			avail_dw = early[1] - early[0];
>  		else
> -			size = ring->ring_size - (early[0] - early[1]);
> +			avail_dw = ring->buf_mask + 1 - (early[0] - early[1]);
>  
> -		while (size) {
> +		start_dw = (*pos > 12) ? ((*pos - 12) >> 2) : 0;
> +		if (start_dw >= avail_dw)
> +			goto out;

[Severity: Medium]
Does this code truncate the 64-bit file position into a 32-bit variable?

When start_dw is calculated, pos is a pointer to loff_t, but the offset is
assigned to a u32 variable. If a user calls pread() on the debugfs file
with an offset exceeding ~17GB (0x400000000 bytes), start_dw could
overflow. This would lead to incorrect EOF evaluation, wrapping around and
returning cyclic data instead of returning 0 for EOF.

> +
> +		p = (p + start_dw) & ring->ptr_mask;
> +		avail_dw -= start_dw;
> +		read_dw = min_t(u32, avail_dw, size >> 2);
> +
> +		while (read_dw) {
>  			if (p == early[1])
>  				goto out;
>  
> @@ -565,9 +577,10 @@ static ssize_t amdgpu_debugfs_ring_read(struct file *f, char __user *buf,
>  
>  			buf += 4;
>  			result += 4;
> -			size--;
> +			read_dw--;
>  			p++;
>  			p &= ring->ptr_mask;
> +			*pos += 4;
>  		}

[Severity: High]
This is a pre-existing issue, but can a put_user() failure result in a large
positive return value being sent to userspace here?

Looking at the unquoted code just above this in amdgpu_debugfs_ring_read():

    r = put_user(value, (uint32_t *)buf);
    if (r) {
        result = r;
        goto out;
    }

Because result is declared as a u32 variable, assigning a negative error
code (like -EFAULT) casts it to a huge positive integer. When this is
returned as ssize_t to userspace, it hides the actual error and breaks
the read() syscall contract by falsely reporting ~4GB of data read.

[Severity: High]
This is another pre-existing issue, but does the CPER header parsing have an
out-of-bounds read and logic failure?

Looking at amdgpu_cper_ring_get_ent_sz() and amdgpu_cper_is_hdr() in
drivers/gpu/drm/amd/amdgpu/amdgpu_cper.c, they both use strcmp() on a 4-byte
non-null-terminated character array:

amdgpu_cper.c:amdgpu_cper_ring_get_ent_sz() {
    ...
    if (!strcmp(chdr->signature, "CPER")) {
        rec_len = chdr->record_length;
    ...
}

amdgpu_cper.c:amdgpu_cper_is_hdr() {
    ...
    return strcmp(chdr->signature, "CPER") ? false : true;
}

Because the signature field is only 4 bytes and the 5th byte in the struct
is the non-zero revision field, strcmp() will always return non-zero,
breaking CPER header detection entirely. This also causes an out-of-bounds
read that could cross a page boundary at the end of the ring buffer
allocation.

Additionally, if pos is near the end of the buffer mask, accessing
chdr->record_length also reads 20 bytes past the end of the ring buffer
allocation.

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