Re: [PATCH 2/4] crypto: iaa - fall back to software for multi-entry scatterlists

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

sashiko-bot@kernel.org writes:

> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
>
> New issues:
> - [High] Hardcoding the `nents` parameter to 1 in `dma_map_sg` causes
> a kernel panic if `req->dst` is NULL.

Upper layers tries to not give us req->dst being NULL, but will be a bit
more defensive.

> - [Medium] The implementation tracks fallback calls rather than processed bytes, contradicting the commit message.
>

Will fix the commit message.

> Pre-existing issues:
> - [High] Software fallback writes to CPU memory while DMA bounce buffers are mapped, risking data corruption during unmap.
> - [High] A timeout during hardware polling causes the CPU to free DMA structures while the hardware device may still be performing DMA, leading to Use-After-Free memory corruption.
> --
>
> --- Patch [2]: [PATCH 2/4] crypto: iaa - fall back to software for multi-entry scatterlists ---
> commit 6f1532575ac946d82236e0ac6dffd05320f89063
> Author: Giovanni Cabiddu <giovanni.cabiddu@intel.com>
> Subject: crypto: iaa - fall back to software for multi-entry scatterlists
>
> This commit adds a software fallback for the IAA driver when dealing with
> scatterlists containing multiple entries, as the hardware cannot process
> them directly. It simplifies the DMA mapping path to expect a single entry
> and adds counters for the fallback path.
>
>>     Add counters to track the number of bytes processed by the software
>>     implementation on the compression direction.
>
> [Severity: Medium]
> The commit message states that counters are added to track the number of
> bytes processed. However, looking at the updates in iaa_crypto_stats.c:
>
> void update_total_sw_comp_calls(void)
> {
>         atomic64_inc(&total_sw_comp_calls);
> }
>
> Could it be that the implementation only tracks fallback calls rather than
> the processed bytes?
>
>> diff --git a/drivers/crypto/intel/iaa/iaa_crypto_main.c b/drivers/crypto/intel/iaa/iaa_crypto_main.c
>> index f62b994e18e58..fb154959c2aa8 100644
>> --- a/drivers/crypto/intel/iaa/iaa_crypto_main.c
>> +++ b/drivers/crypto/intel/iaa/iaa_crypto_main.c
> [ ... ]
>> @@ -983,17 +984,43 @@ static inline int check_completion(struct device *dev,
>>  	return ret;
>>  }
>
> [Severity: High]
> This is a pre-existing issue, but how does the driver handle hardware
> DMA after a timeout?
>
> In check_completion(), if the polling loop exceeds IAA_COMPLETION_TIMEOUT,
> it returns -ETIMEDOUT:
>
> check_completion() {
>     while (!comp->status) {
>         if (status_checks++ >= IAA_COMPLETION_TIMEOUT) {
>             ret = -ETIMEDOUT;
>             goto out;
>         }
>         ...
>     }
> }
>
> When this timeout happens, the caller immediately frees the IDXD descriptor
> and unmaps the DMA buffers. However, the hardware might still be actively
> processing the request. If the delayed hardware eventually performs a DMA
> write to the destination address after the CPU has reallocated that memory
> for something else, could this cause silent memory corruption?
>
>> -static int deflate_generic_decompress(struct acomp_req *req)
>> +static int deflate_fallback(struct acomp_req *req, bool compress)
> [ ... ]
>> +static int deflate_generic_decompress(struct acomp_req *req)
>> +{
>> +	int ret;
>> +
>> +	ret = deflate_fallback(req, false);
>> +	if (ret)
>> +		return ret;
>> +
>>  	update_total_sw_decomp_calls();
>>  
>> -	return ret;
>> +	return 0;
>>  }
>
> [Severity: High]
> This is a pre-existing issue, but I noticed a potential problem when
> deflate_generic_decompress() is called as a fallback from iaa_decompress().
>
> If a hardware error like IAA_ANALYTICS_ERROR occurs in iaa_decompress(),
> the software fallback writes to the CPU virtual mapping of req->dst:
>
> iaa_decompress() {
>     ...
>     if (idxd_desc->iax_completion->status == IAA_ANALYTICS_ERROR) {
>         ret = deflate_generic_decompress(req);
>         ...
>     }
> }
>
> Since req->dst is still mapped for DMA_FROM_DEVICE at this point,
> when dma_unmap_sg() is eventually called, might it synchronize memory
> from a bounce buffer over the original CPU buffer, overwriting the
> software fallback's output? Should the scatterlist be unmapped before
> invoking the software fallback?
>
> [ ... ]
>> @@ -1507,30 +1538,25 @@ static int iaa_comp_acompress(struct acomp_req *req)
> [ ... ]
>> -	nr_sgs = dma_map_sg(dev, req->dst, sg_nents(req->dst), DMA_FROM_DEVICE);
>> -	if (nr_sgs <= 0 || nr_sgs > 1) {
> [ ... ]
>> +	if (!dma_map_sg(dev, req->dst, 1, DMA_FROM_DEVICE)) {
>
> [Severity: High]
> If req->dst is NULL, sg_nents() will return 0, bypassing the fallback check
> earlier in this function. Calling dma_map_sg() unconditionally with nents=1
> will then attempt to dereference the NULL scatterlist pointer.
>
> Would it be safer to verify req->dst is not NULL before attempting to map
> it, preserving the old behavior where sg_nents(req->dst) returning 0 was
> handled safely?
>
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260713-iaa-crypto-fixes-zswap-v1-0-65cac23c684d@intel.com?part=2

-- 
Vinicius