Re: [PATCH 09/11] power: supply: pm8916_lbc: Fix use-after-free in power_supply_changed()

[Nikita Travkin <nikita@trvn.ru>]

Waqar Hameed писал(а) 21.12.2025 03:36:
> Using the `devm_` variant for requesting IRQ _before_ the `devm_`
> variant for allocating/registering the `power_supply` handle, means that
> the `power_supply` handle will be deallocated/unregistered _before_ the
> interrupt handler (since `devm_` naturally deallocates in reverse
> allocation order). This means that during removal, there is a race
> condition where an interrupt can fire just _after_ the `power_supply`
> handle has been freed, *but* just _before_ the corresponding
> unregistration of the IRQ handler has run.
> 
> This will lead to the IRQ handler calling `power_supply_changed()` with
> a freed `power_supply` handle. Which usually crashes the system or
> otherwise silently corrupts the memory...
> 
> Note that there is a similar situation which can also happen during
> `probe()`; the possibility of an interrupt firing _before_ registering
> the `power_supply` handle. This would then lead to the nasty situation
> of using the `power_supply` handle *uninitialized* in
> `power_supply_changed()`.
> 
> Fix this racy use-after-free by making sure the IRQ is requested _after_
> the registration of the `power_supply` handle.
> 
> Fixes: f8d7a3d21160 ("power: supply: Add driver for pm8916 lbc")
> Signed-off-by: Waqar Hameed <waqar.hameed@axis.com>
> ---
>  drivers/power/supply/pm8916_lbc.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/power/supply/pm8916_lbc.c b/drivers/power/supply/pm8916_lbc.c
> index c74b75b1b2676..3ca717d84aade 100644
> --- a/drivers/power/supply/pm8916_lbc.c
> +++ b/drivers/power/supply/pm8916_lbc.c
> @@ -274,15 +274,6 @@ static int pm8916_lbc_charger_probe(struct platform_device *pdev)
>  		return dev_err_probe(dev, -EINVAL,
>  				     "Wrong amount of reg values: %d (4 expected)\n", len);
>  
> -	irq = platform_get_irq_byname(pdev, "usb_vbus");
> -	if (irq < 0)
> -		return irq;
> -
> -	ret = devm_request_threaded_irq(dev, irq, NULL, pm8916_lbc_charger_state_changed_irq,
> -					IRQF_ONESHOT, "pm8916_lbc", chg);
> -	if (ret)
> -		return ret;
> -
>  	ret = device_property_read_u32_array(dev, "reg", chg->reg, len);
>  	if (ret)
>  		return ret;
> @@ -332,6 +323,15 @@ static int pm8916_lbc_charger_probe(struct platform_device *pdev)
>  	if (ret)
>  		return dev_err_probe(dev, ret, "Unable to get battery info\n");
>  
> +	irq = platform_get_irq_byname(pdev, "usb_vbus");
> +	if (irq < 0)
> +		return irq;
> +
> +	ret = devm_request_threaded_irq(dev, irq, NULL, pm8916_lbc_charger_state_changed_irq,
> +					IRQF_ONESHOT, "pm8916_lbc", chg);
> +	if (ret)
> +		return ret;
> +

Thank you for looking at those drivers and fixing this!

As a small note, the interrupt handler also has a call to
extcon_set_state_sync(chg->edev,...) which is allocated right below.
I don't think this is actually a problem since it has a null check for
edev (unlike psy core) so I think this patch is fine as-is. However if
for some reason you'd have to respin this series, perhaps it would be
nice to move irq registration slightly lower, after extcon registration.

In any case,

Reviewed-by: Nikita Travkin <nikita@trvn.ru>

Nikita

>  	chg->edev = devm_extcon_dev_allocate(dev, pm8916_lbc_charger_cable);
>  	if (IS_ERR(chg->edev))
>  		return PTR_ERR(chg->edev);