linux-kernel - Re: [PATCH 09/11] power: supply: pm8916_lbc: Fix use-after-free in power_supply

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <pndtswxebnc.a.out@axis.com>
Date: Wed, 7 Jan 2026 15:32:39 +0100
From: Waqar Hameed <waqar.hameed@...s.com>
To: Nikita Travkin <nikita@...n.ru>
CC: Sebastian Reichel <sre@...nel.org>, <kernel@...s.com>,
	<linux-pm@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 09/11] power: supply: pm8916_lbc: Fix use-after-free in
 power_supply_changed()

On Sun, Dec 21, 2025 at 10:45 +0500 Nikita Travkin <nikita@...n.ru> wrote:

> 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@...s.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!

Thank _you_ for reviewing!

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

Hm, it _is_ actually a problem. During `probe()`, it's fine, due to the
NULL check in `extcon_set_state()` (and the interrupt handler doesn't
check the return value anyway), as you mention. However, during removal,
we have the exact same situation as for `power_supply_changed()` as
explained in the commit message; `devm_extcon_dev_release()` runs and
frees `struct extcon_dev *edev`, the interrupt handler would now call

  `extcon_set_state_sync(chg->edev, ...)` ->
  `extcon_set_state(edev, ...)` ->
  `find_cable_index_by_id(edev, ...)`
  
with an invalid `edev` triggering a crash/corruption in
`find_cable_index_by_id()` (before we get the chance to release the IRQ
handler)!

Good catch! Let's move the registration a further bit down to fix this.
I will send v2 as soon as the other patches in the series also get
feedback.

>
> In any case,
>
> Reviewed-by: Nikita Travkin <nikita@...n.ru>
>
> Nikita
>
>>  	chg->edev = devm_extcon_dev_allocate(dev, pm8916_lbc_charger_cable);
>>  	if (IS_ERR(chg->edev))
>>  		return PTR_ERR(chg->edev);