Re: [PATCH v2] amdkfd: fix doorbell allocation race creating duplicate allocations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] amdkfd: Double-Checked Locking vulnerability exposes unpinned doorbell BO to mmap
- [High] drm/amdkfd: TOCTOU race in doorbell allocation exposing unpinned BO
--

commit 8544c203ebbbee60344c5d18d017125a93e18d7c
Author: Henry Martin <bsdhenrymartin@gmail.com>

amdkfd: fix doorbell allocation race creating duplicate allocations

The commit adds a per-process-device mutex doorbell_mutex inside
kfd_alloc_process_doorbells() to fix a check-and-allocate race condition
where concurrent callers could create duplicate doorbell allocations.

> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c b/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c
> index fdcf7f2d1b5b4..55acadc986bee 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c
> @@ -257,12 +257,21 @@ int kfd_alloc_process_doorbells(struct kfd_dev *kfd, struct kfd_process_device *
>  	int r;
>  	struct qcm_process_device *qpd = &pdd->qpd;
>  
> +	mutex_lock(&pdd->doorbell_mutex);
> +
> +	/* Another thread may have won the check-and-allocate race */
> +	if (qpd->proc_doorbells) {
> +		mutex_unlock(&pdd->doorbell_mutex);
> +		return 0;
> +	}
> +
>  	/* Allocate bitmap for dynamic doorbell allocation */
>  	qpd->doorbell_bitmap = bitmap_zalloc(KFD_MAX_NUM_OF_QUEUES_PER_PROCESS,

[ ... ]

>  	/* Allocate doorbells for this process */
>  	r = amdgpu_bo_create_kernel(kfd->adev,
>  				    kfd_doorbell_process_slice(kfd),
>  				    PAGE_SIZE,
>  				    AMDGPU_GEM_DOMAIN_DOORBELL,
>  				    &qpd->proc_doorbells,
>  				    NULL,
>  				    NULL);
>  	if (r) {
>  		DRM_ERROR("Failed to allocate process doorbells\n");
>  		goto err;
>  	}
> +
> +	mutex_unlock(&pdd->doorbell_mutex);

[Severity: Critical]
Does moving the mutex inside this function create a double-checked locking
vulnerability since the outer check is lockless?

When kfd_get_process_doorbells() is called via mmap, it does a lockless check:

	if (!pdd->qpd.proc_doorbells) {
		if (kfd_alloc_process_doorbells(pdd->dev->kfd, pdd))
			/* phys_addr_t 0 is error */
			return 0;
	}

Concurrently, if another thread is inside kfd_alloc_process_doorbells() and
calls amdgpu_bo_create_kernel() while holding the doorbell_mutex, it assigns
the new BO pointer to &qpd->proc_doorbells before the BO is fully pinned.

Can the thread calling mmap() observe the non-NULL proc_doorbells pointer,
bypass the mutex entirely, and use the unpinned BO?  If it calls
amdgpu_bo_gpu_offset() on the unpinned BO, could it calculate a bogus physical
address and map unintended memory to userspace?

>  	return 0;
>  
>  err:
>  	bitmap_free(qpd->doorbell_bitmap);
>  	qpd->doorbell_bitmap = NULL;
> +unlock:
> +	mutex_unlock(&pdd->doorbell_mutex);
>  	return r;
>  }

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