Re: [PATCH v3 3/8] crypto: acomp - Add trivial segmentation wrapper.

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] crypto: acomp - missing non-zero unit_size check in
  crypto_acomp_compress()
- [Medium] crypto: acomp - union type confusion in
  acomp_reqchain_finish() on virt dst
--

commit c2a605231cabd5e0e15d6d778b3943a2c118d756
Author: Kanchana P. Sridhar <kanchanapsridhar2026@gmail.com>

crypto: acomp - Add trivial segmentation wrapper.

This commit adds a single-segment compression wrapper for algorithms that do
not natively support segmentation, returning the compressed length or error
in req->dst->length when req->unit_size is set.

> diff --git a/crypto/acompress.c b/crypto/acompress.c
> index c8b4b8589769e..ac4dc52668b89 100644
> --- a/crypto/acompress.c
> +++ b/crypto/acompress.c
[ ... ]
> @@ -242,6 +246,10 @@ static int acomp_reqchain_finish(struct acomp_req *req, int err)
>  {
>  	acomp_reqchain_virt(req);
>  	acomp_restore_req(req);
> +
> +	if (req->unit_size)
> +		req->dst->length = unlikely(err) ? err : req->dlen;
> +
>  	return err;
>  }
>  
> @@ -266,10 +274,13 @@ static int acomp_do_req_chain(struct acomp_req *req, bool comp)
>  {
>  	int err;
>  
> +	if (unlikely(req->unit_size && req->slen > req->unit_size))
> +		return -ENOSYS;

[Severity: Medium]
Can acomp_reqchain_finish() corrupt the destination buffer when a request
with a non-zero req->unit_size uses virtual addresses?

In struct acomp_req, dst and dvirt share an anonymous union.  When
acomp_do_req_chain() handles a virtual-address request
(CRYPTO_ACOMP_REQ_DST_VIRT), acomp_reqchain_virt() restores the raw u8 *
buffer pointer into req->dvirt before acomp_restore_req() restores
req->unit_size.  Writing to req->dst->length then treats req->dvirt as a
struct scatterlist * and overwrites 4 bytes inside the caller's output
buffer.

In addition, acomp_do_req_chain() is called for decompression (comp == false),
so a decompression request with req->unit_size set will also hit the
compression-only req->slen > req->unit_size check and write to
req->dst->length.

>  	acomp_save_req(req, acomp_reqchain_done);
>  
>  	err = acomp_do_one_req(req, comp);
> -	if (err == -EBUSY || err == -EINPROGRESS)
> +	if (unlikely(err == -EBUSY || err == -EINPROGRESS))
>  		return err;
>  
>  	return acomp_reqchain_finish(req, err);
> @@ -279,10 +290,27 @@ int crypto_acomp_compress(struct acomp_req *req)
>  {
>  	struct crypto_acomp *tfm = crypto_acomp_reqtfm(req);
>  
> +	if (likely((req->unit_size == req->slen) && acomp_request_issg(req) &&
> +		    !crypto_acomp_req_seg(tfm) && !acomp_is_async(tfm))) {
> +		int err = tfm->compress(req);
> +
> +		req->dst->length = unlikely(err) ? err : req->dlen;
> +		return err;
> +	}

[Severity: Medium]
Should this fast-path check also verify that req->unit_size is non-zero?

Callers that do not use segmentation leave req->unit_size at 0.  If such a
caller submits a scatterlist request with req->slen == 0 on a synchronous
non-segmented transform, req->unit_size == req->slen is true, so
crypto_acomp_compress() enters the segmentation path and writes the negative
error from tfm->compress(req) into req->dst->length, corrupting the caller's
scatterlist or causing a NULL pointer dereference if req->dst is NULL (which
can also happen when req->unit_size is non-zero and req->dst is NULL here or
in acomp_reqchain_finish()).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106072412.7909-1-kanchanapsridhar2026@gmail.com?part=3