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
| ||
|
Date: Mon, 18 May 2015 22:32:50 +0900 From: Krzysztof Kozlowski <k.kozlowski@...sung.com> To: Krzysztof Kozlowski <k.kozlowski@...sung.com> Cc: "Dr. H. Nikolaus Schaller" <hns@...delico.com>, Rodolfo Giometti <giometti@...ux.it>, Sebastian Reichel <sre@...nel.org>, LKML <linux-kernel@...r.kernel.org>, Marek Belisko <marek@...delico.com>, Linux PM mailing list <linux-pm@...r.kernel.org>, Dmitry Eremin-Solenikov <dbaryshkov@...il.com>, David Woodhouse <dwmw2@...radead.org>, List for communicating with real GTA04 owners <gta04-owner@...delico.com> Subject: Re: Bug: power supply - NULL pointer dereference in bq27x00 driver 2015-05-18 21:32 GMT+09:00 Krzysztof Kozlowski <k.kozlowski@...sung.com>: > 2015-05-18 18:40 GMT+09:00 Dr. H. Nikolaus Schaller <hns@...delico.com>: >> Hi, >> we tried to upgrade from 4.0 to 4.1-rc3 kernel and ran into a NULL pointer problem within the bq27x00 driver. >> >> It appears to be introduced by your patch 297d716f6260cc9421d971b124ca196b957ee458 >> >> The problem appears to be that bq27x00_powersupply_init() calls power_supply_register_no_ws() and >> sets di->bat *after* return. The old code did pass an uninitialized struct pointer. >> >> Now for reasons I don’t understand, the power_supply_register_no_ws() appears to call >> uevent related stuff which in turn calls bq27x00_battery_get_property() before di->bat >> is properly initialized. >> >> I have checked with printk in bq27x00_battery_get_property() that di>bat == NULL in this case and >> right before we see the segfault. >> >> The old code simply did pass a zeroed struct power_supply and perhaps initialized its components >> during registration. >> >> Returning some -EINVAL if di->bat == NULL would likely solve the NULL pointer dereference but >> I don’t know what it does to the uevent and if it restores previous operation. >> >> It could have been that it was for good purpose that power_supply_register_no_ws() did not return >> by value, but by reference to the di->bat struct: >> >> - ret = power_supply_register_no_ws(di->dev, &di->bat, NULL); >> + di->bat = power_supply_register_no_ws(di->dev, psy_desc, &psy_cfg); >> >> So that code called within the context of power_supply_register_no_ws() could already >> refer to initialized di->bat. > > Indeed this issue was not present in previous design however I think > the architecture was still error-prone. Starting from driver's probe: > - some_driver_probe() > - power_supply_register(&psy) > - device_add() > - kobject_uevent() - notify userspace > - power_supply_uevent() > - power_supply_show_property() > - power_supply_get_property() > - some_driver_get_property() > > So before probe() ends the power supply core calls driver's > get_property(). In that time the driver internal structures may not be > ready yet, because the probe did not end. The get_property() could > access some registers or other core functions which will be ready > after power_supply_register() call. For example the > bq27x00_battery_get_property() accesses delayed work which could be > (by mistake) not yet initialized. > > I looked at other dev_uevent implementations and almost all of them do > not call back the driver. > > Of course this does not change that I introduced the issue and I feel > bad about it. > I got some ideas for resolving it: > 1. Fix bq27x00_battery and maybe others so they won't access the 'psy' > member of its state container. This does not solve the issue from > architectural point of view - still some uninitialised data may be > accessed because probe() is in progress. However this would be the > fastest and the least intrusive. > > 2. Remove calls to get_property() (and other functions provided by > driver) from power_supply_uevent(). Unfortunately this may break > userspace which expects that some data will be present in uevent. > > 3. Change the API to the previous convention, which you pointed as a remedy: > ret = power_supply_register_no_ws(di->dev, &di->bat, psy_desc, &psy_cfg); > This also won't solve the issue from 1. that uevent will be called > before probe ends. > > 4. Block the power_supply_uevent() from calling the get_property() > functions until device_add() finishes. This would solve this issue but > first uevent messages (from adding device) won't contain all of device > data (just like in solution no 2.) and this won't solve other race - > someone may call sysfs show() on created device nodes and thus access > the get_property() before probe finishes. Unfortunately similar issue is with changed_work of power supply. The power_supply_register() calls at the end power_supply_changed_work() which through leds calls get_property(). Best regards, Krzysztof -- 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/
Powered by blists - more mailing lists