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

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240919031329.3e0f76d0@5400>
Date: Thu, 19 Sep 2024 03:13:29 -0400
From: Andres Salomon <dilinger@...ued.net>
To: Armin Wolf <W_Armin@....de>
Cc: mjg59@...f.ucam.org, pali@...nel.org, rafael@...nel.org,
 lenb@...nel.org, hdegoede@...hat.com, ilpo.jarvinen@...ux.intel.com,
 platform-driver-x86@...r.kernel.org, linux-acpi@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] platform/x86: dell-laptop: Fix crash when unregistering
 battery hook

On Thu, 19 Sep 2024 08:33:32 +0200
Armin Wolf <W_Armin@....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@....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.