Re: [PATCH v2 6/7] crypto: qce - reject a negative scatterlist count in CCM AAD preparation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] crypto: qce: late sg_nents_for_len() check after
  scatterwalk_ffwd()
--

commit 68e95033f32d45aeb2e2a9a17114843648589d9a
Author: ZhaoJinming <zhaojinming@uniontech.com>

crypto: qce - reject a negative scatterlist count in CCM AAD preparation

This commit checks the return value of sg_nents_for_len(req->src, totallen)
in qce_aead_ccm_prepare_buf_assoclen() before passing the entry count to
sg_alloc_table().

> diff --git a/drivers/crypto/qce/aead.c b/drivers/crypto/qce/aead.c
> index 00c5924c057e3..f75e3827d8bcb 100644
> --- a/drivers/crypto/qce/aead.c
> +++ b/drivers/crypto/qce/aead.c
> @@ -186,6 +186,7 @@ qce_aead_ccm_prepare_buf_assoclen(struct aead_request *req)
>  	unsigned int adata_header_len, cryptlen, totallen;
>  	gfp_t gfp;
>  	bool diff_dst;
> +	int nents;
>  	int ret;
>  
>  	if (IS_DECRYPT(rctx->flags))
> @@ -231,10 +232,16 @@ qce_aead_ccm_prepare_buf_assoclen(struct aead_request *req)
>  
>  	diff_dst = (req->src != req->dst) ? true : false;
>  
> +	nents = sg_nents_for_len(req->src, totallen);
> +	if (nents < 0) {
> +		ret = nents;
> +		goto err_free_adata;
> +	}
> +
>  	if (diff_dst)
> -		rctx->src_nents = sg_nents_for_len(req->src, totallen) + 1;
> +		rctx->src_nents = nents + 1;
>  	else
> -		rctx->src_nents = sg_nents_for_len(req->src, totallen) + 2;
> +		rctx->src_nents = nents + 2;
>  
>  	gfp = (req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP) ? GFP_KERNEL : GFP_ATOMIC;
>  	ret = sg_alloc_table(&rctx->src_tbl, rctx->src_nents, gfp);

[Severity: High]
Is this sg_nents_for_len(req->src, totallen) check placed too late in
qce_aead_ccm_prepare_buf_assoclen()?

Earlier in the function, req->src is already walked and copied before
reaching this check:

drivers/crypto/qce/aead.c:qce_aead_ccm_prepare_buf_assoclen() {
	...
	totallen = cryptlen + req->assoclen;

	/* Get the msg */
	msg_sg = scatterwalk_ffwd(__sg, req->src, req->assoclen);
	...
	/* Copy the associated data */
	if (sg_copy_to_buffer(req->src, sg_nents_for_len(req->src, assoclen),
			      rctx->adata + adata_header_len,
			      assoclen) != assoclen) {
	...
}

If req->src is shorter than req->assoclen, scatterwalk_ffwd() walks past the
end of the scatterlist and triggers a NULL pointer dereference before the new
check is reached, and sg_copy_to_buffer() also receives an unchecked negative
return value from sg_nents_for_len(req->src, assoclen) as its unsigned int
nents argument.  Checking sg_nents_for_len(req->src, totallen) immediately
after computing totallen would validate req->src before scatterwalk_ffwd(),
the rctx->adata allocation, and sg_copy_to_buffer() run.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-fix-qce-ccm-adata-leak-v2-0-914501377a62@uniontech.com?part=6