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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] crypto: acomp - missing async check in crypto_acomp_compress() fast path
- [High] crypto: acomp: uninitialized state->src/dst restore in acomp_reqchain_virt()
--

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

crypto: acomp - Add trivial segmentation wrapper.

This commit adds a single-segment compression wrapper in crypto/acompress.c
that stores the compressed length or error code into req->dst->length when
req->unit_size is set on transforms that do not support segmentation. It also
adds an inline fast path in crypto_acomp_compress() for single-segment
scatterlist requests.

> diff --git a/crypto/acompress.c b/crypto/acompress.c
> index c8b4b8589769e..c55b0e8eff3d8 100644
> --- a/crypto/acompress.c
> +++ b/crypto/acompress.c
> @@ -166,14 +166,20 @@ static void acomp_save_req(struct acomp_req *req, crypto_completion_t cplt)
>  
>  	state->compl = req->base.complete;
>  	state->data = req->base.data;
> +	state->unit_size = req->unit_size;
> +	state->flags = req->base.flags & (CRYPTO_ACOMP_REQ_SRC_VIRT |
> +					  CRYPTO_ACOMP_REQ_DST_VIRT);

[Severity: High]
Can moving the state->flags assignment from acomp_virt_to_sg() into
acomp_save_req() corrupt req->svirt and req->dvirt when a request is
configured with acomp_request_set_src_nondma() or
acomp_request_set_dst_nondma()?

When a caller sets up a non-dma virtual request via
acomp_request_set_src_nondma() or acomp_request_set_dst_nondma(), both the
non-dma and virtual request flags are set in req->base.flags. When
crypto_acomp_compress() or crypto_acomp_decompress() calls
acomp_do_req_chain(), the call chain is:

crypto_acomp_compress()
  acomp_do_req_chain()
    acomp_save_req()
      state->flags = req->base.flags & (CRYPTO_ACOMP_REQ_SRC_VIRT |
                                        CRYPTO_ACOMP_REQ_DST_VIRT);
    acomp_do_one_req()
      if (acomp_request_isnondma(req))
          return acomp_do_nondma(req, comp);

Because acomp_do_one_req() returns early through acomp_do_nondma() without
calling acomp_virt_to_sg(), state->src and state->dst are never initialized
from req->svirt and req->dvirt.

When acomp_reqchain_finish() then calls acomp_reqchain_virt() in
crypto/acompress.c:

static void acomp_reqchain_virt(struct acomp_req *req)
{
	struct acomp_req_chain *state = &req->chain;
	unsigned int slen = req->slen;
	unsigned int dlen = req->dlen;

	if (state->flags & CRYPTO_ACOMP_REQ_SRC_VIRT)
		acomp_request_set_src_dma(req, state->src, slen);
	if (state->flags & CRYPTO_ACOMP_REQ_DST_VIRT)
		acomp_request_set_dst_dma(req, state->dst, dlen);
}

won't acomp_reqchain_virt() see the virtual flags set in state->flags and
overwrite req->svirt and req->dvirt with the uninitialized state->src and
state->dst pointers (while also clearing the non-dma flags), causing a null
or wild pointer dereference if acomp_reqchain_finish() then writes to
req->dst->length when req->unit_size is non-zero?

[ ... ]

> @@ -279,10 +289,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_req_on_stack(req))) {
> +		int err = tfm->compress(req);
> +
> +		req->dst->length = unlikely(err) ? err : req->dlen;
> +		return err;
> +	}

[Severity: Critical]
Should this fast path in crypto_acomp_compress() in crypto/acompress.c check
!acomp_is_async(tfm) instead of !acomp_req_on_stack(req)?

Callers using asynchronous hardware compression transforms (where
acomp_is_async(tfm) is true, such as qat, hisi_zip, or iaa in async mode)
allocate requests on the heap with acomp_request_alloc(), so
!acomp_req_on_stack(req) is true for asynchronous requests.

When zswap_compress() submits a single-segment scatterlist request with
req->unit_size == req->slen to an asynchronous transform that does not
support segmentation, this fast path will be taken instead of
acomp_do_req_chain(req, true):

crypto/acompress.c:crypto_acomp_compress() {
	if (likely((req->unit_size == req->slen) && acomp_request_issg(req) &&
		    !crypto_acomp_req_seg(tfm) && !acomp_req_on_stack(req))) {
		int err = tfm->compress(req);

		req->dst->length = unlikely(err) ? err : req->dlen;
		return err;
	}
}

When tfm->compress(req) submits the descriptor to hardware and returns an
in-progress or busy status, won't this immediately overwrite req->dst->length
with the negative error value while hardware dma is still in flight on
req->dst, and skip registering acomp_reqchain_done() via acomp_save_req() so
that req->dst->length is never updated to req->dlen upon async completion?

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