Re: [PATCH v26 2/4] crypto: spacc - Add SPAcc ahash support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 13 potential issue(s) to consider:
- [High] crypto: spacc: double free of salg in spacc_probe_hashes() on error
- [High] crypto: spacc: BUG_ON() in spacc_sg_to_ddt() via sg_last() on partial sg
- [High] crypto: spacc: statesize overflow and broken BLOCK_ONLY in spacc_register_hash()
- [High] crypto: spacc: unconditionally setting CRYPTO_ALG_OPTIONAL_KEY and setkey()
- [High] crypto: spacc: unhandled CMD0 interrupt storm in spacc_process_irq()
- [High] crypto: spacc: use-after-free and kthread leak in spacc_crypto_remove()
- [High] crypto: spacc: premature dma_ref_cnt dec and missing wake in do_one_request()
- [Medium] crypto: spacc: spacc_job_request() treats -EINVAL from spacc_ctx_request() as valid
- [Medium] crypto: spacc: out-of-bounds read of version_string[] in spacc_init()
- [Medium] crypto: spacc: missing acquire barrier in spacc_compute_xcbc_key() wait
- [Medium] crypto: spacc: unlocked IRQ state updates and unmasked status in IRQ handler
- [Medium] crypto: spacc - copy digest only on success in spacc_digest_cb()
- [Low] crypto: spacc: missing closing brace in spacc_unregister_hash_algs()
--

commit 13f9355d3fad0b1de2c06505109dd33bc6804ae0
Author: Pavitrakumar Managutte <pavitrakumarm@vayavyalabs.com>

crypto: spacc - Add SPAcc ahash support

This commit adds core SPAcc driver support along with ahash algorithm
implementations for MD5, SHA1, SHA2, SHA3, SM4-CMAC, and SM4-XCBC.

> diff --git a/drivers/crypto/dwc-spacc/spacc_ahash.c b/drivers/crypto/dwc-spacc/spacc_ahash.c
> new file mode 100644
> index 0000000000000..76dd497a0a17a
> --- /dev/null
> +++ b/drivers/crypto/dwc-spacc/spacc_ahash.c

[ ... ]

> +static int spacc_hash_init_dma(struct device *dev, struct ahash_request *req)
> +{
> +	int rc = -1;
> +	struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
> +	struct spacc_crypto_ctx *tctx = crypto_ahash_ctx(tfm);
> +	struct spacc_crypto_reqctx *ctx = ahash_request_ctx(req);
> +	struct spacc_priv *priv = dev_get_drvdata(dev);
> +
> +	/*
> +	 * do_one_request runs in a sleepable context,
> +	 * so GFP_KERNEL is always safe here.
> +	 */
> +	gfp_t mflags = GFP_KERNEL;
> +
> +	ctx->acb.new_handle = -1;
> +	ctx->digest_buf = dma_pool_alloc(priv->hash_pool, mflags,
> +					 &ctx->digest_dma);

[ ... ]

> +static void spacc_digest_cb(void *spacc, void *tfm)
> +{
> +	int err;
> +	int dig_sz;
> +	struct ahash_cb_data *cb = tfm;
> +	struct spacc_device *device = spacc;
> +	struct spacc_priv *priv = container_of(device, struct spacc_priv,
> +					       spacc);
> +
> +	dig_sz = crypto_ahash_digestsize(crypto_ahash_reqtfm(cb->req));
> +
> +	if (cb->ctx->single_shot)
> +		memcpy(cb->req->result, cb->ctx->digest_buf, dig_sz);
> +	else
> +		memcpy(cb->tctx->digest_ctx_buf, cb->ctx->digest_buf, dig_sz);
> +
> +	err = cb->spacc->job[cb->new_handle].job_err;

[Severity: Medium]
Should spacc_digest_cb() check job_err before copying cb->ctx->digest_buf
into cb->req->result?

In spacc_hash_init_dma(), ctx->digest_buf is allocated from priv->hash_pool
via dma_pool_alloc() without __GFP_ZERO, so it contains dma_pool freelist
metadata and stale digest bytes until the hardware writes the result. If a
job fails in hardware or during deferred submission in spacc_process_jb()
with a non-zero job_err, copying digest_buf unconditionally overwrites
cb->req->result with uninitialized or stale kernel memory.

[ ... ]

> +static int spacc_hash_try_hw(struct spacc_crypto_ctx *tctx,
> +			     struct spacc_priv *priv,
> +			     const struct spacc_alg *salg,
> +			     struct crypto_ahash *reqtfm,
> +			     struct ahash_request *req)
> +{
> +	int rc;
> +
> +	guard(mutex)(&tctx->lock);
> +
> +	if (tctx->handle < 0 || !tctx->ctx_valid) {
> +		/*
> +		 * spacc_open() acquires ctx_sem which may sleep;
> +		 * do_one_request runs in the crypto_engine workqueue
> +		 * which is a sleepable context, so this is safe.
> +		 */
> +		tctx->handle = spacc_open(&priv->spacc,
> +					  CRYPTO_MODE_NULL, salg->mode->id,
> +					  -1, 0, spacc_digest_cb, reqtfm);
> +		if (tctx->handle < 0) {
> +			dev_dbg(salg->dev, "context open failed, fallback\n");
> +			return tctx->handle;
> +		}
> +
> +		rc = spacc_set_operation(&priv->spacc, tctx->handle,
> +				OP_ENCRYPT, ICV_HASH, IP_ICV_OFFSET, 0, 0, 0);
> +		if (rc < 0) {
> +			spacc_close(&priv->spacc, tctx->handle);
> +			tctx->handle = -1;
> +			return rc;
> +		}
> +
> +		if (tctx->keylen > 0) {
> +			u32 mode_id = salg->mode->id;

[ ... ]

> +/* Crypto engine hash operation */
> +static int spacc_hash_do_one_request(struct crypto_engine *engine, void *areq)
> +{

[ ... ]

> +	atomic_inc(&priv->dma_ref_cnt);
> +
> +	rc = spacc_packet_enqueue_ddt(&priv->spacc, ctx->acb.new_handle,
> +			&ctx->src, &ctx->dst, req->nbytes,
> +			0, req->nbytes, 0, 0, 0, false);
> +	if (rc < 0) {
> +
> +		atomic_dec(&priv->dma_ref_cnt);
> +		spacc_free_mems(ctx, tctx, req);
> +		dma_pool_free(priv->hash_pool, ctx->digest_buf,
> +				ctx->digest_dma);
> +		pdu_ddt_free(&ctx->dst);
> +
> +		if (ctx->acb.new_handle >= 0) {
> +			spacc_close(&priv->spacc, ctx->acb.new_handle);
> +			ctx->acb.new_handle = -1;
> +		}

[Severity: High]
In spacc_hash_do_one_request(), when spacc_packet_enqueue_ddt() fails,
priv->dma_ref_cnt is decremented before freeing the DMA buffers and closing
the job handle, and priv->drain_wq is not woken up.

If spacc_crypto_remove() is waiting on priv->drain_wq, could it either hang
indefinitely because wake_up(&priv->drain_wq) is never called when
dma_ref_cnt reaches zero, or wake up early and destroy priv->hash_pool, the
DDT pools, and spacc->job while this error path is still freeing them?

Should this error path release all resources first and then call
atomic_dec_and_test(&priv->dma_ref_cnt) followed by
wake_up(&priv->drain_wq), matching spacc_digest_cb()?

[ ... ]

> +static void spacc_hash_exit_tfm(struct crypto_ahash *tfm)
> +{
> +	struct spacc_crypto_ctx *tctx = crypto_ahash_ctx(tfm);
> +	struct spacc_priv *priv = dev_get_drvdata(tctx->dev);
> +
> +	if (priv && tctx->handle >= 0)
> +		spacc_close(&priv->spacc, tctx->handle);
> +
> +	put_device(tctx->dev);
> +	mutex_destroy(&tctx->lock);
> +}

[ ... ]

> +static int spacc_hash_digest(struct ahash_request *req)
> +{
> +	struct crypto_ahash *reqtfm = crypto_ahash_reqtfm(req);
> +	struct spacc_crypto_ctx *tctx = crypto_ahash_ctx(reqtfm);
> +	struct spacc_priv *priv = dev_get_drvdata(tctx->dev);
> +	unsigned long flags;
> +	bool removing;
> +
> +	if (!priv || !priv->engine)
> +		return -ENODEV;
> +
> +	spin_lock_irqsave(&priv->spacc.lock, flags);
> +	removing = priv->removing;
> +	spin_unlock_irqrestore(&priv->spacc.lock, flags);
> +
> +	if (removing)
> +		return -ENODEV;
> +
> +	return crypto_transfer_hash_request_to_engine(priv->engine, req);
> +}

[ ... ]

> +static void spacc_hash_alg_destroy(struct crypto_alg *alg)
> +{
> +	struct spacc_alg *salg = container_of(alg, struct spacc_alg,
> +					      alg.hash.base.halg.base);
> +
> +	kfree(salg);
> +}
> +
> +static const struct ahash_engine_alg spacc_hash_template = {
> +	.base = {
> +		.init		= spacc_hash_init,
> +		.update		= spacc_hash_update,
> +		.final		= spacc_hash_final,
> +		.finup		= spacc_hash_finup,
> +		.digest		= spacc_hash_digest,
> +		.setkey		= spacc_hash_setkey,
> +		.export		= spacc_hash_export,
> +		.import		= spacc_hash_import,
> +		.init_tfm	= spacc_hash_init_tfm,
> +		.exit_tfm	= spacc_hash_exit_tfm,
> +
> +		.halg.base = {
> +			.cra_priority	= 300,
> +			.cra_module	= THIS_MODULE,
> +			.cra_ctxsize	= sizeof(struct spacc_crypto_ctx),
> +			.cra_reqsize	= sizeof(struct spacc_crypto_reqctx),
> +			.cra_destroy	= spacc_hash_alg_destroy,
> +			.cra_flags	= CRYPTO_ALG_TYPE_AHASH		|
> +					  CRYPTO_ALG_ASYNC		|
> +					  CRYPTO_AHASH_ALG_BLOCK_ONLY	|
> +					  CRYPTO_ALG_OPTIONAL_KEY
> +		},
> +	},
> +	.op = {
> +		.do_one_request = spacc_hash_do_one_request,
> +	},
> +};

[Severity: High]
spacc_hash_template is shared across all 18 algorithms in possible_hashes[],
unconditionally setting .setkey = spacc_hash_setkey and
CRYPTO_ALG_OPTIONAL_KEY in .cra_flags.

Because CRYPTO_ALG_OPTIONAL_KEY is set, ahash_set_needkey() never sets
CRYPTO_TFM_NEED_KEY on keyed algorithms (cmac(sm4), xcbc(sm4), and hmac(*)).
If a caller invokes crypto_ahash_digest() without calling
crypto_ahash_setkey() (or after a 0-length HMAC setkey()),
spacc_hash_try_hw() sees tctx->keylen == 0, skips spacc_write_context(), and
runs the MAC in hardware using whatever stale key bytes remain in
ctx->hash_key instead of returning -ENOKEY.

Conversely, setting .setkey on the unkeyed hash algorithms (md5, sha1,
sha2*, sha3-*) causes crypto_hash_alg_has_setkey() to report that unkeyed
digests require a key.

Should CRYPTO_ALG_OPTIONAL_KEY be dropped, with .setkey only populated for
the keyed MAC and HMAC algorithms, and should spacc_hash_try_hw() still
program a zero-length key when a valid 0-length HMAC key is set?

> +
> +static int spacc_register_hash(struct spacc_alg *salg)
> +{
> +	int rc = 0;
> +	struct spacc_priv *priv = dev_get_drvdata(salg->dev);
> +
> +	salg->calg = &salg->alg.hash.base.halg.base;
> +	salg->alg.hash = spacc_hash_template;
> +
> +	rc = spacc_init_calg(salg->calg, salg->mode);
> +	if (rc)
> +		return rc;
> +
> +	salg->alg.hash.base.halg.digestsize = salg->mode->hashlen;
> +	salg->alg.hash.base.halg.statesize = HASH_MAX_STATESIZE;
> +
> +	rc = crypto_engine_register_ahash(&salg->alg.hash);

[Severity: High]
In spacc_register_hash(), salg->alg.hash.base.halg.statesize is set to
HASH_MAX_STATESIZE while spacc_hash_template sets
CRYPTO_AHASH_ALG_BLOCK_ONLY.

When crypto_engine_register_ahash() calls ahash_prepare_alg(), the crypto
core adds cra_blocksize + 1 to statesize when CRYPTO_AHASH_ALG_BLOCK_ONLY is
set, making crypto_ahash_statesize(tfm) exceed HASH_MAX_STATESIZE.

As a result, any virtual-address request in ahash_do_req_chain() (such as
crypto/hmac.c wrapping SPAcc sha3 algorithms) hits:

    if (crypto_ahash_statesize(tfm) > HASH_MAX_STATESIZE)
        return -ENOSYS;

and fails, while callers of crypto_ahash_export() with a HASH_MAX_STATESIZE
buffer will have cra_blocksize + 1 bytes written past the end of their
buffer by ahash_save_req().

Since spacc_hash_init(), spacc_hash_update(), spacc_hash_final(), and
spacc_hash_finup() delegate incremental hashing to the software fallback
rather than implementing block-only partial-block return semantics, should
CRYPTO_AHASH_ALG_BLOCK_ONLY be removed from spacc_hash_template?

> +	if (rc < 0)
> +		return rc;
> +
> +	guard(mutex)(&priv->hash_alg_mutex);
> +	list_add(&salg->list, &priv->hash_alg_list);
> +
> +	return 0;
> +}
> +
> +int spacc_probe_hashes(struct platform_device *spacc_pdev)
> +{

[ ... ]

> +			salg->mode = &possible_hashes[index];
> +			salg->dev = &spacc_pdev->dev;
> +
> +			rc = spacc_register_hash(salg);
> +			if (rc < 0) {
> +				kfree(salg);
> +				continue;
> +			}

[Severity: High]
Can this cause a double free of salg when spacc_register_hash() fails?

spacc_register_hash() installs spacc_hash_alg_destroy() (which calls
kfree(salg)) as cra_destroy and then calls
crypto_engine_register_ahash(&salg->alg.hash) -> crypto_register_ahash() ->
crypto_register_alg().

In crypto_register_alg(), once crypto_check_alg() initializes cra_refcnt to
1, any failure in __crypto_register_alg() (such as -EEXIST or -ENOMEM) calls
crypto_alg_put(alg) -> crypto_destroy_alg(alg) -> alg->cra_destroy(alg),
which already frees salg via spacc_hash_alg_destroy() before returning the
error to spacc_probe_hashes(), where kfree(salg) is called a second time.

> +
> +			registered++;
> +		}
> +	}
> +
> +	return registered;
> +
> +err_unregister:
> +	spacc_unregister_hash_algs(priv);
> +	return rc;
> +}
> +
> +int spacc_unregister_hash_algs(struct spacc_priv *priv)
> +{
> +	struct spacc_alg *salg, *tmp;
> +
> +	if (!priv)
> +		return 0;
> +
> +	guard(mutex)(&priv->hash_alg_mutex);
> +
> +	list_for_each_entry_safe(salg, tmp, &priv->hash_alg_list, list) {
> +		list_del(&salg->list);
> +		crypto_engine_unregister_ahash(&salg->alg.hash);
> +	}
> +
> +	return 0;

[Severity: Low]
spacc_unregister_hash_algs() is missing its closing brace at the end of
drivers/crypto/dwc-spacc/spacc_ahash.c, which causes a syntax error when
building the driver.

> diff --git a/drivers/crypto/dwc-spacc/spacc_core.c b/drivers/crypto/dwc-spacc/spacc_core.c
> new file mode 100644
> index 0000000000000..aa5fbe9a3e074
> --- /dev/null
> +++ b/drivers/crypto/dwc-spacc/spacc_core.c

[ ... ]

> +int spacc_sg_to_ddt(struct device *dev, struct scatterlist *sg,
> +		    int nbytes, struct pdu_ddt *ddt, int dma_direction)
> +{
> +	int i;
> +	int nents;
> +	int rc = 0;
> +	int orig_nents;
> +	struct scatterlist *sgl;
> +	struct scatterlist *sg_entry;
> +
> +	/* Count the SG entries needed to cover the requested length. */
> +	orig_nents = sg_nents_for_len(sg, nbytes);
> +	if (orig_nents <= 0)
> +		return orig_nents;
> +
> +	/*
> +	 * Ignore a trailing zero-length SG entry if present.
> +	 * Some callers use it as a terminator.
> +	 */
> +	if (orig_nents > 1) {
> +		sgl = sg_last(sg, orig_nents);
> +		if (sgl->length == 0)
> +			orig_nents--;
> +	}

[Severity: High]
Won't calling sg_last(sg, orig_nents) here trigger BUG_ON(!sg_is_last(ret))
in sg_last() whenever the scatterlist contains more entries than the prefix
needed to cover nbytes?

sg_nents_for_len(sg, nbytes) stops as soon as the accumulated length reaches
nbytes and returns the number of entries in that prefix. If orig_nents > 1
and the caller's scatterlist has any entries after the orig_nents-th entry
(including a trailing zero-length entry after entries that already sum to
nbytes, or a buffer larger than req->nbytes), the orig_nents-th entry does
not have the SG_END marker set, causing sg_last() to hit BUG_ON().

[ ... ]

> +int spacc_init(void __iomem *baseaddr, struct spacc_device *spacc,
> +	       struct pdu_info *info, struct device *dev)
> +{
> +	int ret;
> +#ifdef CONFIG_CRYPTO_DEV_SPACC_CONFIG_DEBUG
> +	unsigned long id;
> +	char version_string[3][16]  = { "SPACC", "SPACC-PDU" };
> +	char idx_string[2][16]      = { "(Normal Port)", "(Secure Port)" };
> +	char dma_type_string[4][16] = { "Unknown", "Scattergather", "Linear",
> +					"Unknown" };
> +#endif

[ ... ]

> +	if (spacc->config.version >= 0x48)
> +		dev_dbg(spacc->dptr,
> +			"   TYPE       : %lx (%s)\n", SPACC_ID_TYPE(id),
> +			version_string[SPACC_ID_TYPE(id) & 3]);

[Severity: Medium]
Does indexing version_string[] with (SPACC_ID_TYPE(id) & 3) read out of
bounds when SPACC_ID_TYPE(id) is 3?

version_string is declared as char version_string[3][16] with only 3
elements (and only 2 initialized), whereas SPACC_ID_TYPE(id) & 3 can
evaluate to 3.

[ ... ]

> +int spacc_compute_xcbc_key(struct spacc_device *spacc, int mode_id,
> +			   int job_idx, const unsigned char *key,
> +			   int keylen, unsigned char *xcbc_out)
> +{

[ ... ]

> +			if (wait_event_interruptible(job->waitq,
> +						     job->job_done))
> +				wait_event(job->waitq,
> +					/*
> +					 * Pairs with smp_store_release() of
> +					 * job_done in completion path.
> +					 */
> +					 smp_load_acquire(&job->job_done));
> +
> +			job->job_done = 0;
> +			err = job->job_err;
> +			if (err != 0)
> +				goto xcbc_err;
> +		}
> +	} else {
> +		/*
> +		 * Do the 48 bytes as a single SPAcc job this is the ideal case
> +		 * but only possible if ECB was enabled in the core
> +		 */
> +		spacc_write_context(spacc, handle, SPACC_CRYPTO_OPERATION,
> +				    key, keylen, iv, 16);
> +		err = spacc_packet_enqueue_ddt(spacc, handle, &ddt, &ddt, 48,
> +					       0, 0, 0, 0, 0, false);
> +		if (err != 0)
> +			goto xcbc_err;
> +
> +		if (wait_event_interruptible(job->waitq, job->job_done))
> +
> +			wait_event(job->waitq,
> +				  /*
> +				   * Pairs with smp_store_release() of
> +				   * job_done in completion path.
> +				   */
> +				   smp_load_acquire(&job->job_done));
> +
> +		job->job_done = 0;
> +		err = job->job_err;
> +		if (err != 0)
> +			goto xcbc_err;
> +	}

[Severity: Medium]
In both branches of spacc_compute_xcbc_key(), wait_event_interruptible()
checks plain job->job_done and only falls back to wait_event(...,
smp_load_acquire(&job->job_done)) when interrupted by a signal.

When wait_event_interruptible() succeeds without a signal (returning 0), the
wait_event() call is skipped. If job->job_done is already set before
wait_event_interruptible() is entered, its fast path reads job->job_done
without taking waitq.lock and without an acquire barrier to pair with the
smp_store_release(&job->job_done, 1) in spacc_pop_one_job() and
spacc_process_jb().

Should wait_event_interruptible() also use smp_load_acquire(&job->job_done)
(or simply use wait_event(job->waitq, smp_load_acquire(&job->job_done))
directly)?

> diff --git a/drivers/crypto/dwc-spacc/spacc_device.c b/drivers/crypto/dwc-spacc/spacc_device.c
> new file mode 100644
> index 0000000000000..af95be0a8ebc1
> --- /dev/null
> +++ b/drivers/crypto/dwc-spacc/spacc_device.c

[ ... ]

> +static void spacc_crypto_remove(struct platform_device *pdev)
> +{
> +	struct spacc_priv *priv = platform_get_drvdata(pdev);
> +	unsigned long flags;
> +
> +	/*
> +	 * Set before anything else: sysfs unbind (unlike rmmod) has no
> +	 * built-in wait for existing tfms to be freed, so a request can
> +	 * reach spacc_hash_digest() at any point during this function.
> +	 * This flag, checked under the same lock there, blocks any new
> +	 * submission attempt from this point on.
> +	 */
> +	spin_lock_irqsave(&priv->spacc.lock, flags);
> +	priv->removing = true;
> +	spin_unlock_irqrestore(&priv->spacc.lock, flags);
> +
> +	/* Stop the crypto API from handing out any new tfms. */
> +	spacc_unregister_algs(priv);
> +
> +	if (priv->engine)
> +		crypto_engine_exit(priv->engine);
> +
> +	wait_event(priv->drain_wq, !atomic_read(&priv->dma_ref_cnt));
> +
> +	/* Nothing can dispatch new work past this point. */
> +	spin_lock_irqsave(&priv->spacc.lock, flags);
> +	spacc_irq_glbl_disable(&priv->spacc);
> +	spin_unlock_irqrestore(&priv->spacc.lock, flags);
> +
> +	devm_free_irq(&pdev->dev, priv->irq, &pdev->dev);
> +
> +	if (priv->spacc_wq)
> +		destroy_workqueue(priv->spacc_wq);
> +
> +	/* No job can still reference this pool past the drain above. */
> +	dma_pool_destroy(priv->hash_pool);
> +	priv->hash_pool = NULL;
> +
> +	spacc_remove(pdev);
> +	platform_set_drvdata(pdev, NULL);
> +}

[Severity: High]
Can unbinding the device in spacc_crypto_remove() race with active requests
and surviving tfms, leading to a use-after-free or a leaked crypto_engine
kthread?

First, in spacc_hash_digest():

    spin_lock_irqsave(&priv->spacc.lock, flags);
    removing = priv->removing;
    spin_unlock_irqrestore(&priv->spacc.lock, flags);

    if (removing)
        return -ENODEV;

    return crypto_transfer_hash_request_to_engine(priv->engine, req);

priv->spacc.lock is dropped before crypto_transfer_hash_request_to_engine()
is called, and priv->dma_ref_cnt is not incremented until
spacc_hash_do_one_request() after spacc_hash_try_hw() completes. If
spacc_crypto_remove() runs in that window, wait_event(priv->drain_wq, ...)
can see dma_ref_cnt == 0 and proceed to tear down priv->engine,
priv->hash_pool, and spacc while the request is still in flight.

Second, crypto_engine_exit(priv->engine) is called before
wait_event(priv->drain_wq, ...). Because retry_support is false,
engine->busy remains true until spacc_digest_cb() calls
crypto_finalize_hash_request(); if an in-flight job takes longer than
crypto_engine_stop()'s 200ms timeout, crypto_engine_exit() returns early
without calling kthread_destroy_worker(), leaking the worker kthread.

Third, when unbound via sysfs, existing crypto_ahash tfms can outlive
spacc_crypto_remove(). Because priv is allocated with devm_kzalloc(), devres
frees priv once spacc_crypto_remove() returns (get_device() in
spacc_hash_init_tfm() pins struct device, not devres resources), and
spacc_remove() frees spacc->ctx and spacc->job before
platform_set_drvdata(pdev, NULL). A concurrent or later call to
spacc_hash_exit_tfm(), spacc_hash_setkey(), or spacc_hash_digest() can read
a non-NULL drvdata pointer and dereference freed spacc->job, spacc->ctx, or
priv.

> diff --git a/drivers/crypto/dwc-spacc/spacc_interrupt.c b/drivers/crypto/dwc-spacc/spacc_interrupt.c
> new file mode 100644
> index 0000000000000..68dd049a5f65a
> --- /dev/null
> +++ b/drivers/crypto/dwc-spacc/spacc_interrupt.c

[ ... ]

> +u32 spacc_process_irq(struct spacc_device *spacc)
> +{
> +	u32 irq_status;
> +	int x, cmd_max;
> +	unsigned long lock_flag;
> +
> +	spin_lock_irqsave(&spacc->lock, lock_flag);
> +
> +	irq_status = readl(spacc->regmap + SPACC_REG_IRQ_STAT);
> +
> +	/* clear interrupt pin and run registered callback */
> +	if (irq_status & SPACC_IRQ_STAT_STAT) {
> +		SPACC_IRQ_STAT_CLEAR_STAT(spacc);
> +		if (spacc->op_mode == SPACC_OP_MODE_IRQ) {
> +			spacc->config.fifo_cnt <<= 2;
> +			spacc->config.fifo_cnt = min(spacc->config.fifo_cnt,
> +					spacc->config.stat_fifo_depth);
> +
> +			/* update fifo count to allow more stats to pile up */
> +			spacc_irq_stat_enable(spacc, spacc->config.fifo_cnt);
> +
> +			/* re-enable CMD0 empty interrupt */
> +			spacc_irq_cmdx_enable(spacc, 0, 0);
> +		}
> +
> +		/* Re-enable the watchdog interrupt */
> +		if (spacc->op_mode == SPACC_OP_MODE_IRQ &&
> +				spacc->wd_cnt_limit) {
> +			spacc_irq_stat_wd_enable(spacc);
> +			spacc->wdcnt = 0;
> +			spacc->op_mode = SPACC_OP_MODE_WD;
> +			spacc->wd_cnt_limit = false;
> +		}
> +
> +		if (spacc->irq_cb_stat)
> +			spacc->irq_cb_stat(spacc);
> +	}

[Severity: High]
Can this sequence in spacc_process_irq() trigger an unhandled CMD0 interrupt
storm when recovering from watchdog limit fallback?

When spacc->wdcnt reaches SPACC_WD_LIMIT in SPACC_OP_MODE_WD,
spacc_process_irq() switches spacc->op_mode to SPACC_OP_MODE_IRQ and sets
spacc->wd_cnt_limit = true.

On the next SPACC_IRQ_STAT_STAT interrupt, the first block sees
spacc->op_mode == SPACC_OP_MODE_IRQ and calls:

    spacc_irq_cmdx_enable(spacc, 0, 0);

Immediately after, the second block sees spacc->op_mode == SPACC_OP_MODE_IRQ
&& spacc->wd_cnt_limit and switches spacc->op_mode back to SPACC_OP_MODE_WD
without disabling CMD0 interrupts.

Because the CMDX servicing loop below only runs when spacc->op_mode ==
SPACC_OP_MODE_IRQ:

    if (spacc->op_mode == SPACC_OP_MODE_IRQ) {
        cmd_max = (spacc->config.is_qos ? SPACC_CMDX_MAX_QOS :
                SPACC_CMDX_MAX);
        for (x = 0; x < cmd_max; x++) {
            if (irq_status & SPACC_IRQ_STAT_CMDX(x)) {
                ...

once SPACC_IRQ_STAT_CMDX(0) asserts in SPACC_REG_IRQ_STAT,
spacc_process_irq() will never clear or disable it while spacc_irq_handler()
keeps returning IRQ_HANDLED. Should the two if-blocks in the
SPACC_IRQ_STAT_STAT handler be an if / else if (checking wd_cnt_limit
first)?

[ ... ]

> +/* Function to run callbacks in the IRQ handler */
> +irqreturn_t spacc_irq_handler(int irq, void *dev)
> +{
> +	struct spacc_priv *priv = platform_get_drvdata(to_platform_device(dev));
> +	struct spacc_device *spacc = &priv->spacc;
> +
> +	if (spacc->config.oldtimer != spacc->config.timer) {
> +		spacc->config.wd_timer = spacc->config.timer;
> +		spacc_set_wd_count(spacc, spacc->config.wd_timer);
> +		spacc->config.oldtimer = spacc->config.timer;
> +	}
> +
> +	/* check irq flags and process as required */
> +	if (!spacc_process_irq(spacc))
> +		return IRQ_NONE;
> +
> +	return IRQ_HANDLED;
> +}

[Severity: Medium]
There appear to be a few concurrency and shared-IRQ handling issues between
spacc_irq_handler(), spacc_process_irq(), and spacc_init_device():

1. In spacc_irq_handler(), spacc->config.oldtimer, spacc->config.wd_timer,
and SPACC_REG_STAT_WD_CTRL are updated without holding spacc->lock and
before checking whether the interrupt came from this device on an
IRQF_SHARED line, racing with spacc_process_irq() and
spacc_pop_packets_ex().

2. In spacc_init_device(), after devm_request_irq(..., IRQF_SHARED, ...) and
spacc_irq_glbl_enable() are called, the driver modifies spacc.op_mode,
callback pointers, wd_timer, SPACC_REG_IRQ_CTRL, and SPACC_REG_IRQ_EN
without holding spacc.lock.

3. In spacc_process_irq(), irq_status is read from SPACC_REG_IRQ_STAT
without masking against SPACC_REG_IRQ_EN, and the raw irq_status is returned
to spacc_irq_handler(). If any status bits are set for a mode that is not
currently active in spacc->op_mode, spacc_irq_handler() returns IRQ_HANDLED
without clearing those bits.

Could the timer update and IRQ configuration changes be serialized under
spacc->lock, and should spacc_process_irq() only return IRQ_HANDLED for
enabled interrupt bits that it actually serviced?

> diff --git a/drivers/crypto/dwc-spacc/spacc_manager.c b/drivers/crypto/dwc-spacc/spacc_manager.c
> new file mode 100644
> index 0000000000000..0ee40fa7678cf
> --- /dev/null
> +++ b/drivers/crypto/dwc-spacc/spacc_manager.c

[ ... ]

> +static int spacc_ctx_request(struct spacc_device *spacc,
> +			     int ctx_id, int ncontig)
> +{
> +	int ret = 0;
> +	int x, y, count;
> +
> +	if (!spacc)
> +		return -EINVAL;
> +
> +	if (ctx_id < -1 || ctx_id >= spacc->config.num_ctx)
> +		return -EINVAL;
> +
> +	if (ncontig < 1 || ncontig > spacc->config.num_ctx)
> +		return -EINVAL;

[ ... ]

> +/* Get a new job id and use a specific ctx_idx or -1 for a new one */
> +int spacc_job_request(struct spacc_device *spacc, int ctx_idx)
> +{
> +	int x, ret = 0;
> +	struct spacc_job *job;
> +	unsigned long lock_flag;
> +
> +	if (!spacc)
> +		return -EINVAL;
> +
> +	spin_lock_irqsave(&spacc->lock, lock_flag);
> +
> +	/* find the first available job id */
> +	for (x = 0; x < SPACC_MAX_JOBS; x++) {
> +		job = &spacc->job[x];
> +		if (job->job_used == SPACC_JOB_IDX_UNUSED) {
> +			job->job_used = x;
> +			break;
> +		}
> +	}
> +
> +	if (x == SPACC_MAX_JOBS)
> +		ret = -1;
> +	else {
> +		/* associate a single context to go with job */
> +		ret = spacc_ctx_request(spacc, ctx_idx, 1);
> +		if (ret != -1) {
> +			job->ctx_idx = ret;
> +			ret = x;
> +		} else
> +			job->job_used = SPACC_JOB_IDX_UNUSED;
> +	}

[Severity: Medium]
In spacc_job_request(), if spacc_ctx_request() fails validation and returns
-EINVAL (for instance, if num_ctx is 0 or ctx_idx is out of range), checking
ret != -1 treats -EINVAL as a valid context index and assigns it to
job->ctx_idx.

Should spacc_job_request() check ret >= 0 instead of ret != -1?

[ ... ]

> +}

—
AI reviewed your patch. Please fix the bug or emailuar@kernel.org if the issue is a false positive.
ോട്

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019162457.1000115-1-pavitrakumarm@vayavyalabs.com?part=2