Re: [PATCH 6/6] dmaengine: idxd: Do not release interrupt handles on terminate_all

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

sashiko-bot@kernel.org writes:

> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] dma: idxd: sleep in atomic context in device_terminate_all()
> - [High] dmaengine: idxd: Hardware UAF and IOMMU faults due to flushing active WQ without disable
> - [Medium] dmaengine: idxd: missing check for INVALID_INT_HANDLE in release
> --
>
> commit dc7ea0d446f4b8c4e4f29fa4595acabcca6b297f
> Author: Vinicius Costa Gomes <vinicius.gomes@intel.com>
>
> dmaengine: idxd: Do not release interrupt handles on terminate_all
>
> This commit splits the interrupt handle teardown from the descriptor
> flushing in idxd_wq_flush_descs(). This prevents the interrupt handle
> from being incorrectly released during dmaengine terminate_all, which
> was causing subsequent runs to time out.
>
>> diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
>> index 131138483b877..06299f40f5312 100644
>> --- a/drivers/dma/idxd/device.c
>> +++ b/drivers/dma/idxd/device.c
>> @@ -1349,6 +1349,16 @@ void idxd_wq_free_irq(struct idxd_wq *wq)
>>  }
>>  
>>  void idxd_wq_flush_descs(struct idxd_wq *wq)
>> +{
>> +	guard(mutex)(&wq->wq_lock);
>> +
>> +	if (wq->state != IDXD_WQ_ENABLED || wq->type != IDXD_WQT_KERNEL)
>> +		return;
>> +
>> +	idxd_flush_pending_descs(&wq->ie);
>> +}
>
> [Severity: High]
> Does this code introduce a sleep-in-atomic bug when called via the
> dmaengine termination path?
>
> The dmaengine API allows dmaengine_terminate_async() to be called from
> atomic context (such as an interrupt handler or while holding a spinlock).
>
> This routes to idxd_dma_terminate_all(), which then calls
> idxd_wq_flush_descs().
>
> Because idxd_wq_flush_descs() unconditionally acquires wq->wq_lock using
> a sleepable mutex, terminating an asynchronous transfer from atomic
> context could trigger a scheduling-while-atomic panic.
>

Ugh, bad one. Worried that my lockdep tests didn't catch this one. Will
fix.

> [Severity: High]
> Can this flush implementation cause a use-after-free and IOMMU faults?
>
> By calling idxd_flush_pending_descs() without first disabling or halting
> the hardware workqueue, the software descriptors are completed, freed,
> and client memory is unmapped while the hardware may still be actively
> processing them.
>
> Since the hardware continues to execute DMA operations into buffers that
> have already been unmapped, can this result in memory corruption and
> IOMMU faults?
>
>> +
>> +void idxd_wq_release_int_handle(struct idxd_wq *wq)
>>  {
>>  	struct idxd_irq_entry *ie = &wq->ie;
>>  	struct idxd_device *idxd = wq->idxd;
>> @@ -1358,7 +1368,6 @@ void idxd_wq_flush_descs(struct idxd_wq *wq)
>>  	if (wq->state != IDXD_WQ_ENABLED || wq->type != IDXD_WQT_KERNEL)
>>  		return;
>>  
>> -	idxd_flush_pending_descs(ie);
>>  	if (idxd->request_int_handles)
>>  		idxd_device_release_int_handle(idxd, ie->int_handle,
>> IDXD_IRQ_MSIX);

Another pre-existing one. And another potential to be in another series.
I think I might have caught a consequence of this bug when testing
iaa-crypto. Let's see if the fix fits on v2.

>
> [Severity: Medium]
> Is it possible for ie->int_handle to be INVALID_INT_HANDLE here?
>
> If a previous operation leaves the handle as INVALID_INT_HANDLE (-1) but
> the workqueue remains in the IDXD_WQ_ENABLED state, this code will
> unconditionally issue a release command to the hardware using -1 (0xffff).
>
> Should this function include a check for ie->int_handle ==
> INVALID_INT_HANDLE before calling idxd_device_release_int_handle(),
> similar to how it is handled in idxd_wq_free_irq()?
>
>>  	idxd_device_clear_perm_entry(idxd, ie);
>

Will fix.


Cheers,
-- 
Vinicius