Re: [PATCH 4/6] dmaengine: idxd: Fix unwind bounds in device registration error paths

[Vinicius Costa Gomes <vinicius.gomes@intel.com>]

sashiko-bot@kernel.org writes:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] dmaengine: idxd: UAF/double-free on device registration failure
> --
>
> commit 571be93356be3fdc63d0506588c4e560e5d9eccc
> Author: Vinicius Costa Gomes <vinicius.gomes@intel.com>
>
> dmaengine: idxd: Fix unwind bounds in device registration error paths
>
> When registering the per-idxd wq, engine, or group devices, a failure partway
> through must roll back exactly the devices already added. This commit fixes
> an underflow loop by starting the rollback at i instead of i - 1.
>
>> diff --git a/drivers/dma/idxd/sysfs.c b/drivers/dma/idxd/sysfs.c
>> index d5ffc641c8567..e828292e5fb88 100644
>> --- a/drivers/dma/idxd/sysfs.c
>> +++ b/drivers/dma/idxd/sysfs.c
>> @@ -1876,7 +1876,7 @@ static int idxd_register_engine_devices(struct idxd_device *idxd)
>>  	return 0;
>>  
>>  cleanup:
>> -	j = i - 1;
>> +	j = i;
>>  	for (; i < idxd->max_engines; i++) {
>>  		engine = idxd->engines[i];
>>  		put_device(engine_confdev(engine));
>
> [Severity: High]
> Does this error handling sequence lead to a double-free of the child devices?
>
> If device registration fails partway, this local cleanup path drops the refcounts
> on the child device kobjects via put_device() and device_unregister(), which
> frees them.
>
> The failure then cascades up to idxd_register_devices(), which rolls back
> previously registered child devices (like WQs), also freeing them. Crucially,
> the pointers in idxd->engines and idxd->wqs are left dangling.
>
> Then, the error is returned to idxd_pci_probe_alloc(), which unwinds by jumping
> to err_dev_register and explicitly calling idxd_cleanup():
>
> drivers/dma/idxd/init.c:idxd_pci_probe_alloc() {
>     ...
> 	rc = idxd_register_devices(idxd);
> 	if (rc) {
> 		dev_err(dev, "IDXD sysfs setup failed\n");
> 		goto err_dev_register;
> 	}
>     ...
>  err_dev_register:
> 	idxd_cleanup(idxd);
>     ...
> }
>
> idxd_cleanup() invokes idxd_cleanup_internals(), which iterates over the arrays
> via idxd_clean_engines() and calls put_device() again on the dangling pointers:
>
> drivers/dma/idxd/init.c:idxd_clean_engines() {
> 	for (i = 0; i < idxd->max_engines; i++) {
> 		conf_dev = engine_confdev(idxd->engines[i]);
> 		put_device(conf_dev);
> 	}
> }
>
> Could this secondary cleanup access the freed device pointer and execute a
> double-put, causing a use-after-free?
>
> [ ... ]

This wasn't easy to parse at all. But I think (with help from AI, kind
of funny) that I get it now. This is an pre-existing issue, will try to
fix on v2 unless it makes the series become too messy.


Cheers,
-- 
Vinicius