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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Software fallback decompression executes while scatterlist buffers are still mapped for DMA, leading to data corruption.
- [High] TOCTOU race condition leading to Use-After-Free of `wq_table` during device unbind.
- [High] Hardware DMA writes to freed memory if `check_completion` times out.
--

--- Patch [3]: [PATCH v2 3/5] crypto: iaa - avoid counting fallback decompression bytes ---
commit 4c267bc9414930f6e0fce0b302c075f74ec2f4ff
Author: Giovanni Cabiddu <[email protected]>

crypto: iaa - avoid counting fallback decompression bytes

This commit moves decompression byte accounting into the successful IAA
completion path in both synchronous and asynchronous flows. This ensures
decomp_bytes only reflects bytes actually processed by IAA, avoiding counting
fallback decompression bytes when an analytics error occurs.

> diff --git a/drivers/crypto/intel/iaa/iaa_crypto_main.c b/drivers/crypto/intel/iaa/iaa_crypto_main.c
> index 904d9413ba182..9505ca23e6f38 100644
> --- a/drivers/crypto/intel/iaa/iaa_crypto_main.c
> +++ b/drivers/crypto/intel/iaa/iaa_crypto_main.c
> @@ -1071,15 +1071,17 @@ static void iaa_desc_complete(struct idxd_desc *idxd_desc,
>  		}
>  	} else {
>  		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);
> +		}
>  	}

[Severity: Critical]
This is a pre-existing issue, but can the software fallback decompression
execute while scatterlist buffers are still mapped for DMA, leading to data
corruption?

When the hardware engine returns IAA_ANALYTICS_ERROR and the operation
falls back to deflate_generic_decompress(), does it execute before
dma_unmap_sg() is called?

If the platform uses SWIOTLB bounce buffers, the hardware-untouched bounce
buffer could be copied back to the actual memory pages during unmap,
completely overwriting the CPU-decompressed valid data.

[ ... ]

> @@ -1462,16 +1464,16 @@ static int iaa_decompress(struct crypto_tfm *tfm, struct acomp_req *req,
>  		}
>  	} else {
>  		req->dlen = idxd_desc->iax_completion->output_size;
> +
> +		/* Update stats */
> +		update_total_decomp_bytes_in(slen);
> +		update_wq_decomp_bytes(wq, slen);
>  	}
>  
>  	*dlen = req->dlen;
>  
>  	if (!ctx->async_mode)
>  		idxd_free_desc(wq, idxd_desc);

[Severity: High]
This is a pre-existing issue, but does a timeout in check_completion()
cause the hardware DMA to write to freed memory?

In check_completion(), if the hardware fails to complete the descriptor
within IAA_COMPLETION_TIMEOUT polls, the driver sets
iaa_crypto_enabled = false and returns -ETIMEDOUT.

The caller then frees the IDXD software descriptor and returns an error,
which leads to unmapping and freeing the SG buffers without aborting or
quiescing the hardware engine.

If the hardware is merely stalled and eventually completes the operation,
will it perform asynchronous DMA writes to the now-freed req->dst physical
pages, and write the completion status to the re-allocated descriptor?

[Severity: High]
This is a pre-existing issue, but is there a TOCTOU race condition
leading to a Use-After-Free of wq_table during device unbind?

The fast-paths like iaa_comp_acompress() check the global
iaa_crypto_enabled flag without locks:

	if (!iaa_crypto_enabled) {
		pr_debug("iaa_crypto disabled, not compressing\n");
		return -ENODEV;
	}
...
	cpu = get_cpu();
	wq = wq_table_next_wq(cpu);

Concurrently, iaa_crypto_remove() can be invoked via device unbind,
setting iaa_crypto_enabled = false and freeing the wq_table pointer array.

Since the crypto algorithms are only unregistered during module exit,
they remain accessible. Can a concurrent compression request read
iaa_crypto_enabled == true, get preempted, and then access the freed
wq_table when calling wq_table_next_wq()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3