Re: [PATCH 5.10] drm/amdgpu: Fix integer overflow in amdgpu_cs_pass1

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Returning success (0) for an empty chunk list leaves parser fields uninitialized, causing a NULL pointer dereference.
- [Critical] Missing bounds validation for AMDGPU_CHUNK_ID_IB chunks leads to out-of-bounds reads and NULL pointer dereference.
- [Critical] Double free and use-after-free of user fence BO when amdgpu_cs_user_fence_chunk() fails.
- [Critical] NULL pointer dereference in amdgpu_cs_parser_fini if chunk array allocation fails.
- [High] Integer overflow in amdgpu_bo_create_list_entry_array when calculating bytes for copy_from_user and memset leaves kernel heap memory uninitialized.
- [High] Memory leak of user fence BO when multiple FENCE chunks are provided.
--

commit 9efe72932540079a0dc0c2bc02c23c524aa5971d
Author: hackyzh002 <hackyzh002@gmail.com>

drm/amdgpu: Fix integer overflow in amdgpu_cs_pass1

This commit fixes an integer overflow where size *= sizeof(uint32_t)
could overflow the unsigned int type if size is 0x40000000. It updates
size to a size_t to prevent uninitialized memory from being referenced.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 12598a4b5c788..6c18054e045dc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -110,7 +110,8 @@ static int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, union drm_amdgpu_cs
>  	struct amdgpu_vm *vm = &fpriv->vm;
>  	uint64_t *chunk_array_user;
>  	uint64_t *chunk_array;
> -	unsigned size, num_ibs = 0;
> +	size_t size;
> +	unsigned int num_ibs = 0;
>  	uint32_t uf_offset = 0;
>  	int i;
>  	int ret;

[Severity: High]
This is a pre-existing issue, but does this code overflow the bytes variable?

drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c:amdgpu_bo_create_list_entry_array() {
    ...
		unsigned long bytes = in->bo_number *
			in->bo_info_size;
    ...
		memset(info, 0, in->bo_number * info_size);
    ...
}

Since both in->bo_number and in->bo_info_size are uint32_t from the BO list
ioctl, could multiplying them cause a 32-bit integer overflow?

For example, if bo_number is 0x20000002 and bo_info_size is 8, the result
overflows to 0. Would copy_from_user then copy only 16 bytes while
kvmalloc_array allocates a 4 GB array, leaving gigabytes of kernel heap
memory uninitialized and later used as BO handles?

[Severity: Critical]
This is a pre-existing issue, but does returning 0 for an empty chunk list
leave parser fields uninitialized?

drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c:amdgpu_cs_parser_init() {
    ...
	if (cs->in.num_chunks == 0)
		return 0;
    ...
}

If userspace calls amdgpu_cs_ioctl with num_chunks = 0, this returns early
and skips parser setup, leaving parser->entity as NULL.

Will this cause a NULL pointer dereference later in amdgpu_cs_ib_fill()
when it unconditionally dereferences parser->entity?

drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c:amdgpu_cs_ib_fill() {
    ...
	ring = to_amdgpu_ring(parser->entity->rq->sched);
    ...
}

[Severity: Critical]
This is a pre-existing issue, but is there a missing length check for
AMDGPU_CHUNK_ID_IB chunks?

drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c:amdgpu_cs_parser_init() {
    ...
		case AMDGPU_CHUNK_ID_IB:
			++num_ibs;
			break;
    ...
}

If userspace passes an IB chunk with length_dw = 0, kvmalloc_array could
return ZERO_SIZE_PTR (0x10).

Could this unvalidated pointer then be dereferenced in amdgpu_cs_ib_fill(),
causing a page fault or out-of-bounds heap read?

drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c:amdgpu_cs_ib_fill() {
    ...
		chunk_ib = (struct drm_amdgpu_cs_chunk_ib *)chunk->kdata;

		if (chunk->chunk_id != AMDGPU_CHUNK_ID_IB)
			continue;

		if (chunk_ib->ip_type == AMDGPU_HW_IP_GFX &&
    ...
}

[Severity: Critical]
This is a pre-existing issue, but does this error path lead to a double-free
and use-after-free of the user fence BO?

drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c:amdgpu_cs_user_fence_chunk() {
    ...
error_unref:
	amdgpu_bo_unref(&bo);
	return r;
}

If this error path is taken, the BO is unreferenced, but the dangling pointer
remains in p->uf_entry.tv.bo.

Will the parser cleanup path then blindly unreference the same BO a second time?

drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c:amdgpu_cs_parser_fini() {
    ...
	if (parser->uf_entry.tv.bo) {
		struct amdgpu_bo *uf = ttm_to_amdgpu_bo(parser->uf_entry.tv.bo);

		amdgpu_bo_unref(&uf);
	}
    ...
}

[Severity: Critical]
This is a pre-existing issue, but does setting p->nchunks before checking
if the chunk array allocation succeeded cause a NULL pointer dereference?

drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c:amdgpu_cs_parser_init() {
    ...
	p->nchunks = cs->in.num_chunks;
	p->chunks = kmalloc_array(p->nchunks, sizeof(struct amdgpu_cs_chunk),
			    GFP_KERNEL);
	if (!p->chunks) {
		ret = -ENOMEM;
		goto free_chunk;
	}
    ...
}

If the allocation fails and we branch to free_chunk, p->nchunks is not cleared.

Does this cause the cleanup loop in amdgpu_cs_parser_fini() to iterate over
and dereference the NULL parser->chunks array?

drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c:amdgpu_cs_parser_fini() {
    ...
	for (i = 0; i < parser->nchunks; i++)
		kvfree(parser->chunks[i].kdata);
    ...
}

[Severity: High]
This is a pre-existing issue, but does this code leak the previous user
fence BO if multiple FENCE chunks are provided?

drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c:amdgpu_cs_user_fence_chunk() {
    ...
	bo = amdgpu_bo_ref(gem_to_amdgpu_bo(gobj));
	p->uf_entry.priority = 0;
	p->uf_entry.tv.bo = &bo->tbo;
    ...
}

If userspace submits multiple AMDGPU_CHUNK_ID_FENCE chunks in a single CS
ioctl, this blindly overwrites p->uf_entry.tv.bo with a new BO reference.

Does this permanently leak the previous GEM buffer objects, potentially
leading to system memory exhaustion since amdgpu_cs_parser_fini() only
unreferences the final BO?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914074050.10384-1-roman.demidov.nn@gmail.com?part=1