Re: [PATCH v2 22/30] crypto: sa2ul - validate scatterlist setup and unwind DMA mappings

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] crypto: sa2ul: sa_run() passes 0 nents to dma_map_sgtable() when size is 0
--

commit 615706b20f3b90ba270a289c49acc68499b974e6
Author: Manorit Chawdhry <m-chawdhry@ti.com>

crypto: sa2ul - validate scatterlist setup and unwind DMA mappings

This commit validates source and destination scatterlist lengths with
sg_nents_for_len(), aborts on sg_split() failures, and routes partial setup
errors in sa_run() through sa_free_sa_rx_data().

> diff --git a/drivers/crypto/sa2ul.c b/drivers/crypto/sa2ul.c
> index 2964aa44d50e4..b33fac66c93fc 100644
> --- a/drivers/crypto/sa2ul.c
> +++ b/drivers/crypto/sa2ul.c
> @@ -1071,6 +1071,10 @@ static int sa_run(struct sa_req *req)
>  	 */
>  	src = req->src;
>  	sg_nents = sg_nents_for_len(src, req->size);
> +	if (sg_nents < 0) {
> +		ret = sg_nents;
> +		goto err_cleanup;
> +	}
>  
>  	split_size = req->size;
>  
> @@ -1086,7 +1090,7 @@ static int sa_run(struct sa_req *req)
>  		mapped_sg->sgt.orig_nents = src_nents;
>  		ret = dma_map_sgtable(ddev, &mapped_sg->sgt, dir_src, 0);
>  		if (ret) {
> -			kfree(rxd);
> +			sa_free_sa_rx_data(rxd);
>  			return ret;
>  		}
>  
> @@ -1097,7 +1101,7 @@ static int sa_run(struct sa_req *req)
>  		mapped_sg->sgt.orig_nents = sg_nents;
>  		ret = dma_map_sgtable(ddev, &mapped_sg->sgt, dir_src, 0);
>  		if (ret) {
> -			kfree(rxd);
> +			sa_free_sa_rx_data(rxd);
>  			return ret;
>  		}
>  
> @@ -1106,12 +1110,10 @@ static int sa_run(struct sa_req *req)
>  
>  		ret = sg_split(mapped_sg->sgt.sgl, mapped_sg->sgt.nents, 0, 1,
>  			       &split_size, &src, &src_nents, gfp_flags);
> -		if (ret) {
> -			src_nents = mapped_sg->sgt.nents;
> -			src = mapped_sg->sgt.sgl;
> -		} else {
> -			mapped_sg->split_sg = src;
> -		}
> +		if (ret)
> +			goto err_cleanup;
> +
> +		mapped_sg->split_sg = src;
>  	}
>  
>  	dma_sync_sgtable_for_device(ddev, &mapped_sg->sgt, DMA_TO_DEVICE);
> @@ -1121,6 +1123,10 @@ static int sa_run(struct sa_req *req)
>  		dst = src;
>  	} else {
>  		dst_nents = sg_nents_for_len(req->dst, req->size);
> +		if (dst_nents < 0) {
> +			ret = dst_nents;
> +			goto err_cleanup;
> +		}

[Severity: High]
What happens in sa_run() when req->size is 0?

In sa_aead_run(), an AEAD request with req->assoclen == 0 and zero plaintext
length (req->cryptlen == 0 on encryption, or req->cryptlen ==
crypto_aead_authsize(tfm) on decryption) results in auth_size == 0. Unlike
sa_cipher_run() and sa_sha_run(), sa_aead_run() does not divert zero-length
requests to the software fallback:

drivers/crypto/sa2ul.c:sa_aead_run() {
	...
	if (auth_size > SA_MAX_DATA_SZ ||
	    (auth_size >= SA_UNSAFE_DATA_SZ_MIN &&
	     auth_size <= SA_UNSAFE_DATA_SZ_MAX)) {
		...
	}
	...
	sa_req.size = auth_size;
	...
	return sa_run(&sa_req);
}

When sa_run() calls sg_nents_for_len() with a length of 0, sg_nents_for_len()
returns 0. Because the new checks only test sg_nents < 0 and dst_nents < 0, a
return value of 0 bypasses the error check, skips the sg_nents == 1 branch,
and enters the else branch:

	} else {
		mapped_sg->sgt.sgl = req->src;
		mapped_sg->sgt.orig_nents = sg_nents;
		ret = dma_map_sgtable(ddev, &mapped_sg->sgt, dir_src, 0);

Passing orig_nents == 0 to dma_map_sgtable() triggers the WARN_ON_ONCE(nents
<= 0) in __dma_map_sg_attrs() and fails the request with -EINVAL.

Should sa_run() reject sg_nents <= 0 and dst_nents <= 0 (or should
sa_aead_run() route auth_size == 0 to the software fallback)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-b4-upstream-sa2ul-cleanup-v2-0-15ab90b3f9c2@ti.com?part=22