Re: [PATCH 07/10] dmaengine: bestcomm: Defer publishing bcom_eng until initialisation completes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] dmaengine: bestcomm: use-after-free in bcom_task_alloc()
  [1]
--

commit 0c7452c3ef4427c8251608e31eaba52df094162d
Author: Lee Jones <lee@kernel.org>

dmaengine: bestcomm: Defer publishing bcom_eng until initialisation completes

This commit initializes struct bcom_engine via a local pointer and defers
publishing it to the global bcom_eng pointer until initialization in
bcom_engine_init() has completed.

> diff --git a/drivers/dma/bestcomm/bestcomm.c b/drivers/dma/bestcomm/bestcomm.c
> index 241a91936fe8a..dbc3ce5fc4462 100644
> --- a/drivers/dma/bestcomm/bestcomm.c
> +++ b/drivers/dma/bestcomm/bestcomm.c
[ ... ]
> @@ -398,20 +402,20 @@ static int mpc52xx_bcom_probe(struct platform_device *op)
>  	}
>  
>  	/* Get a clean struct */
> -	bcom_eng = kzalloc_obj(struct bcom_engine);
> -	if (!bcom_eng) {
> +	eng = kzalloc_obj(struct bcom_engine);
> +	if (!eng) {
>  		rv = -ENOMEM;
>  		goto error_sramclean;
>  	}
>  
>  	/* Save the node */
> -	bcom_eng->ofnode = op->dev.of_node;
> +	eng->ofnode = op->dev.of_node;
>  
> -	bcom_eng->regs = regs;
> -	bcom_eng->regs_base = res_bcom->start;
> +	eng->regs = regs;
> +	eng->regs_base = res_bcom->start;
>  
>  	/* Now, do the real init */
> -	rv = bcom_engine_init();
> +	rv = bcom_engine_init(eng);
>  	if (rv)
>  		goto error_sramclean;
>  
> @@ -423,8 +427,7 @@ static int mpc52xx_bcom_probe(struct platform_device *op)
>  
>  	/* Error path */
>  error_sramclean:
> -	kfree(bcom_eng);
> -	bcom_eng = NULL;
> +	kfree(eng);
>  	bcom_sram_cleanup();
>  error_ofput:
>  	of_node_put(op->dev.of_node);

[Severity: Critical]
This problem wasn't introduced by this patch, but can mpc52xx_bcom_remove()
trigger a use-after-free in bcom_task_alloc() [1]?

While this patch fixes premature publication of bcom_eng during probe,
mpc52xx_bcom_remove() frees bcom_eng before clearing the global pointer and
without synchronizing against concurrent bcom_task_alloc() callers:

CPU0                              CPU1
bcom_task_alloc()
  if (!bcom_eng)
                                  mpc52xx_bcom_remove()
                                    kfree(bcom_eng);
                                    bcom_eng = NULL;
  spin_lock(&bcom_eng->lock);  <-- use-after-free

[1] https://sashiko.dev/#/bug/linux-149609ff-866f-4c27-8adf-ef935bca5f89

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111215349.1983909-1-lee@kernel.org?part=7