linux-kernel - Re: [PATCH] arm64: defconfig: Enable cros-ec and battery driver

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: <5745D2DD.6080300@nvidia.com>
Date:	Wed, 25 May 2016 17:29:17 +0100
From:	Jon Hunter <jonathanh@...dia.com>
To:	Rhyland Klein <rklein@...dia.com>,
	Thierry Reding <treding@...dia.com>,
	Krzysztof Kozlowski <k.kozlowski@...sung.com>
CC:	Stephen Warren <swarren@...dotorg.org>,
	Alexandre Courbot <gnurou@...il.com>,
	<linux-kernel@...r.kernel.org>, <linux-tegra@...r.kernel.org>
Subject: Re: [PATCH] arm64: defconfig: Enable cros-ec and battery driver


On 25/05/16 17:10, Jon Hunter wrote:

...

> So power_supply_read_temp() calls ->get_property() and passes the
> power_supply psy struct which is initialised. The problem is that inside
> the bq27xxx driver, this then kicks off the worker thread to update the
> bq27xxx state and when this worker thread runs it attempts to access the
> same psy struct but by dereferencing a pointer to it from the
> bq27xxx_device_info where the pointer has not been initialised yet.
> Therefore, IMO it seems that we should not allow this worker thread to
> start until the registration has completed and hence the pointer is
> initialised.

Sorry, it is not the actual worker thread that triggers the NULL pointer
deference, but the function bq27xxx_battery_poll() that schedules the
worker thread. Anyway, I still don't see that we need to update the
bq27xxx state during the registration especially seeing as we call
bq27xxx_battery_update() after the registration is complete. It seems
that updating the overall state should be mutually exclusive from
reading the temp.

Looking at my patch, it does appear that the worker thread which also
calls bq27xxx_battery_update() is still scheduled and so may be it
should be ...

diff --git a/drivers/power/bq27xxx_battery.c b/drivers/power/bq27xxx_battery.c
index 45f6ebf88df6..1334ed522332 100644
--- a/drivers/power/bq27xxx_battery.c
+++ b/drivers/power/bq27xxx_battery.c
@@ -733,6 +733,9 @@ static void bq27xxx_battery_poll(struct work_struct *work)
                        container_of(work, struct bq27xxx_device_info,
                                     work.work);
 
+       if (!di->bat)
+               return;
+
        bq27xxx_battery_update(di);
 
        if (poll_interval > 0) {


Cheers
Jon

-- 
nvpublic