Re: [PATCH] power: sequencing: pcie-m2: fix use-after-free in pwrseq_pcie_m2_vregs_enable()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] power: sequencing: overwritten ret zeroes num_vregs in pwrseq_pcie_m2_probe()
--

commit 4538b4fe7cf3308c45ac12f332947a46b82e94d0
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

power: sequencing: pcie-m2: fix use-after-free in pwrseq_pcie_m2_vregs_enable()

This commit replaces manual regulator cleanup in pwrseq_pcie_m2_remove() and
the pwrseq_pcie_m2_probe() error path with a devres action so that the
regulator array is freed only after devm_pwrseq_device_unregister() runs.

> diff --git a/drivers/power/sequencing/pwrseq-pcie-m2.c b/drivers/power/sequencing/pwrseq-pcie-m2.c
> index 56c5d51293c10..a794e2ad6a9c8 100644
> --- a/drivers/power/sequencing/pwrseq-pcie-m2.c
> +++ b/drivers/power/sequencing/pwrseq-pcie-m2.c
> @@ -533,6 +533,14 @@ static int pwrseq_pcie_m2_create_serdev(struct pwrseq_pcie_m2_ctx *ctx)
>  	return ret;
>  }
>  
> +static void pwrseq_pcie_m2_free_regulators(void *data)
> +{
> +	struct pwrseq_pcie_m2_ctx *ctx = data;
> +
> +	regulator_bulk_free(ctx->num_vregs, ctx->regs);
> +	kfree(ctx->regs);
> +}
> +
>  static int pwrseq_pcie_m2_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -561,21 +569,21 @@ static int pwrseq_pcie_m2_probe(struct platform_device *pdev)
>  		return dev_err_probe(dev, ret,
>  				     "Failed to get all regulators\n");
>  
> +	ret = devm_add_action_or_reset(dev, pwrseq_pcie_m2_free_regulators, ctx);
> +	if (ret)
> +		return ret;
> +
>  	ctx->num_vregs = ret;

[Severity: High]
In drivers/power/sequencing/pwrseq-pcie-m2.c, pwrseq_pcie_m2_probe() stores
the number of regulators returned by of_regulator_bulk_get_all() in ret, but
then overwrites ret with the return value of devm_add_action_or_reset()
before setting ctx->num_vregs = ret.

When devm_add_action_or_reset() succeeds, ret is 0, so ctx->num_vregs is
always set to 0. Doesn't this cause pwrseq_pcie_m2_vregs_enable() and
pwrseq_pcie_m2_vregs_disable() to pass 0 to regulator_bulk_enable() and
regulator_bulk_disable(), leaving the M.2 connector regulators unpowered?

Also, because ctx->num_vregs is 0 both during devm_add_action_or_reset() and
afterward, pwrseq_pcie_m2_free_regulators() will call
regulator_bulk_free(0, ctx->regs) and leak the struct regulator references in
ctx->regs before freeing the array. Should ctx->num_vregs = ret be assigned
before calling devm_add_action_or_reset()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-pwrseq-pcie-m2-uaf-v1-1-0ae1bbea9533@oss.qualcomm.com?part=1