Re: [PATCH v5 2/2] crypto: ti: Add driver for DTHE V2 AES Engine (ECB, CBC)

Re: [PATCH v5 2/2] crypto: ti: Add driver for DTHE V2 AES Engine (ECB, CBC)

[Herbert Xu]

On Tue, Jun 03, 2025 at 06:07:29PM +0530, T Pratham wrote:
>
> +	// Need to do a timeout to ensure finalise gets called if DMA callback fails for any reason
> +	ret = wait_for_completion_timeout(&rctx->aes_compl, msecs_to_jiffies(DTHE_DMA_TIMEOUT_MS));

This doesn't look safe.  What if the callback is invoked after a
timeout? That would be a UAF.

Does the DMA engine provide any timeout mechanism? If not, then
you could do it with a delayed work struct.  Just make sure that
you cancel the work struct in the normal path callback.  Vice versa
you need to terminate the DMA job in the timeout work struct.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v5 2/2] crypto: ti: Add driver for DTHE V2 AES Engine (ECB, CBC)

[T Pratham]

On 11/06/25 15:23, Herbert Xu wrote:
> On Tue, Jun 03, 2025 at 06:07:29PM +0530, T Pratham wrote:
>>
>> +	// Need to do a timeout to ensure finalise gets called if DMA callback fails for any reason
>> +	ret = wait_for_completion_timeout(&rctx->aes_compl, msecs_to_jiffies(DTHE_DMA_TIMEOUT_MS));
> 
> This doesn't look safe.  What if the callback is invoked after a
> timeout? That would be a UAF.
> 
> Does the DMA engine provide any timeout mechanism? If not, then
> you could do it with a delayed work struct.  Just make sure that
> you cancel the work struct in the normal path callback.  Vice versa
> you need to terminate the DMA job in the timeout work struct.
> 
> Cheers,

Calling dma_terminate_sync() here should suffice I presume? I'll update the code accordingly.

Regards
T Pratham <t-pratham@ti.com>

Re: [PATCH v5 2/2] crypto: ti: Add driver for DTHE V2 AES Engine (ECB, CBC)

[Herbert Xu]

On Thu, Jun 12, 2025 at 03:45:54PM +0530, T Pratham wrote:
>
> Calling dma_terminate_sync() here should suffice I presume? I'll update the code accordingly.

This is just me having a skim through the DMA engine API.  Vinod,
could you please confirm whether this is how timeouts should be
dealt with through the DMA engine?

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v5 2/2] crypto: ti: Add driver for DTHE V2 AES Engine (ECB, CBC)

[Vinod Koul]

On 11-06-25, 17:53, Herbert Xu wrote:
> On Tue, Jun 03, 2025 at 06:07:29PM +0530, T Pratham wrote:
> >
> > +	// Need to do a timeout to ensure finalise gets called if DMA callback fails for any reason
> > +	ret = wait_for_completion_timeout(&rctx->aes_compl, msecs_to_jiffies(DTHE_DMA_TIMEOUT_MS));
> 
> This doesn't look safe.  What if the callback is invoked after a
> timeout? That would be a UAF.
> 
> Does the DMA engine provide any timeout mechanism? If not, then
> you could do it with a delayed work struct.  Just make sure that
> you cancel the work struct in the normal path callback.  Vice versa
> you need to terminate the DMA job in the timeout work struct.

Typically no. Most of the hardware may not have capability, so we have
apis to terminate.

-- 
~Vinod