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

[Phil Reid]

On 21/12/2025 06:36, Waqar Hameed wrote:
> 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. Keep the old behavior of
> just printing a warning in case of any failures during the IRQ request
> and finishing the probe successfully.
>
> Fixes: d2cec82c2880 ("power: sbs-battery: Request threaded irq and fix dev callback cookie")
> Signed-off-by: Waqar Hameed <waqar.hameed@xxxxxxxx>

Reviewed-by: Phil Reid <preid@xxxxxxxxxxxxxxxxx>


> ---
>   drivers/power/supply/sbs-battery.c | 36 +++++++++++++++---------------
>   1 file changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
> index 943c82ee978f4..43c48196c1674 100644
> --- a/drivers/power/supply/sbs-battery.c
> +++ b/drivers/power/supply/sbs-battery.c
> @@ -1174,24 +1174,6 @@ static int sbs_probe(struct i2c_client *client)
>   
>   	i2c_set_clientdata(client, chip);
>   
> -	if (!chip->gpio_detect)
> -		goto skip_gpio;
> -
> -	irq = gpiod_to_irq(chip->gpio_detect);
> -	if (irq <= 0) {
> -		dev_warn(&client->dev, "Failed to get gpio as irq: %d\n", irq);
> -		goto skip_gpio;
> -	}
> -
> -	rc = devm_request_threaded_irq(&client->dev, irq, NULL, sbs_irq,
> -		IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> -		dev_name(&client->dev), chip);
> -	if (rc) {
> -		dev_warn(&client->dev, "Failed to request irq: %d\n", rc);
> -		goto skip_gpio;
> -	}
> -
> -skip_gpio:
>   	/*
>   	 * Before we register, we might need to make sure we can actually talk
>   	 * to the battery.
> @@ -1217,6 +1199,24 @@ static int sbs_probe(struct i2c_client *client)
>   		return dev_err_probe(&client->dev, PTR_ERR(chip->power_supply),
>   				     "Failed to register power supply\n");
>   
> +	if (!chip->gpio_detect)
> +		goto out;
> +
> +	irq = gpiod_to_irq(chip->gpio_detect);
> +	if (irq <= 0) {
> +		dev_warn(&client->dev, "Failed to get gpio as irq: %d\n", irq);
> +		goto out;
> +	}
> +
> +	rc = devm_request_threaded_irq(&client->dev, irq, NULL, sbs_irq,
> +		IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +		dev_name(&client->dev), chip);
> +	if (rc) {
> +		dev_warn(&client->dev, "Failed to request irq: %d\n", rc);
> +		goto out;
> +	}
> +
> +out:
>   	dev_info(&client->dev,
>   		"%s: battery gas gauge device registered\n", client->name);
>   


--
Regards
Phil Reid
 
ElectroMagnetic Imaging Technology Pty Ltd
Development of Geophysical Instrumentation & Software
www.electromag.com.au
 
23 Junction Parade, Midland WA 6056, AUSTRALIA
Ph: +61 8 9250 8100
Fax: +61 8 9250 7100

Email: preid@xxxxxxxxxxxxxxxxx