Re: [PATCH] usb: typec: ucsi: Fix NULL pointer dereference in power level debugfs

Re: [PATCH] usb: typec: ucsi: Fix NULL pointer dereference in power level debugfs

[sashiko-bot]

> ucsi_register() creates the debugfs entries right away, but
> ucsi->connector is only set by ucsi_init(), which runs later from
> ucsi->work and only publishes the connectors once the PPM has been
> reset and all ports are registered. Until then, or for good if init
> fails, ucsi->connector is NULL, yet the peak_current, avg_current and
> vbus_voltage show callbacks dereference it unconditionally.
> 
> On an Intel NUC13ANKi5 whose PPM never answers when CONFIG_ACPI_EC=n,
> reading those files after the failed init oopses:
> 
>   ucsi_acpi USBC000:00: error -ETIMEDOUT: PPM init failed
>   BUG: kernel NULL pointer dereference, address: 000000000000045c
>   #PF: supervisor read access in kernel mode
>   RIP: 0010:ucsi_debugfs_register+0x291/0x420 [typec_ucsi]
>   Call Trace:
>    <TASK>
>    seq_read_iter+0x1e4/0x4c0
>    seq_read+0x12b/0x160
>    full_proxy_read+0x9a/0x1f0
>    vfs_read+0xba/0x320
>    ksys_read+0x74/0x110
>    do_syscall_64+0x79/0x240
>    entry_SYSCALL_64_after_hwframe+0x71/0x79
> 
> Return -ENODEV from those callbacks while no connector is available.
> 
> Fixes: c851b71fd6cd ("usb: typec: ucsi: Add support for READ_POWER_LEVEL command")
> Cc: stable@vger.kernel.org
> Signed-off-by: Bertrand Jacquin <bertrand@jacquin.bzh>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261010120357.4937-1-bertrand@jacquin.bzh?part=1

[PATCH] usb: typec: ucsi: Fix NULL pointer dereference in power level debugfs

[Bertrand Jacquin]

ucsi_register() creates the debugfs entries right away, but
ucsi->connector is only set by ucsi_init(), which runs later from
ucsi->work and only publishes the connectors once the PPM has been
reset and all ports are registered. Until then, or for good if init
fails, ucsi->connector is NULL, yet the peak_current, avg_current and
vbus_voltage show callbacks dereference it unconditionally.

On an Intel NUC13ANKi5 whose PPM never answers when CONFIG_ACPI_EC=n,
reading those files after the failed init oopses:

  ucsi_acpi USBC000:00: error -ETIMEDOUT: PPM init failed
  BUG: kernel NULL pointer dereference, address: 000000000000045c
  #PF: supervisor read access in kernel mode
  RIP: 0010:ucsi_debugfs_register+0x291/0x420 [typec_ucsi]
  Call Trace:
   <TASK>
   seq_read_iter+0x1e4/0x4c0
   seq_read+0x12b/0x160
   full_proxy_read+0x9a/0x1f0
   vfs_read+0xba/0x320
   ksys_read+0x74/0x110
   do_syscall_64+0x79/0x240
   entry_SYSCALL_64_after_hwframe+0x71/0x79

Return -ENODEV from those callbacks while no connector is available.

Fixes: c851b71fd6cd ("usb: typec: ucsi: Add support for READ_POWER_LEVEL command")
Cc: stable@vger.kernel.org
Signed-off-by: Bertrand Jacquin <bertrand@jacquin.bzh>
---
 drivers/usb/typec/ucsi/debugfs.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/usb/typec/ucsi/debugfs.c b/drivers/usb/typec/ucsi/debugfs.c
index 77a0dd75edd3..911a151ac16a 100644
--- a/drivers/usb/typec/ucsi/debugfs.c
+++ b/drivers/usb/typec/ucsi/debugfs.c
@@ -92,6 +92,9 @@ static int ucsi_peak_curr_show(struct seq_file *m, void *v)
 {
 	struct ucsi *ucsi = m->private;
 
+	if (!ucsi->connector)
+		return -ENODEV;
+
 	seq_printf(m, "%u mA\n", ucsi->connector->peak_current);
 	return 0;
 }
@@ -101,6 +104,9 @@ static int ucsi_avg_curr_show(struct seq_file *m, void *v)
 {
 	struct ucsi *ucsi = m->private;
 
+	if (!ucsi->connector)
+		return -ENODEV;
+
 	seq_printf(m, "%u mA\n", ucsi->connector->avg_current);
 	return 0;
 }
@@ -110,6 +116,9 @@ static int ucsi_vbus_volt_show(struct seq_file *m, void *v)
 {
 	struct ucsi *ucsi = m->private;
 
+	if (!ucsi->connector)
+		return -ENODEV;
+
 	seq_printf(m, "%u mV\n", ucsi->connector->vbus_voltage);
 	return 0;
 }

Re: [PATCH] usb: typec: ucsi: Fix NULL pointer dereference in power level debugfs

[Huang Wei]

Hi Bertrand,

I checked this over. The window is real - ucsi_register() creates the
debugfs entries right away while ucsi->connector is only assigned at
the end of ucsi_init(), so on the PPM-init-failed path those three
files stay reachable with a NULL connector.

I also grepped debugfs.c for other ucsi->connector dereferences - the
three show functions the patch guards are the only ones (ucsi_resp
and the command interface go through ucsi->debugfs), so the fix is
complete.

One thing I'd note in favour of guarding rather than moving
ucsi_debugfs_register() after connector assignment: the command and
response files are exactly what you want available when the PPM is
misbehaving, so keeping the early registration and just making the
power-level reads fail cleanly looks like the better shape to me.

Reviewed-by: Huang Wei <huangwei@kylinos.cn>

Thanks,
Huang Wei