[PATCH] platform/x86: dell-laptop: Fix crash when unregistering battery hook

[PATCH] platform/x86: dell-laptop: Fix crash when unregistering battery hook

[Armin Wolf]

If the battery hook encounters a unsupported battery, it will
return an error. This in turn will cause the battery driver to
automatically unregister the battery hook.

However as soon as the driver itself attempts to unregister the
already unregistered battery hook, a crash occurs due to a
corrupted linked list.

Fix this by simply ignoring unsupported batteries.

Tested on a Dell Inspiron 3505.

Fixes: ab58016c68cc ("platform/x86:dell-laptop: Add knobs to change battery charge settings")
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
I CCed the maintainers of the ACPI battery driver since i believe
that this patch highlights a general issue inside the battery hook
mechanism.

This is because the same crash will be triggered should for example
device_add_groups() fail.

Any ideas on how to solve this problem?
---
 drivers/platform/x86/dell/dell-laptop.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
index a3cd0505f282..5671bd0deee7 100644
--- a/drivers/platform/x86/dell/dell-laptop.c
+++ b/drivers/platform/x86/dell/dell-laptop.c
@@ -2391,12 +2391,18 @@ static struct attribute *dell_battery_attrs[] = {
 };
 ATTRIBUTE_GROUPS(dell_battery);

+static bool dell_battery_supported(struct power_supply *battery)
+{
+	/* We currently only support the primary battery */
+	return strcmp(battery->desc->name, "BAT0") == 0;
+}
+
 static int dell_battery_add(struct power_supply *battery,
 		struct acpi_battery_hook *hook)
 {
-	/* this currently only supports the primary battery */
-	if (strcmp(battery->desc->name, "BAT0") != 0)
-		return -ENODEV;
+	/* Return 0 instead of an error to avoid being unloaded */
+	if (!dell_battery_supported(battery))
+		return 0;

 	return device_add_groups(&battery->dev, dell_battery_groups);
 }
@@ -2404,6 +2410,9 @@ static int dell_battery_add(struct power_supply *battery,
 static int dell_battery_remove(struct power_supply *battery,
 		struct acpi_battery_hook *hook)
 {
+	if (!dell_battery_supported(battery))
+		return 0;
+
 	device_remove_groups(&battery->dev, dell_battery_groups);
 	return 0;
 }
--
2.39.5

Re: [PATCH] platform/x86: dell-laptop: Fix crash when unregistering battery hook

[Andres Salomon]

On Thu, 19 Sep 2024 08:33:32 +0200
Armin Wolf <W_Armin@gmx.de> wrote:

> If the battery hook encounters a unsupported battery, it will
> return an error. This in turn will cause the battery driver to
> automatically unregister the battery hook.
> 
> However as soon as the driver itself attempts to unregister the
> already unregistered battery hook, a crash occurs due to a
> corrupted linked list.
> 
> Fix this by simply ignoring unsupported batteries.
> 
> Tested on a Dell Inspiron 3505.
> 
> Fixes: ab58016c68cc ("platform/x86:dell-laptop: Add knobs to change battery charge settings")
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
> I CCed the maintainers of the ACPI battery driver since i believe
> that this patch highlights a general issue inside the battery hook
> mechanism.
> 
> This is because the same crash will be triggered should for example
> device_add_groups() fail.
> 
> Any ideas on how to solve this problem?
> ---

Hm, I see that when battery_hook_register() has the add_battery hook fail,
then __battery_hook_unregister() calls the remove_battery hook. Does
something like the following fix it?

(note: not any kind of final patch, just a test.)

---
 drivers/platform/x86/dell/dell-laptop.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
index a9de19799f01..c7b92b2f7ed2 100644
--- a/drivers/platform/x86/dell/dell-laptop.c
+++ b/drivers/platform/x86/dell/dell-laptop.c
@@ -2390,21 +2390,29 @@ static struct attribute *dell_battery_attrs[] = {
 	NULL,
 };
 ATTRIBUTE_GROUPS(dell_battery);
+static bool bgroup_registered = false;
 
 static int dell_battery_add(struct power_supply *battery,
 		struct acpi_battery_hook *hook)
 {
+	int err;
+
 	/* this currently only supports the primary battery */
 	if (strcmp(battery->desc->name, "BAT0") != 0)
 		return -ENODEV;
 
-	return device_add_groups(&battery->dev, dell_battery_groups);
+	err = device_add_groups(&battery->dev, dell_battery_groups);
+	if (!err)
+		bgroup_registered = true;
+
+	return err;
 }
 
 static int dell_battery_remove(struct power_supply *battery,
 		struct acpi_battery_hook *hook)
 {
-	device_remove_groups(&battery->dev, dell_battery_groups);
+	if (bgroup_registered)
+		device_remove_groups(&battery->dev, dell_battery_groups);
 	return 0;
 }
 
-- 
2.39.5


-- 
I'm available for contract & employment work, please contact me if
interested.

Re: [PATCH] platform/x86: dell-laptop: Fix crash when unregistering battery hook

[Armin Wolf]

Am 19.09.24 um 09:13 schrieb Andres Salomon:

> On Thu, 19 Sep 2024 08:33:32 +0200
> Armin Wolf <W_Armin@gmx.de> wrote:
>
>> If the battery hook encounters a unsupported battery, it will
>> return an error. This in turn will cause the battery driver to
>> automatically unregister the battery hook.
>>
>> However as soon as the driver itself attempts to unregister the
>> already unregistered battery hook, a crash occurs due to a
>> corrupted linked list.
>>
>> Fix this by simply ignoring unsupported batteries.
>>
>> Tested on a Dell Inspiron 3505.
>>
>> Fixes: ab58016c68cc ("platform/x86:dell-laptop: Add knobs to change battery charge settings")
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>> I CCed the maintainers of the ACPI battery driver since i believe
>> that this patch highlights a general issue inside the battery hook
>> mechanism.
>>
>> This is because the same crash will be triggered should for example
>> device_add_groups() fail.
>>
>> Any ideas on how to solve this problem?
>> ---
> Hm, I see that when battery_hook_register() has the add_battery hook fail,
> then __battery_hook_unregister() calls the remove_battery hook. Does
> something like the following fix it?
>
> (note: not any kind of final patch, just a test.)

Not really, since the error can happen even while a new battery is being added,
which can happen anytime (even during battery hook removal). This means that using
a global variable would be prone to race conditions.

The issue can only be solved inside the ACPI battery driver itself, that is the reason
why i CCed the ACPI maintainers.

Aside from that, ignoring unsupported batteries allows the driver to work on
machines with multiple batteries.

Thanks,
Armin Wolf

>
> ---
>   drivers/platform/x86/dell/dell-laptop.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
> index a9de19799f01..c7b92b2f7ed2 100644
> --- a/drivers/platform/x86/dell/dell-laptop.c
> +++ b/drivers/platform/x86/dell/dell-laptop.c
> @@ -2390,21 +2390,29 @@ static struct attribute *dell_battery_attrs[] = {
>   	NULL,
>   };
>   ATTRIBUTE_GROUPS(dell_battery);
> +static bool bgroup_registered = false;
>
>   static int dell_battery_add(struct power_supply *battery,
>   		struct acpi_battery_hook *hook)
>   {
> +	int err;
> +
>   	/* this currently only supports the primary battery */
>   	if (strcmp(battery->desc->name, "BAT0") != 0)
>   		return -ENODEV;
>
> -	return device_add_groups(&battery->dev, dell_battery_groups);
> +	err = device_add_groups(&battery->dev, dell_battery_groups);
> +	if (!err)
> +		bgroup_registered = true;
> +
> +	return err;
>   }
>
>   static int dell_battery_remove(struct power_supply *battery,
>   		struct acpi_battery_hook *hook)
>   {
> -	device_remove_groups(&battery->dev, dell_battery_groups);
> +	if (bgroup_registered)
> +		device_remove_groups(&battery->dev, dell_battery_groups);
>   	return 0;
>   }
>