Re: [PATCH 3/4] crypto: iaa - avoid counting fallback decompression bytes

[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:
>
> Pre-existing issues:
> - [High] Pre-existing DMA API violation: software fallback
> (`deflate_generic_decompress`) writes to the destination scatterlist
> while it is still mapped for DMA (`DMA_FROM_DEVICE`), leading to
> silent data corruption when unmapped if SWIOTLB or confidential
> computing is active.

Even if this is pre-existing, will think a bit more about this one, and
see if I can fix this more or less easily.

> --
>
> --- Patch [3]: [PATCH 3/4] crypto: iaa - avoid counting fallback decompression bytes ---
> commit 589e7e250edf8839b3b071db5475eb3581825d01
> Author: Giovanni Cabiddu <giovanni.cabiddu@intel.com>
>
> crypto: iaa - avoid counting fallback decompression bytes
>
> When decompression falls back to deflate-generic after an analytics
> error, the request no longer completes through IAA. Move decompression
> byte accounting into the successful IAA completion path in both the
> synchronous and asynchronous flows so decomp_bytes only reflects bytes
> actually processed by IAA.
>
>> diff --git a/drivers/crypto/intel/iaa/iaa_crypto_main.c b/drivers/crypto/intel/iaa/iaa_crypto_main.c
>> index fb154959c2aa8..8f68b1478476e 100644
>> --- a/drivers/crypto/intel/iaa/iaa_crypto_main.c
>> +++ b/drivers/crypto/intel/iaa/iaa_crypto_main.c
>> @@ -1084,15 +1084,17 @@ static void iaa_desc_complete(struct idxd_desc *idxd_desc,
>>  		}
>>  	} else {
>
> [Severity: High]
> This is a pre-existing issue, but does calling deflate_generic_decompress()
> in the error path just above this block violate the DMA API?
>
> In iaa_desc_complete(), if check_completion() fails with IAA_ANALYTICS_ERROR,
> the driver falls back to software decompression:
>
> drivers/crypto/intel/iaa/iaa_crypto_main.c:iaa_desc_complete() {
>     ...
>     ret = deflate_generic_decompress(ctx->req);
>     ...
> err:
>     dma_unmap_sg(dev, ctx->req->dst, sg_nents(ctx->req->dst), DMA_FROM_DEVICE);
>     ...
> }
>
> Because the scatterlist is still mapped for DMA, won't dma_unmap_sg()
> overwrite the CPU's decompression work with the bounce buffer contents
> if SWIOTLB or confidential computing is active?
>
>>  		ctx->req->dlen = idxd_desc->iax_completion->output_size;
>> +
>> +		if (!ctx->compress) {
>> +			update_total_decomp_bytes_in(ctx->req->slen);
>> +			update_wq_decomp_bytes(iaa_wq->wq, ctx->req->slen);
>> +		}
>>  	}
>
> [ ... ]
>
>> @@ -1475,16 +1477,16 @@ static int iaa_decompress(struct crypto_tfm *tfm, struct acomp_req *req,
>>  		}
>>  	} else {
>
> [Severity: High]
> This is a pre-existing issue, but is there a similar DMA API violation here?
>
> In iaa_decompress(), the synchronous path executes the software fallback
> and returns to the caller:
>
> drivers/crypto/intel/iaa/iaa_crypto_main.c:iaa_decompress() {
>     ...
>     ret = deflate_generic_decompress(req);
>     ...
> out:
>     return ret;
> }
>
> Does this mean the caller will subsequently unmap the scatterlist and
> unknowingly overwrite the fallback's data when SWIOTLB is active?
>
>>  		req->dlen = idxd_desc->iax_completion->output_size;
>> +
>> +		/* Update stats */
>> +		update_total_decomp_bytes_in(slen);
>> +		update_wq_decomp_bytes(wq, slen);
>>  	}
>
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260713-iaa-crypto-fixes-zswap-v1-0-65cac23c684d@intel.com?part=3

-- 
Vinicius