| Message ID | 20220905172428.105564-1-Jason@zx2c4.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | [RESEND] power: supply: avoid nullptr deref in __power_supply_is_system_supplied | expand |
Hi, On Mon, Sep 05, 2022 at 07:24:28PM +0200, Jason A. Donenfeld wrote: > Fix the following OOPS: > > BUG: kernel NULL pointer dereference, address: 0000000000000000 > PGD 0 P4D 0 > Oops: 0010 [#1] PREEMPT SMP > CPU: 14 PID: 1156 Comm: upowerd Tainted: G S U 6.0.0-rc1+ #366 > Hardware name: LENOVO 20Y5CTO1WW/20Y5CTO1WW, BIOS N40ET36W (1.18 ) 07/19/2022 > RIP: 0010:0x0 > Code: Unable to access opcode bytes at RIP 0xffffffffffffffd6. > RSP: 0018:ffff88815350bd08 EFLAGS: 00010212 > RAX: ffff88810207d620 RBX: ffff88815350bd7c RCX: 000000000000394e > RDX: ffff88815350bd10 RSI: 0000000000000004 RDI: ffff888111722c00 > RBP: ffff88815350bd68 R08: ffff8881187a8af8 R09: ffff8881187a8af8 > R10: 0000000000000000 R11: 000000000000005f R12: ffffffff8162d0b0 > R13: ffff88810159a038 R14: ffffffff823b3768 R15: ffff88810159a000 > FS: 00007fd1f0958140(0000) GS:ffff88901f780000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: ffffffffffffffd6 CR3: 0000000152c7a004 CR4: 0000000000770ee0 > PKRU: 55555554 > Call Trace: > <TASK> > __power_supply_is_system_supplied+0x26/0x40 > class_for_each_device+0xa5/0xd0 > ? acpi_battery_get_state+0x4e/0x1f0 > power_supply_is_system_supplied+0x26/0x40 > acpi_battery_get_property+0x301/0x310 > power_supply_show_property+0xa5/0x1d0 > dev_attr_show+0x10/0x30 > sysfs_kf_seq_show+0x78/0xc0 > seq_read_iter+0xfd/0x3e0 > vfs_read+0x1cb/0x290 > ksys_read+0x4e/0xc0 > do_syscall_64+0x2b/0x50 > entry_SYSCALL_64_after_hwframe+0x46/0xb0 > RIP: 0033:0x7fd1f0bed70c > Code: ec 28 48 89 54 24 18 48 89 74 24 10 89 7c 24 08 e8 39 a4 f8 ff 41 89 c0 48 8b 54 24 18 48 8b 74 24 10 8b 7c 24 08 31 c0 0f 05 <48> 3d 00 f0 ff ff 77 34 44 89 c7 48 89 44 24 08 e8 8f a4 f8 ff 48 > RSP: 002b:00007ffc8d3f27e0 EFLAGS: 00000246 ORIG_RAX: 0000000000000000 > RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007fd1f0bed70c > RDX: 0000000000001000 RSI: 000055957d534850 RDI: 000000000000000c > RBP: 000055957d50b1d0 R08: 0000000000000000 R09: 0000000000001000 > R10: 000000000000006f R11: 0000000000000246 R12: 00007ffc8d3f2910 > R13: 0000000000000000 R14: 0000000000000000 R15: 000000000000000c > </TASK> > > CR2: 0000000000000000 > ---[ end trace 0000000000000000 ]--- > RIP: 0010:0x0 > Code: Unable to access opcode bytes at RIP 0xffffffffffffffd6. > RSP: 0018:ffff88815350bd08 EFLAGS: 00010212 > RAX: ffff88810207d620 RBX: ffff88815350bd7c RCX: 000000000000394e > RDX: ffff88815350bd10 RSI: 0000000000000004 RDI: ffff888111722c00 > RBP: ffff88815350bd68 R08: ffff8881187a8af8 R09: ffff8881187a8af8 > R10: 0000000000000000 R11: 000000000000005f R12: ffffffff8162d0b0 > R13: ffff88810159a038 R14: ffffffff823b3768 R15: ffff88810159a000 > FS: 00007fd1f0958140(0000) GS:ffff88901f780000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: ffffffffffffffd6 CR3: 0000000152c7a004 CR4: 0000000000770ee0 > > The disassembly of the top function in the stack trace is: > > .text:0000000000000000 __power_supply_is_system_supplied proc near > .text:0000000000000000 ; DATA XREF: power_supply_is_system_supplied+12↓o > .text:0000000000000000 > .text:0000000000000000 var_8 = qword ptr -8 > .text:0000000000000000 > .text:0000000000000000 sub rsp, 8 > .text:0000000000000004 mov rdi, [rdi+78h] > .text:0000000000000008 inc dword ptr [rsi] > .text:000000000000000A mov [rsp+8+var_8], 0 > .text:0000000000000012 mov rax, [rdi] > .text:0000000000000015 cmp dword ptr [rax+8], 1 > .text:0000000000000019 jz short loc_2A > .text:000000000000001B mov rdx, rsp > .text:000000000000001E mov esi, 4 > .text:0000000000000023 call qword ptr [rax+30h] > .text:0000000000000026 test eax, eax > .text:0000000000000028 jz short loc_31 > .text:000000000000002A > .text:000000000000002A loc_2A: ; CODE XREF: __power_supply_is_system_supplied+19↑j > .text:000000000000002A xor eax, eax > .text:000000000000002C add rsp, 8 > .text:0000000000000030 retn > .text:0000000000000031 ; --------------------------------------------------------------------------- > .text:0000000000000031 > .text:0000000000000031 loc_31: ; CODE XREF: __power_supply_is_system_supplied+28↑j > .text:0000000000000031 mov eax, dword ptr [rsp+8+var_8] > .text:0000000000000034 add rsp, 8 > .text:0000000000000038 retn > .text:0000000000000038 __power_supply_is_system_supplied endp > > So presumably `call qword ptr [rax+30h]` is jumping to NULL. > > Cc: stable@vger.kernel.org > Acked-by: Rafael J. Wysocki <rafael@kernel.org> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> > --- > drivers/power/supply/power_supply_core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c > index 4b5fb172fa99..aa4c97f11588 100644 > --- a/drivers/power/supply/power_supply_core.c > +++ b/drivers/power/supply/power_supply_core.c > @@ -349,7 +349,7 @@ static int __power_supply_is_system_supplied(struct device *dev, void *data) > unsigned int *count = data; > > (*count)++; > - if (psy->desc->type != POWER_SUPPLY_TYPE_BATTERY) > + if (psy->desc->type != POWER_SUPPLY_TYPE_BATTERY && psy->desc->get_property) > if (!psy->desc->get_property(psy, POWER_SUPPLY_PROP_ONLINE, > &ret)) > return ret.intval; Thanks, queued into power-supply's fixes branch. But I'm curioous how you triggered this. Which power-supply driver does not add a get_property function? -- Sebastian
Hi Sebastian, On Sun, Sep 11, 2022 at 02:33:46PM +0200, Sebastian Reichel wrote: > > - if (psy->desc->type != POWER_SUPPLY_TYPE_BATTERY) > > + if (psy->desc->type != POWER_SUPPLY_TYPE_BATTERY && psy->desc->get_property) > > if (!psy->desc->get_property(psy, POWER_SUPPLY_PROP_ONLINE, > > &ret)) > > return ret.intval; > > Thanks, queued into power-supply's fixes branch. But I'm curioous > how you triggered this. Which power-supply driver does not add > a get_property function? AFAIK, I'm just using the normal ACPI one. Really nothing fancy. Thinkpad X1 Extreme Gen 4. Maybe get_property was being set and unset during some kind of initialization/deinitialization that was happening in response to some other event? Not sure, except that I managed to trigger it twice before patching my kernel so my laptop would keep working. My machine went through three changes I know about between the threshold of "not crashing" and "crashing": - Upgraded to 5.19 and then 6.0-rc1. - I used my laptop on batteries for a prolonged period of time for the first time in a while. - I updated KDE, whose power management UI elements may or may not make frequent calls to this subsystem to update some visual representation. I don't have any data to back up this claim at all, but something that at least _sounds_ plausible is that an updated KDE thing was hammering on this read() method, while a decreasing battery state caused some aspect of the subsystem to reinitialize the power management object, making ->get_property() temporarily NULL. But just a guess! Jason
Ah another thing: On Mon, Sep 12, 2022 at 11:45 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > My machine went through three changes I know about between the threshold > of "not crashing" and "crashing": > - Upgraded to 5.19 and then 6.0-rc1. > - I used my laptop on batteries for a prolonged period of time for the > first time in a while. > - I updated KDE, whose power management UI elements may or may not make > frequent calls to this subsystem to update some visual representation. - Updated my BIOS.
CC+ Mark Pearson from Lenovo Full thread is here: https://lore.kernel.org/all/YwDsy3ZUgTtlKH9r@zx2c4.com/ On Mon, Sep 12, 2022 at 11:48 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > Ah another thing: > > On Mon, Sep 12, 2022 at 11:45 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > My machine went through three changes I know about between the threshold > > of "not crashing" and "crashing": > > - Upgraded to 5.19 and then 6.0-rc1. > > - I used my laptop on batteries for a prolonged period of time for the > > first time in a while. > > - I updated KDE, whose power management UI elements may or may not make > > frequent calls to this subsystem to update some visual representation. > > - Updated my BIOS. GASP! The plot thickens. It appears that the BIOS update I applied has been removed from https://pcsupport.lenovo.com/fr/en/downloads/ds551052-bios-update-utility-bootable-cd-for-windows-10-64-bit-and-linux-thinkpad-p1-gen-4-x1-extreme-gen-4 and now it only shows the 1.16 version. I updated from 1.16 to 1.18. The missing release notes are still online if you futz with the URL: https://download.lenovo.com/pccbbs/mobiles/n40ur14w.txt https://download.lenovo.com/pccbbs/mobiles/n40ur15w.txt One of the items for 1.17 says: > - (Fix) Fixed an issue where it took a long time to update the battery FW. So maybe something was happening here... I'm CC'ing Mark from Lenovo to see if he has any insight as to why this BIOS update was pulled. Maybe the battery was appearing and disappearing rapidly. If that's correct, then it'd indicate that this bandaid patch is *wrong* and what actually is needed is some kind of reference counting or RCU around that sysfs interface (and maybe others). Jason
<Note - hope this works - moved to my more opensource friendly email account> On 2022-09-12 08:20, Mark Pearson wrote: > > -------------------------------------------------------------------------------- > *From:* Jason A. Donenfeld <Jason@zx2c4.com> > *Sent:* September 12, 2022 6:56 > *To:* Sebastian Reichel <sebastian.reichel@collabora.com>; Mark Pearson > <mpearson@lenovo.com> > *Cc:* linux-pm@vger.kernel.org <linux-pm@vger.kernel.org>; > stable@vger.kernel.org <stable@vger.kernel.org>; Rafael J . Wysocki > <rafael@kernel.org> > *Subject:* [External] Re: [PATCH RESEND] power: supply: avoid nullptr deref in > __power_supply_is_system_supplied > CC+ Mark Pearson from Lenovo > Full thread is here: > https://lore.kernel.org/all/YwDsy3ZUgTtlKH9r@zx2c4.com/ <https://lore.kernel.org/all/YwDsy3ZUgTtlKH9r@zx2c4.com/>> > On Mon, Sep 12, 2022 at 11:48 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote: >> >> Ah another thing: >> >> On Mon, Sep 12, 2022 at 11:45 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote: >> > My machine went through three changes I know about between the threshold >> > of "not crashing" and "crashing": >> > - Upgraded to 5.19 and then 6.0-rc1. >> > - I used my laptop on batteries for a prolonged period of time for the >> > first time in a while. >> > - I updated KDE, whose power management UI elements may or may not make >> > frequent calls to this subsystem to update some visual representation. >> >> - Updated my BIOS. > > GASP! The plot thickens. > > It appears that the BIOS update I applied has been removed from > https://pcsupport.lenovo.com/fr/en/downloads/ds551052-bios-update-utility-bootable-cd-for-windows-10-64-bit-and-linux-thinkpad-p1-gen-4-x1-extreme-gen-4 <https://pcsupport.lenovo.com/fr/en/downloads/ds551052-bios-update-utility-bootable-cd-for-windows-10-64-bit-and-linux-thinkpad-p1-gen-4-x1-extreme-gen-4> > and now it only shows the 1.16 version. I updated from 1.16 to 1.18. > > The missing release notes are still online if you futz with the URL: > https://download.lenovo.com/pccbbs/mobiles/n40ur14w.txt > <https://download.lenovo.com/pccbbs/mobiles/n40ur14w.txt> > https://download.lenovo.com/pccbbs/mobiles/n40ur15w.txt > <https://download.lenovo.com/pccbbs/mobiles/n40ur15w.txt> > > One of the items for 1.17 says: >> - (Fix) Fixed an issue where it took a long time to update the battery FW. > > So maybe something was happening here... > > I'm CC'ing Mark from Lenovo to see if he has any insight as to why > this BIOS update was pulled. > > Maybe the battery was appearing and disappearing rapidly. If that's > correct, then it'd indicate that this bandaid patch is *wrong* and > what actually is needed is some kind of reference counting or RCU > around that sysfs interface (and maybe others). > > Jason Hi Jason, I'll have to check with the FW team but looking at the internal notes I think the FW was pulled because of a graphics display regression. Version 36W was fixing a brightness control issue in discrete mode and 37W (not yet released) is fixing external display - so my guess is something about the fix in 36W has a side effect More interesting is the EC FW updates. There isn't a new version posted but there are fixes in the previous version (EC 33W) for a fix for a 'suspected EC-Battery communication transaction failure'. Is that potentially related to this patch in some way? I can go and ask for more details if we think it's related. I'll also see if I can repro on my P1G4 - but I hadn't seen any other reports so it might be HW specific. Can you confirm which FW you have from the BIOS setup screen (F1 during early boot)? BIOS and EC please. Mark
Hi, On Mon, Sep 12, 2022 at 11:56:43AM +0100, Jason A. Donenfeld wrote: > AFAIK, I'm just using the normal ACPI one. Really nothing fancy. > Thinkpad X1 Extreme Gen 4. All ACPI drivers setup get_property method in their power-supply devices. > Maybe get_property was being set and unset during some kind of > initialization/deinitialization that was happening in response to some > other event? Not sure, except that I managed to trigger it twice before > patching my kernel so my laptop would keep working. The function is not intended to be changed during the lifetime of the device and AFAIK no mainline drivers does this. > On Mon, Sep 12, 2022 at 11:48 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > On Mon, Sep 12, 2022 at 11:45 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > > My machine went through three changes I know about between the threshold > > > of "not crashing" and "crashing": > > > - Upgraded to 5.19 and then 6.0-rc1. > > > - I used my laptop on batteries for a prolonged period of time for the > > > first time in a while. > > > - I updated KDE, whose power management UI elements may or may not make > > > frequent calls to this subsystem to update some visual representation. > > > > - Updated my BIOS. > > GASP! The plot thickens. > > It appears that the BIOS update I applied has been removed from > https://pcsupport.lenovo.com/fr/en/downloads/ds551052-bios-update-utility-bootable-cd-for-windows-10-64-bit-and-linux-thinkpad-p1-gen-4-x1-extreme-gen-4 > and now it only shows the 1.16 version. I updated from 1.16 to 1.18. > > The missing release notes are still online if you futz with the URL: > https://download.lenovo.com/pccbbs/mobiles/n40ur14w.txt > https://download.lenovo.com/pccbbs/mobiles/n40ur15w.txt > > One of the items for 1.17 says: > > - (Fix) Fixed an issue where it took a long time to update the battery FW. > > So maybe something was happening here... > > I'm CC'ing Mark from Lenovo to see if he has any insight as to why > this BIOS update was pulled. > > Maybe the battery was appearing and disappearing rapidly. > > If that's correct, then it'd indicate that this bandaid patch is > *wrong* and what actually is needed is some kind of reference > counting or RCU around that sysfs interface (and maybe others). Device create/remove is the only time that is supposed to touch the get_property callback. So I suppose a race condition in that path would be a sensible root cause. Considering systems usually registers the device once and keeps it until shutdown would also explain why this has not been noticed earlier. The function you modified is only called by power_supply_is_system_supplied(), which is an in-kernel function to figure out if the system is running on battery. Can you trigger this easy enough to figure out a few more details about the state of the problematic device? -- Sebastian
diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c index 4b5fb172fa99..aa4c97f11588 100644 --- a/drivers/power/supply/power_supply_core.c +++ b/drivers/power/supply/power_supply_core.c @@ -349,7 +349,7 @@ static int __power_supply_is_system_supplied(struct device *dev, void *data) unsigned int *count = data; (*count)++; - if (psy->desc->type != POWER_SUPPLY_TYPE_BATTERY) + if (psy->desc->type != POWER_SUPPLY_TYPE_BATTERY && psy->desc->get_property) if (!psy->desc->get_property(psy, POWER_SUPPLY_PROP_ONLINE, &ret)) return ret.intval;