Re: [PATCH AUTOSEL 6.18-6.1] drm/amdkfd: Check bounds on allocate_doorbell

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing bounds check on user-supplied `restore_sdma_id` during CRIU restore causes out-of-bounds bit operations on `sdma_bitmap` and `xgmi_sdma_bitmap`.
- [High] Missing bounds check on restore_id causes out-of-bounds array access to page_slots array during CRIU event restore.
- [High] Memory leak of gang_ctx_bo and wptr_bo_gart when queue creation fails.
- [Medium] User-triggerable WARN(1) by supplying an invalid queue type during CRIU restore.
--

commit 075dbb15b0e52c82ca15eafa0439de9c1173bea4
Author: David Francis <David.Francis@amd.com>

drm/amdkfd: Check bounds on allocate_doorbell
    
allocated_doorbell has an option to set the doorbell id
to a specific value (used by CRIU). This value was not
bounds checked.
    
Check to confirm it's less than KFD_MAX_NUM_OF_QUEUES_PER_PROCESS.

> 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 6e7bc983fc0b6..394ed3d3ce26a 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> @@ -472,6 +472,9 @@ static int allocate_doorbell(struct qcm_process_device *qpd,
>  	} else {
>  		/* For CP queues on SOC15 */
>  		if (restore_id) {
> +			if (*restore_id >= KFD_MAX_NUM_OF_QUEUES_PER_PROCESS)
> +				return -EINVAL;
> +
>  			/* make sure that ID is free  */
>  			if (__test_and_set_bit(*restore_id, qpd->doorbell_bitmap))
>  				return -EINVAL;

[Severity: High]
This isn't a bug introduced by this patch, but does the same missing bounds 
check issue exist in allocate_sdma_queue() for restore_sdma_id?

drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c:allocate_sdma_queue() {
    ...
    if (restore_sdma_id) {
        /* Re-use existing sdma_id */
        if (!test_bit(*restore_sdma_id, dqm->sdma_bitmap)) {
            dev_err(dev, "SDMA queue already in use\n");
            return -EBUSY;
        }
        clear_bit(*restore_sdma_id, dqm->sdma_bitmap);
    ...
}

And also further down in the same function for the xgmi_sdma_bitmap?

drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c:allocate_sdma_queue() {
    ...
    if (restore_sdma_id) {
        /* Re-use existing sdma_id */
        if (!test_bit(*restore_sdma_id, dqm->xgmi_sdma_bitmap)) {
            dev_err(dev, "SDMA queue already in use\n");
            return -EBUSY;
        }
        clear_bit(*restore_sdma_id, dqm->xgmi_sdma_bitmap);
    ...
}

Since restore_sdma_id is provided by userspace during a CRIU restore via 
kfd_criu_restore_queue()->pqm_create_queue()->create_queue_cpsch(), can an 
out-of-bounds ID cause out-of-bounds memory read and write operations on the 
statically sized sdma_bitmap arrays?


[Severity: High]
This is a pre-existing issue, but is there a similar missing bounds check 
for restore_id during CRIU event restore?

drivers/gpu/drm/amd/amdkfd/kfd_events.c:allocate_event_notification_slot() {
    ...
    if (restore_id) {
        id = idr_alloc(&p->event_idr, ev, *restore_id, *restore_id + 1,
                       GFP_KERNEL);
    }
    ...
    ev->event_id = id;
    page_slots(p->signal_page)[id] = UNSIGNALED_EVENT_SLOT;
    ...
}

During CRIU restore, kfd_criu_restore_event() passes the userspace-supplied 
restore_id to create_signal_event()->allocate_event_notification_slot(). 
If restore_id is >= KFD_SIGNAL_EVENT_LIMIT, will this cause an out-of-bounds 
write to the page_slots array?


[Severity: High]
This is a pre-existing issue, but does this code leak the gang_ctx_bo and 
wptr_bo_gart when queue creation fails?

drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c:init_user_queue() {
    ...
    retval = amdgpu_amdkfd_alloc_gtt_mem(dev->adev,
                                         AMDGPU_MES_GANG_CTX_SIZE,
                                         &(*q)->gang_ctx_bo,
                                         &(*q)->gang_ctx_gpu_addr,
                                         &(*q)->gang_ctx_cpu_ptr,
                                         false);
    ...
}

If dev->dqm->ops.create_queue() subsequently fails (for example, due to an 
invalid doorbell ID being rejected by your new bounds check), we jump to the 
err_create_queue path:

drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c:pqm_create_queue() {
    ...
err_create_queue:
    uninit_queue(q);
    if (kq)
        kernel_queue_uninit(kq);
    kfree(pqn);
    ...
}

Because pqm_clean_queue_resource() is not called here, will this result in a 
permanent leak of the MES GPU memory allocations? A privileged user could 
potentially trigger this path repeatedly by passing invalid doorbell IDs 
during CRIU restore.


[Severity: Medium]
This is a pre-existing issue, but can a user trigger a kernel WARN by 
supplying an invalid queue type during CRIU restore?

drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c:pqm_create_queue() {
    ...
    switch (type) {
    ...
    default:
        WARN(1, "Invalid queue type %d", type);
        retval = -EINVAL;
    }
    ...
}

Since kfd_criu_restore_queue() copies the queue type from the userspace 
q_data, could a malicious checkpoint file hit this WARN(1)? If a system 
runs with panic_on_warn enabled, could this translate into a denial of 
service?

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