linux-kernel - [PATCH] HID: input: return ENODATA if reading battery attrs fails

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

Date:	Mon, 13 May 2013 17:01:30 +0200
From:	David Herrmann <dh.herrmann@...il.com>
To:	linux-input@...r.kernel.org
Cc:	linux-kernel@...r.kernel.org,
	David Herrmann <dh.herrmann@...il.com>,
	Jiri Kosina <jkosina@...e.cz>, Anton Vorontsov <cbou@...l.ru>,
	David Woodhouse <dwmw2@...radead.org>
Subject: [PATCH] HID: input: return ENODATA if reading battery attrs fails

power_supply core has the bad habit of calling our battery callbacks
from within power_supply_register(). Furthermore, if the callbacks
fail with an unhandled error code, it will skip any uevent that it
might currently process.
So if HID-core registers battery devices, an "add" uevent is generated
and the battery callbacks are called. These will gracefully fail due
to timeouts as they might still hold locks on event processing. One
could argue that this should be fixed in power_supply core, but the
least we can do is to signal ENODATA so power_supply core will just
skip the property and continue with the uevent.

This fixes a bug where "add" and "remove" uevents are skipped for
battery devices. upower is unable to track these devices and currently
needs to ignore them.

This patch also overwrites any other error code. I cannot see any reason
why we should forward protocol- or I/O-errors to the power_supply core.
We handle these errors in hid_ll_driver later, anyway, so just skip
them. power_supply core cannot do anything useful with them, anyway,
and we avoid skipping important uevents and confusing user-space.

Thanks a lot to Daniel Nicoletti for pushing and investigating
on this.

Cc: Jiri Kosina <jkosina@...e.cz>
Cc: Anton Vorontsov <cbou@...l.ru>
Cc: David Woodhouse <dwmw2@...radead.org>
Reported-by: Daniel Nicoletti <dantti12@...il.com>
Signed-off-by: David Herrmann <dh.herrmann@...il.com>
---
Hi

I really dislike the way power_supply core calls into the drivers during the
"add" uevent. If a driver holds an I/O mutex (or anything else), it might
even deadlock in a very non-obvious way. Is there a reason why we need to
pass _all_ battery properties along "add" and "remove" uevents? Isn't it
enough to pass them with "change" uevents? This would guarantee that the
power_supply callbacks are only called from user-context and "change" events.

Anyway, I'd still like to see this patch applied so we have this annoying
bug fixed. I'd be willing to change the power_supply core, too, if one of the
maintainers agrees on the design (David? Anton?).

And, @Daniel, can you check whether this actually fixes the bug? I don't own
a device that reports battery-information, but it at least fixes the same bug
for the hid-wiimote driver (tested by me).

Cheers
David

 drivers/hid/hid-input.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 945b815..c526a3c 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -354,10 +354,10 @@ static int hidinput_get_battery_property(struct power_supply *psy,
 					      dev->battery_report_type);

 		if (ret != 2) {
-			if (ret >= 0)
-				ret = -EINVAL;
+			ret = -ENODATA;
 			break;
 		}
+		ret = 0;

 		if (dev->battery_min < dev->battery_max &&
 		    buf[1] >= dev->battery_min &&
-- 
1.8.2.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/