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

[sashiko-bot@kernel.org]

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.
- [Medium] The implementation tracks fallback calls rather than processed bytes, contradicting 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