Re: [PATCH] power: supply: avoid nullptr deref in __power_supply_is_system_supplied

Re: [PATCH] power: supply: avoid nullptr deref in __power_supply_is_system_supplied

[Rafael J. Wysocki]

CC: linux-pm (where the power supply subsystem is developed)

On Mon, Aug 29, 2022 at 5:28 PM Jason A. Donenfeld <Jason@zx2c4.com> 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
> Cc: Rafael J. Wysocki <rafael@kernel.org>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

Acked-by: Rafael J. Wysocki <rafael@kernel.org>

> ---
>  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;
> --
> 2.37.2
>

[PATCH RESEND] power: supply: avoid nullptr deref in __power_supply_is_system_supplied

[Jason A. Donenfeld]

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;
-- 
2.37.3

Re: [PATCH RESEND] power: supply: avoid nullptr deref in __power_supply_is_system_supplied

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 6049 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RESEND] power: supply: avoid nullptr deref in __power_supply_is_system_supplied

[Jason A. Donenfeld]

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

Re: [PATCH RESEND] power: supply: avoid nullptr deref in __power_supply_is_system_supplied

[Jason A. Donenfeld]

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.

Re: [PATCH RESEND] power: supply: avoid nullptr deref in __power_supply_is_system_supplied

[Jason A. Donenfeld]

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

Re: [PATCH RESEND] power: supply: avoid nullptr deref in __power_supply_is_system_supplied

[Mark Pearson]

<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

Re: [PATCH RESEND] power: supply: avoid nullptr deref in __power_supply_is_system_supplied

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2947 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]