Re: [PATCH] drm/amdkfd: use kzalloc_objs() instead of kzalloc() with multiply

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Double free of `dqm->fence_mem`, `dqm->hung_db_array`, `dqm->hqd_info` and packet manager due to `dqm->sched_running` remaining true on `start_cpsch` error path.
--

commit 9326059d02e56cf390501c700d0f8e0e83592163
Author: Hrushiraj Gandhi <hrushirajg23@gmail.com>

    drm/amdkfd: use kzalloc_objs() instead of kzalloc() with multiply
    
    dqm->detect_hang_info, dqm->hung_db_array and dqm->hqd_info are all
    allocated with a hand-computed count * sizeof(TYPE) size. If count
    were ever attacker- or firmware-influenced and large enough, the
    multiply could wrap size_t and produce a too-small allocation that
    the code then writes count-many elements into.
    
    kzalloc_objs() computes the same size via size_mul(), which saturates
    to SIZE_MAX on overflow instead of wrapping, so the allocation itself
    fails cleanly (already handled here via the existing NULL checks)
    rather than silently succeeding undersized.
    
    dqm->detect_hang_info_size is left as-is since it is also used later
    for a memset() call; only the allocation itself is converted.
    
    No functional change in the non-overflowing case.
    
    Signed-off-by: Hrushiraj Gandhi <hrushirajg23@gmail.com>

> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> index 69cd4441e240c..488176e947c3e 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> @@ -2160,17 +2160,15 @@ static int start_cpsch(struct device_queue_manager *dqm)
>  			      NUM_XCC(dqm->dev->xcc_mask);
>  
>  	dqm->detect_hang_info_size = num_hw_queue_slots * sizeof(struct dqm_detect_hang_info);
> -	dqm->detect_hang_info = kzalloc(dqm->detect_hang_info_size, GFP_KERNEL);
> +	dqm->detect_hang_info = kzalloc_objs(*dqm->detect_hang_info, num_hw_queue_slots);
>  
>  	if (!dqm->detect_hang_info) {
>  		retval = -ENOMEM;
>  		goto fail_detect_hang_buffer;

[Severity: High]
This isn't a bug introduced by this patch, but does this error path leave
dqm->sched_running set to true?

Earlier in start_cpsch(), dqm->sched_running is set to true. When this
allocation fails, the error path executes kfd_gtt_sa_free() on dqm->fence_mem
and uninitializes the packet manager:

fail_detect_hang_buffer:
	kfd_gtt_sa_free(dqm->dev, dqm->fence_mem);
fail_allocate_vidmem:
fail_set_sched_resources:
	if (!dqm->dev->kfd->shared_resources.enable_mes)
		pm_uninit(&dqm->packet_mgr);

Because it fails to clear dqm->sched_running or set dqm->fence_mem to NULL,
subsequent cleanup via stop_cpsch() will proceed rather than exiting early.
This causes stop_cpsch() to call kfd_gtt_sa_free() on the dangling fence_mem
pointer and pm_uninit() a second time. The second pm_uninit() call passes a
NULL pointer which gets dereferenced, leading to a kernel panic.

Additionally, stop_cpsch() frees dqm->hung_db_array and dqm->hqd_info without
nullifying them, meaning a subsequent start/stop cycle could trigger a double
free on those arrays as well.

Could dqm->sched_running be set to false and the freed pointers cleared to
NULL in this error path to avoid the double free?

>  	}
>  
> -	dqm->hung_db_array = kzalloc(hung_array_size * sizeof(u32), GFP_KERNEL);
> -	dqm->hqd_info = kzalloc(
> -		hqd_info_size * sizeof(struct amdgpu_mes_hung_queue_hqd_info),
> -		GFP_KERNEL);
> +	dqm->hung_db_array = kzalloc_objs(*dqm->hung_db_array, hung_array_size);
> +	dqm->hqd_info = kzalloc_objs(*dqm->hqd_info, hqd_info_size);
>  
>  	dqm_unlock(dqm);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901072615.522925-1-hrushirajg23@gmail.com?part=1