Re: [PATCH 3/4] crypto: hisilicon/hpre - use crypto engine framework

[sashiko-bot@kernel.org]

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 <qianweili@huawei.com>

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.

> diff --git a/drivers/crypto/hisilicon/hpre/hpre_crypto.c b/drivers/crypto/hisilicon/hpre/hpre_crypto.c
> index 66a276786a7d0..c02c33ba04d4a 100644
> --- a/drivers/crypto/hisilicon/hpre/hpre_crypto.c
> +++ b/drivers/crypto/hisilicon/hpre/hpre_crypto.c
[ ... ]
> @@ -386,10 +398,20 @@ static int hpre_ctx_init(struct hpre_ctx *ctx, u8 type)
>  		return -ENODEV;
>  	}
>  
> +	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                                  CPU1
hpre_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 kworker

Similarly, 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.

[ ... ]

> @@ -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);
> -
> -	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.

[ ... ]

> @@ -1679,86 +1732,176 @@ static void hpre_ecdh_exit_tfm(struct crypto_kpp *tfm)
[ ... ]
> +static int hpre_rsa_do_one_request(struct crypto_engine *engine, void *areq)
> +{
> +	struct akcipher_request *akreq =
> +	    container_of(areq, struct akcipher_request, base);
> +	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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109073556.2517351-1-huangchenghai2@huawei.com?part=3