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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] crypto: spacc: salg leak or priv ref underflow in
  spacc_probe_hashes()
- [Low] crypto: spacc: wrong field logged for AUX in spacc_init()
- [Low] crypto: spacc - missing stack key zeroization in context
  read/write helpers
--

commit 7f65f106112a1ad39dd6972a667e410520c333a1
Author: Pavitrakumar Managutte <pavitrakumarm@vayavyalabs.com>

crypto: spacc - Add SPAcc ahash support

This commit introduces the Synopsys DesignWare SPAcc crypto accelerator
driver along with ahash support for MD5, SHA-1, SHA-2, SHA-3, HMAC, and
SM4 CMAC/XCBC algorithms.

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

[ ... ]

> +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);
> +	struct spacc_priv *priv = salg->priv;
> +
> +	kfree(salg);
> +	kref_put(&priv->ref, spacc_priv_release);
> +}

[ ... ]

> +static int spacc_register_hash(struct spacc_alg *salg)
> +{
> +	int rc = 0;
> +	struct spacc_priv *priv = salg->priv;
> +
> +	salg->calg = &salg->alg.hash.base.halg.base;
> +	salg->alg.hash = spacc_hash_template;
> +
> +	/* Unkeyed digests must not advertise a setkey. */
> +	if (!spacc_mode_is_keyed(salg->mode->id))
> +		salg->alg.hash.base.setkey = NULL;
> +
> +	rc = spacc_init_calg(salg->calg, salg->mode);
> +	if (rc)
> +		return rc;
> +
> +	salg->alg.hash.base.halg.digestsize = salg->mode->hashlen;
> +	{
> +		struct crypto_ahash *fb;
> +
> +		/* statesize must equal the software fallback's exported state. */
> +		fb = crypto_alloc_ahash(salg->calg->cra_name, 0,
> +					CRYPTO_ALG_ASYNC);
> +		if (IS_ERR(fb))
> +			return PTR_ERR(fb);
> +		salg->alg.hash.base.halg.statesize = crypto_ahash_statesize(fb);
> +		crypto_free_ahash(fb);
> +	}
> +
> +	rc = crypto_engine_register_ahash(&salg->alg.hash);
> +	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)
> +{

[ ... ]

> +		if (spacc_is_mode_keysize_supported(&priv->spacc,
> +					possible_hashes[index].id & 0xFF,
> +					possible_hashes[index].hashlen, 1)) {
> +			salg = kmalloc_obj(*salg, GFP_KERNEL);
> +			if (!salg) {
> +				rc = -ENOMEM;
> +				goto err_unregister;
> +			}
> +
> +			salg->mode = &possible_hashes[index];
> +			salg->dev = &spacc_pdev->dev;
> +			salg->priv = priv;
> +			kref_get(&priv->ref);
> +
> +			rc = spacc_register_hash(salg);
> +			if (rc < 0) {
> +				kref_put(&priv->ref, spacc_priv_release);
> +				continue;
> +			}
> +			registered++;
> +		}
> +	}

[Severity: High]
Can a failure in spacc_register_hash() leak salg or underflow priv->ref?

If spacc_register_hash() fails before crypto_check_alg() initializes
cra_refcnt (for example in spacc_init_calg() or crypto_alloc_ahash()),
spacc_hash_alg_destroy() never runs and spacc_probe_hashes() never frees
salg.

Conversely, if crypto_engine_register_ahash() fails inside
__crypto_register_alg() (for example with -EEXIST when a second SPAcc
device probes with the same "spacc-<alg>" cra_driver_name),
crypto_register_alg() calls crypto_alg_put() -> spacc_hash_alg_destroy(),
which frees salg and drops priv->ref.  The error path in
spacc_probe_hashes() then calls kref_put(&priv->ref, spacc_priv_release) a
second time, freeing priv while spacc_probe_hashes() is still using it.

Taking priv->ref inside spacc_register_hash() right before
crypto_engine_register_ahash() and freeing salg directly on earlier errors
would avoid both issues.

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

[ ... ]

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

[Severity: Low]
Should this print info->spacc_version.aux instead of
info->spacc_version.qos?

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

[ ... ]

> +/* Write appropriate context data which depends on operation and mode */
> +int spacc_write_context(struct spacc_device *spacc, int job_idx, int op,
> +			const unsigned char *key, int ksz,
> +			const unsigned char *iv, int ivsz)
> +{
> +	int buflen;
> +	int ret = 0;
> +	unsigned char buf[300] __aligned(sizeof(u32));
> +	struct spacc_ctx *ctx = NULL;
> +	struct spacc_job *job = NULL;

[ ... ]

> +	return ret;
> +}
> +
> +int spacc_read_context(struct spacc_device *spacc, int job_idx,
> +		       int op, unsigned char *key, int ksz,
> +		       unsigned char *iv, int ivsz)
> +{
> +	int buflen;
> +	int ret = 0;
> +	unsigned char buf[300] __aligned(sizeof(u32));
> +	struct spacc_ctx *ctx = NULL;
> +	struct spacc_job *job = NULL;

[ ... ]

> +	return ret;
> +}

[Severity: Low]
Should spacc_write_context() and spacc_read_context() clear buf with
memzero_explicit() before returning so cipher and hash keys are not left on
the stack?

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