Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The Poly1305 driver dynamically allocates memory chunks for ahash requests which are permanently leaked when a request is aborted, re-initialized, or imported, leading to an unbounded memory leak and Denial of Service (OOM).
- [High] The driver maps an unaligned memory offset for DMA and shares a cacheline with frequently CPU-mutated fields, violating architecture cacheline alignment rules.
- [High] The driver allocates huge contiguous buffers (up to 1MB) using `kmalloc` without `__GFP_NOWARN` in potentially atomic contexts, which will reliably trigger page allocation failures.
--
--- Patch [10]: [PATCH v3 10/19] crypto: cmh - add ChaCha20-Poly1305 ---
commit 736bbd76828dea383bcdcc54143a0030ecd02a41
Author: Alex Ousherovitch <[email protected]>
crypto: cmh - add ChaCha20-Poly1305
Register ChaCha20-Poly1305 AEAD and ChaCha20 skcipher algorithms
using the CMH CCP core (core ID 0x18). Also registers the Poly1305
ahash for standalone use.
> diff --git a/drivers/crypto/cmh/cmh_ccp.c b/drivers/crypto/cmh/cmh_ccp.c
> index 0000000000000..babf6ffe6ff09
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_ccp.c
[ ... ]
> @@ -175,34 +175,34 @@ static int cmh_ccp_crypt(struct skcipher_request *req, u32 ccp_op)
> struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
> struct cmh_ccp_tfm_ctx *tctx = crypto_skcipher_ctx(tfm);
> struct cmh_ccp_reqctx *rctx = skcipher_request_ctx(req);
> struct vcq_cmd cmds[CMH_CCP_MAX_PAYLOAD];
> u64 key_ref;
> u32 keylen;
> struct core_dispatch d;
> s32 target_mbx;
> u32 core_id;
> u32 idx;
> int ret;
> gfp_t gfp;
>
> if (tctx->key.mode == CMH_KEY_NONE)
> return -ENOKEY;
>
> if (!req->cryptlen)
> return 0;
>
> /* Limit linearisation buffers to avoid large allocations. */
> if (req->cryptlen > SZ_1M)
> return -EINVAL;
>
> gfp = req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP ?
> GFP_KERNEL : GFP_ATOMIC;
>
> memset(rctx, 0, sizeof(*rctx));
> rctx->cryptlen = req->cryptlen;
>
> /* Linearise input from scatterlist */
> rctx->in_buf = kmalloc(req->cryptlen, gfp);
[Severity: High]
Does this code risk triggering reliable page allocation failures?
In cmh_ccp_crypt() (drivers/crypto/cmh/cmh_ccp.c), since cryptlen can be up to
1MB, calling kmalloc() with GFP_ATOMIC for high-order allocations is highly
susceptible to memory fragmentation and will fail frequently.
Since __GFP_NOWARN is not used, these failures will also spam the kernel log
with stack traces, causing a denial of service.
[ ... ]
> diff --git a/drivers/crypto/cmh/cmh_ccp_aead.c b/drivers/crypto/cmh/cmh_ccp_aead.c
> index 0000000000000..df5482179bdc4
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_ccp_aead.c
[ ... ]
> @@ -235,36 +235,36 @@ static int cmh_ccp_aead_crypt(struct aead_request *req, u32 ccp_op)
> struct crypto_aead *tfm = crypto_aead_reqtfm(req);
> struct cmh_ccp_aead_tfm_ctx *tctx = crypto_aead_ctx(tfm);
> struct cmh_ccp_aead_reqctx *rctx = aead_request_ctx(req);
> struct vcq_cmd cmds[CMH_CCP_AEAD_MAX_PAYLOAD];
> u64 key_ref;
> u32 keylen, authsize, cryptlen;
> struct core_dispatch d;
> s32 target_mbx;
> u32 core_id;
> u32 idx;
> int ret;
> gfp_t gfp;
>
> if (tctx->key.mode == CMH_KEY_NONE)
> return -ENOKEY;
>
> authsize = tctx->authsize;
>
> if (ccp_op == CCP_OP_ENCRYPT) {
> cryptlen = req->cryptlen;
> } else {
> if (req->cryptlen < authsize)
> return -EINVAL;
> cryptlen = req->cryptlen - authsize;
> }
>
> /*
> * HW uses a proprietary LLI scatter-gather format that is
> * incompatible with struct scatterlist, so the payload is
> * linearised into contiguous buffers for DMA. Cap total
> * size to prevent excessive memory consumption.
> */
> if ((u64)cryptlen + req->assoclen > SZ_1M)
> return -EINVAL;
>
> gfp = req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP ?
> GFP_KERNEL : GFP_ATOMIC;
>
> memset(rctx, 0, sizeof(*rctx));
> rctx->cryptlen = cryptlen;
> rctx->assoclen = req->assoclen;
> rctx->authsize = authsize;
> rctx->encrypting = (ccp_op == CCP_OP_ENCRYPT);
>
> /*
> * rfc7539esp: the last ivsize (8) bytes of the AAD region are the
> * IV/nonce, not actual associated data. Subtract them so HW only
> * authenticates the real AAD.
> */
> if (crypto_aead_ivsize(tfm) == CCP_ESP_IV_SIZE) {
> if (rctx->assoclen < CCP_ESP_IV_SIZE)
> return -EINVAL;
> rctx->assoclen -= CCP_ESP_IV_SIZE;
> }
>
> /* Linearise AAD */
> if (rctx->assoclen > 0) {
> rctx->aad_buf = kmalloc(rctx->assoclen, gfp);
> if (!rctx->aad_buf)
> return -ENOMEM;
> scatterwalk_map_and_copy(rctx->aad_buf, req->src,
> 0, rctx->assoclen, 0);
> rctx->aad_dma = cmh_dma_map_single(rctx->aad_buf,
> rctx->assoclen,
> DMA_TO_DEVICE);
> if (cmh_dma_map_error(rctx->aad_dma)) {
> ret = -ENOMEM;
> goto out_free_aad;
> }
> }
>
> /* Linearise input */
> if (cryptlen > 0) {
> rctx->in_buf = kmalloc(cryptlen, gfp);
[Severity: High]
Does this face the same allocation failure risk as in cmh_ccp_crypt()?
In cmh_ccp_aead_crypt() (drivers/crypto/cmh/cmh_ccp_aead.c), with allocations
up to 1MB and GFP_ATOMIC context, high-order memory allocations could fail
easily and produce large stack trace spam without __GFP_NOWARN.
[ ... ]
> diff --git a/drivers/crypto/cmh/cmh_ccp_poly.c b/drivers/crypto/cmh/cmh_ccp_poly.c
> index 0000000000000..0f783a98d54de
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_ccp_poly.c
[ ... ]
> @@ -62,14 +62,14 @@
> */
> struct cmh_poly_tfm_ctx {
> u8 key[POLY1305_KEY_SIZE];
> dma_addr_t rkey_dma;
> dma_addr_t skey_dma;
> u32 keylen;
> bool has_key;
> spinlock_t chunk_lock; /* protects all_chunks */
> struct list_head all_chunks; /* orphan-safe chunk tracking */
> };
[Severity: High]
Can this structure layout cause data corruption on non-cache-coherent
architectures?
In struct cmh_poly_tfm_ctx (drivers/crypto/cmh/cmh_ccp_poly.c), the key array
is mapped for DMA. Since it shares a cacheline with chunk_lock and all_chunks,
CPU modifications to these fields could be lost or corrupted during DMA cache
clean/invalidate operations.
[ ... ]
> @@ -144,32 +144,32 @@ static int cmh_poly_setkey(struct crypto_ahash *tfm, const u8 *key,
> unsigned int keylen)
> {
> struct cmh_poly_tfm_ctx *tctx = crypto_ahash_ctx(tfm);
>
> /* Poly1305: exactly 32 bytes (r[16] + s[16]) */
> if (keylen != POLY1305_KEY_SIZE)
> return -EINVAL;
>
> /* Unmap old key DMA if re-keying */
> if (tctx->has_key) {
> cmh_dma_unmap_single(tctx->rkey_dma, CCP_POLY_KEY_SIZE,
> DMA_TO_DEVICE);
> cmh_dma_unmap_single(tctx->skey_dma, CCP_POLY_KEY_SIZE,
> DMA_TO_DEVICE);
> }
>
> memcpy(tctx->key, key, POLY1305_KEY_SIZE);
> tctx->keylen = POLY1305_KEY_SIZE;
>
> /*
> * Pre-map both key halves for DMA. The key buffer lives in
> * the tfm context and is stable until exit_tfm() or re-setkey.
> */
> tctx->skey_dma = cmh_dma_map_single(tctx->key + CCP_POLY_KEY_SIZE,
> CCP_POLY_KEY_SIZE,
> DMA_TO_DEVICE);
[Severity: High]
Does this break architecture alignment rules?
In cmh_poly_setkey() (drivers/crypto/cmh/cmh_ccp_poly.c), mapping tctx->key + 16
(CCP_POLY_KEY_SIZE) means the buffer offset isn't ARCH_DMA_MINALIGN aligned.
[ ... ]
> @@ -245,15 +245,15 @@ static void cmh_poly_fb_forward(struct ahash_request *req,
> struct cmh_poly_reqctx *rctx)
> {
> if (req->base.flags & CRYPTO_AHASH_REQ_VIRT)
> poly1305_update(&rctx->fb_state, req->svirt, req->nbytes);
> else
> cmh_poly_fb_feed_sg(&rctx->fb_state, req->src, req->nbytes);
> }
>
> static int cmh_poly_init(struct ahash_request *req)
> {
> struct cmh_poly_reqctx *rctx = ahash_request_ctx(req);
>
> memset(rctx, 0, sizeof(*rctx));
> INIT_LIST_HEAD(&rctx->chunks);
> return 0;
> }
[Severity: High]
Does this leak any chunks accumulated in a previous update call?
In cmh_poly_init() (drivers/crypto/cmh/cmh_ccp_poly.c), if this function or
cmh_poly_import() is called on an active request, the memset() overwrites the
rctx->chunks list head, silently orphaning any existing chunks and leaking
memory.
[ ... ]
> @@ -262,35 +262,35 @@ static int cmh_poly_update(struct ahash_request *req)
> {
> struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
> struct cmh_poly_tfm_ctx *tctx = crypto_ahash_ctx(tfm);
> struct cmh_poly_reqctx *rctx = ahash_request_ctx(req);
> struct cmh_poly_chunk *chunk;
> gfp_t gfp;
> int ret;
>
> if (!req->nbytes)
> return 0;
>
> /* Already handed off to the library: forward directly (O(1) mem). */
> if (rctx->switched) {
> cmh_poly_fb_forward(req, rctx);
> return 0;
> }
>
> /*
> * Exceeding the HW input cap: switch to the Poly1305 library
> * (replaying the buffered chunks) rather than failing, then
> * forward this update.
> */
> if (req->nbytes > POLY_MAX_DATA - rctx->total_len) {
> ret = cmh_poly_switch_to_fb(req);
> if (ret)
> goto err_free_chunks;
> cmh_poly_fb_forward(req, rctx);
> return 0;
> }
>
> gfp = req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP ?
> GFP_KERNEL : GFP_ATOMIC;
> chunk = kmalloc(sizeof(*chunk) + req->nbytes, gfp);
> if (!chunk) {
> ret = -ENOMEM;
> goto err_free_chunks;
> }
>
> chunk->len = req->nbytes;
> if (req->base.flags & CRYPTO_AHASH_REQ_VIRT)
> memcpy(chunk->data, req->svirt, req->nbytes);
> else
> scatterwalk_map_and_copy(chunk->data, req->src,
> 0, req->nbytes, 0);
> list_add_tail(&chunk->list, &rctx->chunks);
> spin_lock_bh(&tctx->chunk_lock);
> list_add_tail(&chunk->tfm_node, &tctx->all_chunks);
> spin_unlock_bh(&tctx->chunk_lock);
[Severity: High]
Does this leak memory if the request is closed by userspace before final() is
called?
In cmh_poly_update() (drivers/crypto/cmh/cmh_ccp_poly.c), if an unprivileged
local attacker uses AF_ALG to hash data, calls update(), and then closes the
socket, the crypto ahash API has no .freereq callback to clean up.
The chunks allocated here will be orphaned and remain on tctx->all_chunks
indefinitely until the TFM is destroyed, leading to an unbounded memory leak
and system OOM.
--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.