Re: [PATCH AUTOSEL 6.18-6.1] drm/amdkfd: Check bounds for allocate_sdma_queue restore_sdma_id

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing bounds check on user-supplied restore_id in allocate_doorbell can lead to a heap out-of-bounds write.
- [High] Validation bypass in `kfd_ioctl_create_queue` when a device has no SDMA engines, leading to an infinite loop and out-of-bounds memory access.
--

commit f2095bccc592ac2c6dbb0b7629acce3bdd15ede4
Author: David Francis <David.Francis@amd.com>

drm/amdkfd: Check bounds for allocate_sdma_queue restore_sdma_id

The allocate_sdma_queue function lacked bounds checking for a user-specified
SDMA queue ID during CRIU restore. This patch adds checks to ensure the ID
is less than the maximum number of queues, preventing out-of-bounds access.

> 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..842720811599e 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c

[ ... ]

> @@ -1622,6 +1625,9 @@ static int allocate_sdma_queue(struct device_queue_manager *dqm,
>  			return -ENOMEM;
>  		}
>  		if (restore_sdma_id) {
> +			if (*restore_sdma_id >= get_num_xgmi_sdma_queues(dqm))
> +				return -EINVAL;
> +
>  			/* Re-use existing sdma_id */
>  			if (!test_bit(*restore_sdma_id, dqm->xgmi_sdma_bitmap)) {
>  				dev_err(dev, "SDMA queue already in use\n");

[Severity: High]
This isn't a bug introduced by this patch, but there is a validation bypass in
kfd_ioctl_create_queue() when a device has no SDMA engines, which can lead to
an infinite loop and out-of-bounds memory access here in allocate_sdma_queue().

If an unprivileged user calls kfd_ioctl_create_queue() requesting
KFD_QUEUE_TYPE_SDMA_BY_ENG_ID on a device with 0 SDMA engines, both
kfd_get_num_sdma_engines() and kfd_get_num_xgmi_sdma_engines() evaluate to 0.
This makes max_sdma_eng_id in kfd_ioctl_create_queue() evaluate to -1:

drivers/gpu/drm/amd/amdkfd/kfd_chardev.c:kfd_ioctl_create_queue() {
    ...
		int max_sdma_eng_id = kfd_get_num_sdma_engines(dev) +
				      kfd_get_num_xgmi_sdma_engines(dev) - 1;

		if (q_properties.sdma_engine_id > max_sdma_eng_id) {
			err = -EINVAL;
    ...
}

Due to integer promotion, -1 is cast to 0xFFFFFFFF, bypassing the validation.

Execution then continues down to allocate_sdma_queue(), where the user-supplied
sdma_engine_id is assigned to the int variable start_engine:

drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c:allocate_sdma_queue() {
    ...
		start_engine = q->properties.sdma_engine_id - eng_offset;
		for (i = start_engine; i < num_queues; i += num_engines) {

			if (!test_bit(i, is_xgmi ? dqm->xgmi_sdma_bitmap : dqm->sdma_bitmap))
				continue;
    ...
}

With num_queues == 0 and num_engines == 0, if the user-supplied sdma_engine_id
is negative when cast to int (e.g., -2147483648), start_engine becomes negative.
The condition (i < 0) is true and the loop becomes infinite (i += 0). The
test_bit() function is then called with the negative index, leading to an
out-of-bounds memory access.

Could we add a check to ensure max_sdma_eng_id is >= 0 before doing the bounds
check in kfd_ioctl_create_queue()?

[Severity: High]
This is a pre-existing issue, but this patch seems to have missed the
equivalent bounds check for allocate_doorbell().

A user process utilizing the KFD CRIU restore ioctl can provide a malicious
queue data structure with an arbitrarily large doorbell_id. When
create_queue_cpsch() calls allocate_doorbell(), this user-controlled ID
is passed as restore_id without validation:

drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c:allocate_doorbell() {
    ...
			if (__test_and_set_bit(*restore_id, qpd->doorbell_bitmap))
				return -EINVAL;
    ...
}

Since *restore_id isn't checked against KFD_MAX_NUM_OF_QUEUES_PER_PROCESS,
this results in a heap out-of-bounds write on the dynamically allocated
qpd->doorbell_bitmap array. This path is reachable via the KFD_IOC_CRIU_OP
ioctl which requires CAP_CHECKPOINT_RESTORE or CAP_SYS_ADMIN.

Is there a companion patch that adds the missing bounds check for
allocate_doorbell() as well?

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