Re: [PATCH v19 09/14] crypto: qce - Remove unused ignore_buf

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The asynchronous workqueue accesses freed DMA channels, freed memory, and a destroyed mutex during device teardown due to incorrect devres registration order.
- [High] The driver leaks uninitialized kernel heap memory or previous cryptographic results to user space when a hardware error occurs.
- [High] Sensitive 3DES key material is copied to a stack buffer but is not explicitly zeroized before the function returns.
--

commit d001aa784f83034e2199faf1b8d96703d4ecbbb7
Author: Bartosz Golaszewski <brgl@kernel.org>

crypto: qce - Remove unused ignore_buf

This commit removes the unused ignore_buf pointer from the qce_dma_data
struct and moves the QCE_IGNORE_BUF_SZ definition directly into dma.c
since it is not used outside of this compilation unit.

> diff --git a/drivers/crypto/qce/dma.c b/drivers/crypto/qce/dma.c
> index 7ec9d72fd690f..d1daa229361aa 100644
> --- a/drivers/crypto/qce/dma.c
> +++ b/drivers/crypto/qce/dma.c

[ ... ]

> @@ -43,8 +45,6 @@ int devm_qce_dma_request(struct device *dev, struct qce_dma_data *dma)
>  		goto error_nomem;
>  	}
>  
> -	dma->ignore_buf = dma->result_buf + QCE_RESULT_BUF_SZ;
> -

[Severity: High]
This is a pre-existing issue, but does the allocation of result_buf risk
leaking uninitialized kernel heap memory or stale cryptographic results to
user space?

In devm_qce_dma_request(), result_buf is allocated with kmalloc() without
the __GFP_ZERO flag. During asynchronous request completion in
qce_ahash_done() and qce_aead_done(), the driver appears to unconditionally
copy the digest, authentication tag, or IV from these buffers to the caller's
output buffers (e.g., req->result) which often map directly to user space.

If a hardware error occurs and the BAM DMA fails to write to the result
buffer, wouldn't this copy operation expose uninitialized heap memory or
previous request data before qce_check_status() can handle the error?

[Severity: Critical]
This is a pre-existing issue, but I noticed a potential use-after-free
during device teardown related to the devres registration order here.

If qce_crypto_probe() registers the devres action to cancel the
asynchronous workqueue (qce_cancel_work) first, and then calls
devm_qce_dma_request() and devm_mutex_init(), won't the teardown happen
in reverse order?

During device detach, the LIFO devres execution would destroy the mutex
and free the DMA channels and buffers before actually canceling the
workqueue. If a DMA callback schedules qce->done_work just before DMA
termination, could qce_req_done_work() execute concurrently during
teardown, attempting to acquire the destroyed qce->lock and using the
freed DMA resources?

>  	return devm_add_action_or_reset(dev, qce_dma_release, dma);
>  
>  error_nomem:

[ ... ]

> diff --git a/drivers/crypto/qce/dma.h b/drivers/crypto/qce/dma.h
> index 31629185000e1..fc337c435cd14 100644
> --- a/drivers/crypto/qce/dma.h
> +++ b/drivers/crypto/qce/dma.h

[ ... ]

> @@ -31,7 +30,6 @@ struct qce_dma_data {
>  	struct dma_chan *txchan;
>  	struct dma_chan *rxchan;
>  	struct qce_result_dump *result_buf;
> -	void *ignore_buf;
>  };
>  
>  int devm_qce_dma_request(struct device *dev, struct qce_dma_data *dma);

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the
cryptographic operations I noticed an issue in qce_aead_setkey() and
qce_des3_setkey().

Sensitive 3DES key material is copied into a local stack array
u32 _key[6] to check for weak keys. However, the _key array doesn't
seem to be explicitly zeroized using memzero_explicit() before the
function returns.

Does this leave sensitive cryptographic key material on the kernel
stack, potentially allowing it to be leaked through subsequent
uninitialized stack reads?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260526-qcom-qce-cmd-descr-v19-0-08472fdcbf4a@oss.qualcomm.com?part=9