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 <[email protected]> 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/[email protected]?part=1
