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