crypto: engine - add acomp support and adapt Hisilicon HPRE and ZIP drivers
Subsystems (4)
Chenghai Huang
maintainerPATCH 1/4 crypto: engine - clear busy and recheck running in the retry pump loop
In reply to Chenghai HuangThe retry pump loop jumps back to the dequeue path directly, skipping the idle check that clears engine->busy and rechecks engine->running.
When the loop drains the queue it exits with busy still set, and crypto_transfer_request() queues a pump only when the engine is not
busy: if the loop ends with the inline error completion in req_err_1, which queues no pump, the next request is enqueued with nothing to run it and the engine queue stalls. crypto_engine_stop() also spins waiting for the stale busy flag, up to failing with -EBUSY, and the loop keeps submitting to hardware after stop was requested because it never rechecks running.
Jump to the idle check instead: the loop now clears busy and exits when the queue is empty, and stops submitting once the engine is no longer running. Engines without retry support are unaffected.
--- crypto/crypto_engine.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
crypto/crypto_engine.c +2 -2
@@ -82,6 +82,7 @@ static void crypto_pump_requests(struct crypto_engine *engine,if (!engine->retry_support && engine->cur_req) goto out; +check_idle:/* Check if the engine queue is idle */ if (!crypto_queue_len (&engine->queue) || !engine->running) {if (!engine->busy)
@@ -98,7 +99,6 @@ static void crypto_pump_requests(struct crypto_engine *engine, goto out; } -start_request:/* Get the fist request from the engine queue to handle */ backlog =crypto_get_backlog (&engine->queue); async_req =crypto_dequeue_request (&engine->queue);
@@ -161,7 +161,7 @@ static void crypto_pump_requests(struct crypto_engine *engine,/* If retry mechanism is supported, send new requests to engine */ if (engine->retry_support) {spin_lock_irqsave (&engine->queue_lock, flags); - goto start_request; + goto check_idle; }return ;
Signature · 1 line
2.43.0
Chenghai Huang
maintainercover crypto: engine - add acomp support and adapt Hisilicon HPRE and ZIP drivers
This series adds asynchronous compression (acomp) support to the crypto engine framework, then converts the Hisilicon HPRE and ZIP drivers to use it instead of their hand-rolled backlog / queue-retry mechanisms.
Patch 1 fixes the retry pump loop in the engine core: it skips the idle check that clears engine->busy and rechecks engine->running, so after the queue drains a later request can be enqueued without a pump and stall the engine, and crypto_engine_stop() can spin on the stale busy flag up to -EBUSY.
Patch 2 adds struct acomp_engine_alg with transfer/finalize helpers and a register/unregister interface, mirroring the existing akcipher and kpp support. struct acomp_alg moves to <crypto/acompress.h> so that engine.h can embed it.
Patch 3 gives each HPRE QP (DH/RSA/ECDH) its own crypto engine. MAY_BACKLOG requests are routed through the engine, which provides the backlog queue and retries on -ENOSPC; other requests are sent directly with bounded retries, and in-flight requests are cleaned up by the completion callback.
Patch 4 gives ZIP a per-device engine pool, with each QP context borrowing an engine by its QP id so compression and decompression queue independently. Requests are sent to the hardware when it has room; on a full queue they are parked in the engine queue and retried, with the DMA mappings kept across retries.
Chenghai Huang (3):
crypto: engine - clear busy and recheck running in the retry pump loop
crypto: engine - add acomp support
crypto: hisilicon/zip - adapt to use crypto engine
Weili Qian (1):
crypto: hisilicon/hpre - use crypto engine framework
crypto/crypto_engine.c | 46 ++- drivers/crypto/hisilicon/Kconfig | 2 + drivers/crypto/hisilicon/hpre/hpre.h | 4 + drivers/crypto/hisilicon/hpre/hpre_crypto.c | 414 +++++++++++++++----- drivers/crypto/hisilicon/zip/zip.h | 5 + drivers/crypto/hisilicon/zip/zip_crypto.c | 358 +++++++++++------ include/crypto/acompress.h | 33 ++ include/crypto/engine.h | 13 + include/crypto/internal/acompress.h | 33 -- 9 files changed, 649 insertions(+), 259 deletions(-)
Signature · 1 line
2.43.0
Chenghai Huang
maintainerPATCH 3/4 crypto: hisilicon/hpre - use crypto engine framework
From: Weili Qian <[email protected]>
The HPRE driver has no software backlog queue for MAY_BACKLOG requests. When the hardware queue is full, such requests fail instead of being queued for later retry.
Migrate DH/RSA/ECDH to the crypto engine framework, which provides a backlog queue and built-in retry on -ENOSPC. Convert akcipher_alg/kpp_alg to akcipher_engine_alg/kpp_engine_alg and use the corresponding engine register/unregister APIs.
Only MAY_BACKLOG requests are routed through the engine; the QM hardware queue is deep enough to accept other requests directly, so they skip the engine overhead and fail with -ENOSPC when full, per the API contract. Non-backlog requests use a direct-send path with bounded retries; the error code for a full queue changes from -EBUSY to -ENOSPC in this path.
HPRE_SEND_CNT now counts successful sends instead of attempts.
--- drivers/crypto/hisilicon/Kconfig | 1 + drivers/crypto/hisilicon/hpre/hpre.h | 4 + drivers/crypto/hisilicon/hpre/hpre_crypto.c | 414 +++++++++++++++----- 3 files changed, 314 insertions(+), 105 deletions(-)
drivers/crypto/hisilicon/Kconfig +1 -0
@@ -58,6 +58,7 @@ config CRYPTO_DEV_HISI_HPRE select CRYPTO_DH select CRYPTO_RSA select CRYPTO_ECDH + select CRYPTO_ENGINE help Support for HiSilicon HPRE(High Performance RSA Engine) accelerator, which can accelerate RSA and DH algorithms.
drivers/crypto/hisilicon/hpre/hpre.h +4 -0
@@ -5,10 +5,12 @@#include <linux/list.h> #include <linux/hisi_acc_qm.h> +#include <crypto/engine.h> #define HPRE_SQE_SIZE sizeof(struct hpre_sqe)#define HPRE_PF_DEF_Q_NUM 64#define HPRE_PF_DEF_Q_BASE 0 +#define HPRE_MAX_CRYPTO_ENGINE 64/* * type used in qm sqc DW6.
@@ -70,6 +72,8 @@ struct hpre_debug {struct hpre {struct hisi_qm qm;struct hpre_debug debug; +struct crypto_engine *engine[HPRE_MAX_CRYPTO_ENGINE ]; +u32 engine_num; };enum hpre_alg_type {
drivers/crypto/hisilicon/hpre/hpre_crypto.c +309 -105
578 lines not on this page · raw message
Signature · 1 line
2.43.0
Chenghai Huang
maintainerPATCH 4/4 crypto: hisilicon/zip - adapt to use crypto engine
In reply to Chenghai HuangThe ZIP driver has no backlog queue: when the hardware queue is full, requests fail immediately.
Migrate the ZIP driver to the crypto engine framework. Each QP context borrows an engine from a per-device pool by its QP id, so compression and decompression queue independently. Requests are sent to the hardware when it has room; on -ENOSPC they are parked in the engine queue and retried with their DMA mappings kept, so transient queue pressure never fails a request. The per-request state moves into the request context so the engine can retry queued requests, and the soft fallback for QP allocation failures is kept.
Also make CRYPTO_DEV_HISI_ZIP select CRYPTO_ENGINE.
--- drivers/crypto/hisilicon/Kconfig | 1 + drivers/crypto/hisilicon/zip/zip.h | 5 + drivers/crypto/hisilicon/zip/zip_crypto.c | 358 +++++++++++++++------- 3 files changed, 245 insertions(+), 119 deletions(-)
drivers/crypto/hisilicon/Kconfig +1 -0
@@ -45,6 +45,7 @@ config CRYPTO_DEV_HISI_ZIP select CRYPTO_DEV_HISI_QM select CRYPTO_DEFLATE select CRYPTO_LZ4 + select CRYPTO_ENGINE help Support for HiSilicon ZIP Driver
drivers/crypto/hisilicon/zip/zip.h +5 -0
@@ -8,6 +8,9 @@#include <linux/list.h> #include <linux/hisi_acc_qm.h> +#include <crypto/engine.h> + +#define HZIP_MAX_CRYPTO_ENGINE 64enum hisi_zip_error_type {/* negative compression */
@@ -27,6 +30,8 @@ struct hisi_zip {struct hisi_qm qm;struct hisi_zip_ctrl *ctrl;struct hisi_zip_dfx dfx; +struct crypto_engine *engine[HZIP_MAX_CRYPTO_ENGINE ]; +u32 engine_num; };struct hisi_zip_sqe {
drivers/crypto/hisilicon/zip/zip_crypto.c +239 -119
580 lines not on this page · raw message
Signature · 1 line
2.43.0
Chenghai Huang
maintainerPATCH 2/4 crypto: engine - add acomp support
In reply to Chenghai HuangAdd support for asynchronous compression (acomp) algorithms to the crypto engine framework: transfer/finalize helpers plus an acomp_engine_alg registration interface, mirroring the existing akcipher and kpp support.
As engine.h needs to embed struct acomp_alg by value, move it from include/crypto/internal/acompress.h to include/crypto/acompress.h. The internal header still provides it through the existing include.
--- crypto/crypto_engine.c | 42 +++++++++++++++++++++++++++++ include/crypto/acompress.h | 33 +++++++++++++++++++++++ include/crypto/engine.h | 13 +++++++++ include/crypto/internal/acompress.h | 33 ----------------------- 4 files changed, 88 insertions(+), 33 deletions(-)
crypto/crypto_engine.c +42 -0
@@ -7,6 +7,7 @@* Author: Baolin Wang <[email protected]> */ +#include <crypto/internal/acompress.h> #include <crypto/internal/aead.h> #include <crypto/internal/akcipher.h> #include <crypto/internal/engine.h>
@@ -220,6 +221,19 @@ static int crypto_transfer_request_to_engine(struct crypto_engine *engine,return crypto_transfer_request (engine, req, true); } +/** +* crypto_transfer_acomp_request_to_engine - transfer one acomp_request +* to list into the engine queue +* @engine: the hardware engine +* @req: the request need to be listed into the engine queue +*/ +int crypto_transfer_acomp_request_to_engine (struct crypto_engine *engine, +struct acomp_req *req) +{ +return crypto_transfer_request_to_engine (engine, &req->base); +} +EXPORT_SYMBOL_GPL (crypto_transfer_acomp_request_to_engine); +/** * crypto_transfer_aead_request_to_engine - transfer one aead_request * to list into the engine queue
@@ -285,6 +299,20 @@ int crypto_transfer_skcipher_request_to_engine(struct crypto_engine *engine, }EXPORT_SYMBOL_GPL (crypto_transfer_skcipher_request_to_engine); +/** +* crypto_finalize_acomp_request - finalize one acomp_request if +* the request is done +* @engine: the hardware engine +* @req: the request need to be finalized +* @err: error number +*/ +void crypto_finalize_acomp_request (struct crypto_engine *engine, +struct acomp_req *req,int err) +{ +return crypto_finalize_request (engine, &req->base, err); +} +EXPORT_SYMBOL_GPL (crypto_finalize_acomp_request); +/** * crypto_finalize_aead_request - finalize one aead_request if * the request is done
@@ -504,6 +532,20 @@ void crypto_engine_exit(struct crypto_engine *engine) }EXPORT_SYMBOL_GPL (crypto_engine_exit); +int crypto_engine_register_acomp (struct acomp_engine_alg *alg) +{ +if (!alg->op.do_one_request) +return -EINVAL ; +return crypto_register_acomp (&alg->base); +} +EXPORT_SYMBOL_GPL (crypto_engine_register_acomp); + +void crypto_engine_unregister_acomp (struct acomp_engine_alg *alg) +{ +crypto_unregister_acomp (&alg->base); +} +EXPORT_SYMBOL_GPL (crypto_engine_unregister_acomp); +int crypto_engine_register_aead (struct aead_engine_alg *alg) {if (!alg->op.do_one_request)
include/crypto/acompress.h +33 -0
@@ -122,6 +122,39 @@ struct crypto_acomp { }struct comp_alg_common COMP_ALG_COMMON ; +/** +* struct acomp_alg - asynchronous compression algorithm +* +* @compress: Function performs a compress operation +* @decompress: Function performs a de-compress operation +* @init: Initialize the cryptographic transformation object. +* This function is used to initialize the cryptographic +* transformation object. This function is called only once at +* the instantiation time, right after the transformation context +* was allocated. In case the cryptographic hardware has some +* special requirements which need to be handled by software, this +* function shall check for the precise requirement of the +* transformation and put any software fallbacks in place. +* @exit: Deinitialize the cryptographic transformation object. This is a +* counterpart to @init, used to remove various changes set in +* @init. +* +* @base: Common crypto API algorithm data structure +* @calg: Common algorithm data structure shared with scomp +* @COMP_ALG_COMMON: see struct comp_alg_common +*/ +struct acomp_alg { +int (*compress)(struct acomp_req *req); +int (*decompress)(struct acomp_req *req); +int (*init)(struct crypto_acomp *tfm); +void (*exit)(struct crypto_acomp *tfm); + +union { +struct COMP_ALG_COMMON ; +struct comp_alg_common calg; + }; +}; +/** * DOC: Asynchronous Compression API *
include/crypto/engine.h +13 -0
@@ -7,6 +7,7 @@#ifndef _CRYPTO_ENGINE_H#define _CRYPTO_ENGINE_H +#include <crypto/acompress.h> #include <crypto/aead.h> #include <crypto/akcipher.h> #include <crypto/hash.h>
@@ -26,6 +27,11 @@ struct crypto_engine_op { void *areq); }; +struct acomp_engine_alg { +struct acomp_alg base; +struct crypto_engine_op op; +}; +struct aead_engine_alg {struct aead_alg base;struct crypto_engine_op op;
@@ -51,6 +57,8 @@ struct skcipher_engine_alg {struct crypto_engine_op op; }; +int crypto_transfer_acomp_request_to_engine (struct crypto_engine *engine, +struct acomp_req *req);int crypto_transfer_aead_request_to_engine (struct crypto_engine *engine,struct aead_request *req); int crypto_transfer_akcipher_request_to_engine(struct crypto_engine *engine,
@@ -61,6 +69,8 @@ int crypto_transfer_kpp_request_to_engine(struct crypto_engine *engine, struct kpp_request *req);int crypto_transfer_skcipher_request_to_engine (struct crypto_engine *engine,struct skcipher_request *req); +void crypto_finalize_acomp_request (struct crypto_engine *engine, +struct acomp_req *req,int err);void crypto_finalize_aead_request (struct crypto_engine *engine,struct aead_request *req,int err); void crypto_finalize_akcipher_request(struct crypto_engine *engine,
@@ -79,6 +89,9 @@ struct crypto_engine *crypto_engine_alloc_init_and_set(struct device *dev, bool rt, int qlen);void crypto_engine_exit (struct crypto_engine *engine); +int crypto_engine_register_acomp (struct acomp_engine_alg *alg); +void crypto_engine_unregister_acomp (struct acomp_engine_alg *alg); +int crypto_engine_register_aead (struct aead_engine_alg *alg);void crypto_engine_unregister_aead (struct aead_engine_alg *alg);int crypto_engine_register_aeads (struct aead_engine_alg *algs,int count);
include/crypto/internal/acompress.h +0 -33
@@ -23,39 +23,6 @@struct acomp_req *name =acomp_fbreq_on_stack_init ( \ __##name##_req, (req)) -/** -* struct acomp_alg - asynchronous compression algorithm -* -* @compress: Function performs a compress operation -* @decompress: Function performs a de-compress operation -* @init: Initialize the cryptographic transformation object. -* This function is used to initialize the cryptographic -* transformation object. This function is called only once at -* the instantiation time, right after the transformation context -* was allocated. In case the cryptographic hardware has some -* special requirements which need to be handled by software, this -* function shall check for the precise requirement of the -* transformation and put any software fallbacks in place. -* @exit: Deinitialize the cryptographic transformation object. This is a -* counterpart to @init, used to remove various changes set in -* @init. -* -* @base: Common crypto API algorithm data structure -* @calg: Cmonn algorithm data structure shared with scomp -* @COMP_ALG_COMMON: see struct comp_alg_common - */ -struct acomp_alg { -int (*compress)(struct acomp_req *req); -int (*decompress)(struct acomp_req *req); -int (*init)(struct crypto_acomp *tfm); -void (*exit)(struct crypto_acomp *tfm); - -union { -struct COMP_ALG_COMMON ; -struct comp_alg_common calg; - }; -}; -struct crypto_acomp_stream {spinlock_t lock;void *ctx;
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH 2/4] crypto: engine - add acomp support
Thank you for your contribution! Sashiko AI review found 1 potential issue to consider: - [High] crypto: engine - missing CRYPTO_ACOMP select in CRYPTO_ENGINE Kconfig --
Thank you for your contribution! Sashiko AI review found 1 potential issue to consider:
- [High] crypto: engine - missing CRYPTO_ACOMP select in CRYPTO_ENGINE Kconfig
--
commit 21d47c711df84337042460c8075ae29a58a54c22
Author: Chenghai Huang <[email protected]>
crypto: engine - add acomp support
This commit adds asynchronous compression (acomp) request transfer, finalization, and algorithm registration helpers to the crypto engine framework, and moves struct acomp_alg to include/crypto/acompress.h.
Quoted 4 lines
crypto/crypto_engine.c +0
-0
[ ... ]
Quoted 11 lines
crypto/crypto_engine.c +7
-0@@ -504,6 +532,20 @@ void crypto_engine_exit(struct crypto_engine *engine) }EXPORT_SYMBOL_GPL (crypto_engine_exit); +int crypto_engine_register_acomp (struct acomp_engine_alg *alg) +{ +if (!alg->op.do_one_request) +return -EINVAL ; +return crypto_register_acomp (&alg->base); +} +EXPORT_SYMBOL_GPL (crypto_engine_register_acomp);
crypto/crypto_engine.c void crypto_engine_exit(struct crypto_engine *engine) · line 551
Quoted 10 lines
crypto/crypto_engine.c +7
-0+ +void crypto_engine_unregister_acomp (struct acomp_engine_alg *alg) +{ +crypto_unregister_acomp (&alg->base); +} +EXPORT_SYMBOL_GPL (crypto_engine_unregister_acomp); +int crypto_engine_register_aead (struct aead_engine_alg *alg) {if (!alg->op.do_one_request)
[Severity: High]
Does CRYPTO_ENGINE in crypto/Kconfig also need to select CRYPTO_ACOMP?
crypto_register_acomp() and crypto_unregister_acomp() are only built when CONFIG_CRYPTO_ACOMP2 is enabled, and have no inline stub fallbacks. Unlike CRYPTO_AEAD, CRYPTO_AKCIPHER, CRYPTO_HASH, CRYPTO_KPP, and CRYPTO_SKCIPHER, CRYPTO_ENGINE does not select CRYPTO_ACOMP, so enabling CONFIG_CRYPTO_ENGINE without CONFIG_CRYPTO_ACOMP2 (or with CONFIG_CRYPTO_ENGINE=y and CONFIG_CRYPTO_ACOMP2=m) fails to link with undefined references to crypto_register_acomp() and crypto_unregister_acomp().
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
[email protected]
AI reviewRe: [PATCH 3/4] crypto: hisilicon/hpre - use crypto engine framework
In reply to Chenghai HuangThank you for your contribution! Sashiko AI review found 1 potential issue to consider: - [Medium] crypto: hisilicon/hpre: race in hpre_ctx_init() with engine init and exit It also noticed 2 related…
Thank you for your contribution! Sashiko AI review found 1 potential issue to consider:
- [Medium] crypto: hisilicon/hpre: race in hpre_ctx_init() with engine init and exit
It also noticed 2 related pre-existing issues this patch does not introduce:
- [Critical] crypto: hisilicon/hpre - null pointer dereference in hpre_ecdh_cb() [1]
- [Critical] crypto: hisilicon/hpre: destination scatterlist overwrite in hpre_hw_data_clr_all() [2]
--
commit fd11798f0428979de89ed11bd547ffce95a2a817
Author: Weili Qian <[email protected]>
crypto: hisilicon/hpre - use crypto engine framework
This commit migrates the HiSilicon HPRE DH, RSA, and ECDH algorithms to the crypto engine framework so that CRYPTO_TFM_REQ_MAY_BACKLOG requests are queued and retried when the hardware queue is full.
Quoted 4 lines
drivers/crypto/hisilicon/hpre/hpre_crypto.c +0
-0
[ ... ]
Quoted 4 lines
drivers/crypto/hisilicon/hpre/hpre_crypto.c +0
-0@@ -386,10 +398,20 @@ static int hpre_ctx_init(struct hpre_ctx *ctx, u8 type)return -ENODEV ; }
drivers/crypto/hisilicon/hpre/hpre_crypto.c static int hpre_ctx_init(struct hpre_ctx *ctx, u8 type) · line 410
Quoted 10 lines
drivers/crypto/hisilicon/hpre/hpre_crypto.c +10
-0+ hpre = container_of(qp->qm, struct hpre, qm); +if (!hpre->engine_num) { +hisi_qm_free_qps (&qp,1 ); +return -ENODEV ; + } + ctx->engine = hpre->engine[qp->qp_id % hpre->engine_num]; +if (!ctx->engine) { +hisi_qm_free_qps (&qp,1 ); +return -ENODEV ; + }
[Severity: Medium]
Can hpre_ctx_init() race with hpre_alloc_crypto_engine() and hpre_free_crypto_engine() when multiple HPRE devices are present?
In hpre_probe(), hisi_qm_add_list() adds the QM to hpre_devices before hisi_qm_alg_register() calls hpre_algs_register(), and error rollback or hpre_remove() calls hpre_algs_unregister() before hisi_qm_del_list(). When a first device has already registered the algorithms, a transform allocated on another CPU can grab a QP from a second device while its engines are being set up or torn down:
CPU0 CPU1hpre_remove()
hisi_qm_alg_unregister()
hpre_algs_unregister()
hpre_free_crypto_engine()
crypto_engine_exit(engine[0])
hpre_ctx_init()
hpre_create_qp()
ctx->engine = hpre->engine[0];
hpre->engine_num = 0;
hpre_rsa_enc()
crypto_transfer_..._to_engine()
<-- use-after-free on kworkerSimilarly, during hpre_probe() of a second device, hpre_create_qp() can select the new QM before hpre_alloc_crypto_engine() sets hpre->engine_num, causing hpre_ctx_init() to fail with -ENODEV and forcing the transform to software fallback even though other HPRE devices are ready.
[ ... ]
Quoted 5 lines
drivers/crypto/hisilicon/hpre/hpre_crypto.c +0
-1@@ -1433,8 +1475,12 @@ static void hpre_ecdh_cb(struct hpre_ctx *ctx, void *resp) p =sg_virt (areq->dst);memmove (p, p + ctx->key_sz - curve_sz, curve_sz);memmove (p + curve_sz, p + areq->dst_len - curve_sz, curve_sz); -
drivers/crypto/hisilicon/hpre/hpre_crypto.c static void hpre_ecdh_cb(struct hpre_ctx *ctx, void *resp) · line 1486
Quoted 10 lines
drivers/crypto/hisilicon/hpre/hpre_crypto.c +6
-1-kpp_request_complete (areq, ret); +local_bh_disable (); +if (areq->base.flags &CRYPTO_TFM_REQ_MAY_BACKLOG ) +crypto_finalize_kpp_request (ctx->engine, areq, ret); +else +kpp_request_complete (areq, ret); +local_bh_enable ();atomic64_inc (&dfx[HPRE_RECV_CNT ].value); }
[Severity: Critical]
This problem wasn't introduced by this patch, but can sg_virt(areq->dst) cause a NULL pointer dereference [1] when a request scatterlist is backed by highmem pages?
Because the algorithms are registered without CRYPTO_ALG_KERN_DRIVER_ONLY, requests submitted via AF_ALG can carry highmem pages where sg_virt() returns NULL. Passing that result to dma_map_single() in hpre_get_data_dma_addr() performs an invalid DMA mapping, and passing p to memmove() here in hpre_ecdh_cb() dereferences NULL.
[ ... ]
Quoted 1 line
drivers/crypto/hisilicon/hpre/hpre_crypto.c +0
-0@@ -1679,86 +1732,176 @@ static void hpre_ecdh_exit_tfm(struct crypto_kpp *tfm)
[ ... ]
Quoted 4 lines
drivers/crypto/hisilicon/hpre/hpre_crypto.c +4
-0+static int hpre_rsa_do_one_request (struct crypto_engine *engine,void *areq) +{ +struct akcipher_request *akreq = + container_of(areq, struct akcipher_request, base);
drivers/crypto/hisilicon/hpre/hpre_crypto.c static void hpre_ecdh_exit_tfm(struct crypto_kpp *tfm) · line 1798
Quoted 10 lines
drivers/crypto/hisilicon/hpre/hpre_crypto.c +10
-0+void *tmp =akcipher_request_ctx (akreq); +struct hpre_asym_request *hpre_req =PTR_ALIGN (tmp,hpre_align_sz ()); +int ret; + + ret =hpre_alg_send_message (hpre_req); +if (ret && ret != -ENOSPC ) +hpre_hw_data_clr_all (hpre_req->ctx, hpre_req, akreq->dst, akreq->src); + +return ret; +}
[Severity: Critical]
This problem wasn't introduced by this patch, but does calling hpre_hw_data_clr_all() with a non-NULL dst scatterlist on send failure overwrite the caller's destination buffer [2] with zeroes?
drivers/crypto/hisilicon/hpre/hpre_crypto.c:hpre_hw_data_clr_all() {
...
if (req->dst) {
if (dst)
scatterwalk_map_and_copy(req->dst, dst, 0,
ctx->key_sz, 1);
dma_free_coherent(dev, ctx->key_sz, req->dst, tmp);
}
}When hpre_hw_data_init() allocates a zeroed coherent bounce buffer in req->dst and sending the request later fails in hpre_dh_compute_value(), hpre_rsa_enc(), hpre_rsa_dec(), hpre_dh_do_one_request(), or hpre_rsa_do_one_request(), the hardware never runs, so passing dst to hpre_hw_data_clr_all() copies those unpopulated zeroes into the caller's scatterlist. Passing NULL for dst on error cleanup paths would free the bounce buffer without modifying the caller's destination memory.
[1] https://sashiko.dev/#/bug/linux-2e7b66d8-14a8-417d-adb9-4989357e46d7
[2] https://sashiko.dev/#/bug/linux-547b9bb7-6119-42c4-b3f0-fda7348ea883
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
[email protected]
AI reviewRe: [PATCH 4/4] crypto: hisilicon/zip - adapt to use crypto engine
In reply to Chenghai HuangThank you for your contribution! Sashiko AI review found 2 potential issues to consider: - [High] crypto: hisilicon/zip - req_id exhaustion deadlock in hisi_zip_do_comp() - [Medium] crypto…
Thank you for your contribution! Sashiko AI review found 2 potential issues to consider:
- [High] crypto: hisilicon/zip - req_id exhaustion deadlock in hisi_zip_do_comp()
- [Medium] crypto: hisilicon/zip: hisi_zip_do_acomp() clobbers -EBUSY on backlog
--
commit e6135edb946689a8be0edb23655def633c6b3f9a
Author: Chenghai Huang <[email protected]>
crypto: hisilicon/zip - adapt to use crypto engine
This commit migrates the HiSilicon ZIP acomp driver to the crypto_engine framework so requests that hit a full hardware queue are queued and retried instead of failing immediately.
Quoted 4 lines
drivers/crypto/hisilicon/zip/zip_crypto.c +0
-0
[ ... ]
Quoted 1 line
drivers/crypto/hisilicon/zip/zip_crypto.c +0
-0@@ -333,68 +345,111 @@ static void hisi_zip_acomp_cb(struct hisi_qp *qp, void *data)
[ ... ]
Quoted 12 lines
drivers/crypto/hisilicon/zip/zip_crypto.c +12
-0+static int hisi_zip_do_comp (struct hisi_zip_req *req) +{ +struct hisi_qp *qp = req->qp_ctx->qp; +struct device *dev = &qp->qm->pdev->dev; +int ret; + +if (!req->buf_ready) { + ret =hisi_zip_create_req (req); +if (unlikely (ret)) +return -ENOSPC ; + + ret =hisi_zip_map_req_buffers (req);
drivers/crypto/hisilicon/zip/zip_crypto.c static void hisi_zip_acomp_cb(struct hisi_qp *qp, void *data)
Quoted 10 lines
drivers/crypto/hisilicon/zip/zip_crypto.c +10
-0+if (unlikely (ret)) + goto remove_req; + } + + ret =hisi_zip_do_work (req); +if (ret == -EINPROGRESS ) +return -EINPROGRESS ; + +if (ret == -EBUSY ) +return -ENOSPC ;
[Severity: High]
Can hisi_zip_do_comp() deadlock qp_ctx->engine when a request with req->buf_ready == false sits at the head of engine->queue while all req_q->size bits in req_q->req_bitmap are held by req->buf_ready == true requests queued behind it?
req_q->req_bitmap has sq_depth bits, while qm_get_avail_sqe() caps the hardware SQ at sq_depth - 1 in-flight requests. When hisi_qp_send() returns -EBUSY, hisi_zip_do_comp() returns -ENOSPC with req->buf_ready == true and keeps req->req_id allocated while the request is parked in engine->queue. Because hisi_zip_do_acomp() runs hisi_zip_do_comp() before crypto_transfer_acomp_request_to_engine(), and qm_poll_req_cb() runs hisi_zip_acomp_cb() (freeing req_id) before atomic_dec(&qp->qp_status.used), new submissions can claim freed req_bitmap bits and queue behind an unmapped request:
CPU0 CPU1 (QM completion / new submitters)hisi_zip_do_acomp(req_b)
/* sq_depth - 1 in flight,
* 1 parked with req_id held:
* req_bitmap is full
*/
hisi_zip_do_comp(req_b)
hisi_zip_create_req(req_b)
-> -EBUSY (-ENOSPC,
buf_ready == false) crypto_transfer_acomp_request_to_engine(req_b)
/* req_b at head of engine->queue */
qm_poll_req_cb()
hisi_zip_acomp_cb()
hisi_zip_remove_req()
/* before atomic_dec(&qp_status.used),
* new hisi_zip_do_acomp() calls
* allocate the freed req_ids, get
* -EBUSY from hisi_qp_send(), and
* queue behind req_b with
* buf_ready == true
*/
kworker:crypto_pump_requests()
hisi_zip_do_one_request(req_b)
hisi_zip_create_req(req_b)
-> -ENOSPC crypto_enqueue_request_head(req_b) kthread_queue_work(pump_requests) <-- spins forever
Once all sq_depth bits in req_bitmap are held by requests queued behind req_b and the hardware SQ drains, req_b can never allocate a req_id and the requests behind it can never be dequeued to release theirs, wedging the queue and spinning engine->kworker at 100% CPU.
[ ... ]
Quoted 22 lines
drivers/crypto/hisilicon/zip/zip_crypto.c +22
-0+static int hisi_zip_do_acomp (struct acomp_req *acomp_req,u8 alg_type) +{ +struct hisi_zip_ctx *ctx =crypto_tfm_ctx (acomp_req->base.tfm); +struct hisi_zip_qp_ctx *qp_ctx = &ctx->qp_ctx[alg_type]; +struct hisi_zip_req *req =acomp_request_ctx (acomp_req); +int ret; + +if (unlikely (!acomp_req->src || !acomp_req->slen || + !acomp_req->dst || !acomp_req->dlen)) +return -EINVAL ; + + req->req = acomp_req; + req->alg_type = alg_type; + req->qp_ctx = qp_ctx; + req->buf_ready = false; + +if (ctx->fallback) +return hisi_zip_fallback_do_work (acomp_req, + req->alg_type ==HZIP_ALG_TYPE_DECOMP ); + + ret =hisi_zip_do_comp (req); +if (ret != -ENOSPC )
drivers/crypto/hisilicon/zip/zip_crypto.c
Quoted 10 lines
drivers/crypto/hisilicon/zip/zip_crypto.c +10
-0+return ret; + + ret =crypto_transfer_acomp_request_to_engine (qp_ctx->engine, acomp_req); +if (ret != -EINPROGRESS && ret != -EBUSY ) { +hisi_zip_release_req_resource (req); +return ret; + } + +return -EINPROGRESS ; +}
[Severity: Medium]
Should hisi_zip_do_acomp() return ret instead of -EINPROGRESS at the end of the function?
When engine->queue is full and CRYPTO_TFM_REQ_MAY_BACKLOG is set, crypto_transfer_acomp_request_to_engine() records the request on engine->queue.backlog and returns -EBUSY. Returning -EINPROGRESS hides the backlog signal from callers and still triggers the extra crypto_request_complete(backlog, -EINPROGRESS) notification from crypto_pump_requests() that callers only expect after receiving -EBUSY.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
kernel test robot
build botRe: [PATCH 2/4] crypto: engine - add acomp support
In reply to Chenghai Huangkernel test robot noticed the following build errors:
Hi Chenghai,
kernel test robot noticed the following build errors:
[auto build test ERROR on herbert-cryptodev-2.6/master] [also build test ERROR on linus/master v7.4-rc2 next-20261109] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Chenghai-Huang/crypto-engine-clear-busy-and-recheck-running-in-the-retry-pump-loop/20261109-153553 base: https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master patch link: https://lore.kernel.org/r/20261109073556.2517351-3-huangchenghai2%40huawei.com patch subject: [PATCH 2/4] crypto: engine - add acomp support config: riscv-defconfig (https://download.01.org/0day-ci/archive/20261110/[email protected]/config) compiler: clang version 24.0.0git (https://github.com/llvm/llvm-project fc9576e3b179738d99c5babe5cd9d6c1f883e0db) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20261110/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: crypto/crypto_engine.ko: symbol 'crypto_register_acomp' undefined! >> ERROR: modpost: crypto/crypto_engine.ko: symbol 'crypto_unregister_acomp' undefined!
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
kernel test robot
build botRe: [PATCH 2/4] crypto: engine - add acomp support
In reply to Chenghai Huangkernel test robot noticed the following build errors:
Hi Chenghai,
kernel test robot noticed the following build errors:
[auto build test ERROR on herbert-cryptodev-2.6/master]
[also build test ERROR on herbert-crypto-2.6/master linus/master v7.4-rc2 next-20261109] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Chenghai-Huang/crypto-engine-clear-busy-and-recheck-running-in-the-retry-pump-loop/20261109-153553 base: https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master patch link: https://lore.kernel.org/r/20261109073556.2517351-3-huangchenghai2%40huawei.com patch subject: [PATCH 2/4] crypto: engine - add acomp support config: arm-gemini_defconfig (https://download.01.org/0day-ci/archive/20261110/[email protected]/config) compiler: clang version 24.0.0git (https://github.com/llvm/llvm-project b3e2466a7ef2bbc97d31230c3bf39a6fa50a608a) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20261110/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
>> ld.lld: error: undefined symbol: crypto_register_acomp >>> referenced by crypto_engine.c >>> crypto/crypto_engine.o:(crypto_engine_register_acomp) in archive vmlinux.a -- >> ld.lld: error: undefined symbol: crypto_unregister_acomp >>> referenced by crypto_engine.c >>> crypto/crypto_engine.o:(crypto_engine_unregister_acomp) in archive vmlinux.a
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki