| Message ID | alpine.DEB.2.21.2008271131570.37762@montezuma.home |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4,1/2] usb/typec: fix array overruns in ucsi.c partner_altmode[] | expand |
Hi Zwane, Sorry to keep you waiting. I'm trying to find a board I can use to test these... On Sun, Aug 30, 2020 at 02:26:36AM -0700, Zwane Mwaikambo wrote: > con->partner_altmode[i] ends up with the value 0x2 in the call to > typec_altmode_update_active because the array has been accessed out of > bounds causing a random memory read. > > This patch fixes the first occurrence and 2/2 the second. That, when read from the final commit log, does not actually tell you anything. > [ 565.452023] BUG: kernel NULL pointer dereference, address: 00000000000002fe > [ 565.452025] #PF: supervisor read access in kernel mode > [ 565.452026] #PF: error_code(0x0000) - not-present page > [ 565.452026] PGD 0 P4D 0 > [ 565.452028] Oops: 0000 [#1] PREEMPT SMP NOPTI > [ 565.452030] CPU: 0 PID: 502 Comm: kworker/0:3 Not tainted 5.8.0-rc3+ #1 > [ 565.452031] Hardware name: LENOVO 20RD002VUS/20RD002VUS, > BIOS R16ET25W (1.11 ) 04/21/2020 > [ 565.452034] Workqueue: events ucsi_handle_connector_change [typec_ucsi] > [ 565.452039] RIP: 0010:typec_altmode_update_active+0x1f/0x100 [typec] > [ 565.452040] Code: 0f 1f 84 00 00 00 00 00 0f 1f 00 0f 1f 44 00 00 55 48 > 89 e5 41 54 53 48 83 ec 10 65 48 8b 04 25 28 00 00 00 48 89 45 e8 31 c0 > <0f> b6 87 fc 02 00 00 83 e0 01 40 38 f0 0f 84 95 00 00 00 48 8b 47 > [ 565.452041] RSP: 0018:ffffb729c066bdb0 EFLAGS: 00010246 > [ 565.452042] RAX: 0000000000000000 RBX: ffffa067c3e64a70 RCX: 0000000000000000 > [ 565.452043] RDX: ffffb729c066bd20 RSI: 0000000000000000 RDI: 0000000000000002 > [ 565.452044] RBP: ffffb729c066bdd0 R08: 00000083a7910a4f R09: 0000000000000000 > [ 565.452044] R10: ffffffffa106a220 R11: 0000000000000000 R12: 0000000000000000 > [ 565.452045] R13: ffffa067c3e64a98 R14: ffffa067c3e64810 R15: ffffa067c3e64800 > [ 565.452046] FS: 0000000000000000(0000) GS:ffffa067d1400000(0000) > knlGS:0000000000000000 > [ 565.452047] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > [ 565.452048] CR2: 00000000000002fe CR3: 00000001b060a003 CR4: 00000000003606f0 > [ 565.452048] Call Trace: > [ 565.452052] ucsi_altmode_update_active+0x83/0xd0 [typec_ucsi] > [ 565.452054] ucsi_handle_connector_change+0x1dc/0x320 [typec_ucsi] > [ 565.452057] process_one_work+0x1df/0x3d0 > [ 565.452059] worker_thread+0x4d/0x3d0 > [ 565.452060] ? process_one_work+0x3d0/0x3d0 > [ 565.452062] kthread+0x127/0x170 > [ 565.452063] ? kthread_park+0x90/0x90 > [ 565.452065] ret_from_fork+0x1f/0x30 > > The failing instruction is; > > 0x0000000000000380 <+0>: callq 0x385 <typec_altmode_update_active+5> > 0x0000000000000385 <+5>: push %rbp > 0x0000000000000386 <+6>: mov %rsp,%rbp > 0x0000000000000389 <+9>: push %r12 > 0x000000000000038b <+11>: push %rbx > 0x000000000000038c <+12>: sub $0x10,%rsp > 0x0000000000000390 <+16>: mov %gs:0x28,%rax > 0x0000000000000399 <+25>: mov %rax,-0x18(%rbp) > 0x000000000000039d <+29>: xor %eax,%eax > 0x000000000000039f <+31>: movzbl 0x2fc(%rdi),%eax <====== > 0x00000000000003a6 <+38>: and $0x1,%eax > > (gdb) list *typec_altmode_update_active+0x1f > 0x39f is in typec_altmode_update_active (drivers/usb/typec/class.c:221). > 216 */ > 217 void typec_altmode_update_active(struct typec_altmode *adev, bool active) > 218 { > 219 char dir[6]; > 220 > 221 if (adev->active == active) > 222 return; > 223 > 224 if (!is_typec_port(adev->dev.parent) && adev->dev.driver) { > 225 if (!active) > > (gdb) list *ucsi_altmode_update_active+0x83 > 0x12a3 is in ucsi_altmode_update_active (drivers/usb/typec/ucsi/ucsi.c:221). > 216 } > 217 > 218 if (cur < UCSI_MAX_ALTMODES) > 219 altmode = typec_altmode_get_partner(con->port_altmode[cur]); > 220 > 221 for (i = 0; con->partner_altmode[i]; i++) > 222 typec_altmode_update_active(con->partner_altmode[i], > 223 con->partner_altmode[i] == altmode); > 224 } > > Fixes: ad74b8649beaf ("usb: typec: ucsi: Preliminary support for alternate modes") > Cc: stable@vger.kernel.org > Signed-off-by: Zwane Mwaikambo <zwane@yosper.io> > --- > v2 addresses both instances of array overrun > v3 addresses patch submission/formatting issues > v4 addresses patch submission/formatting issues > v5 adds and cc to stable > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c > index affd024190c9..16906519c173 100644 > --- a/drivers/usb/typec/ucsi/ucsi.c > +++ b/drivers/usb/typec/ucsi/ucsi.c > @@ -218,9 +218,10 @@ void ucsi_altmode_update_active(struct ucsi_connector *con) > if (cur < UCSI_MAX_ALTMODES) > altmode = typec_altmode_get_partner(con->port_altmode[cur]); > > - for (i = 0; con->partner_altmode[i]; i++) > - typec_altmode_update_active(con->partner_altmode[i], > - con->partner_altmode[i] == altmode); > + for (i = 0; i < UCSI_MAX_ALTMODES; i++) > + if (con->partner_altmode[i]) > + typec_altmode_update_active(con->partner_altmode[i], > + con->partner_altmode[i] == altmode); > } > > static u8 ucsi_altmode_next_mode(struct typec_altmode **alt, u16 svid) > thanks,
On Thu, Sep 03, 2020 at 02:10:50PM +0300, Heikki Krogerus wrote: > Hi Zwane, > > Sorry to keep you waiting. I'm trying to find a board I can use to > test these... I've now tested this (these) with ThinkPad X280, and there is no regression, however, now that (I think) I understand what's going on, I would not try to fix the issue like you do. I would simply make sure the alternate mode arrays are NULL terminated. For example with something like this: diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h index cba6f77bea61b..7e66e4d232996 100644 --- a/drivers/usb/typec/ucsi/ucsi.h +++ b/drivers/usb/typec/ucsi/ucsi.h @@ -317,8 +317,8 @@ struct ucsi_connector { struct typec_port *port; struct typec_partner *partner; - struct typec_altmode *port_altmode[UCSI_MAX_ALTMODES]; - struct typec_altmode *partner_altmode[UCSI_MAX_ALTMODES]; + struct typec_altmode *port_altmode[UCSI_MAX_ALTMODES + 1]; + struct typec_altmode *partner_altmode[UCSI_MAX_ALTMODES + 1]; struct typec_capability typec_cap; Though I'm not sure we should need even that. Try it out in any case. Even if that works, I have a feeling there is something odd going on. What kinds of device has all 30 modes supported (or even more)? I want to know if this is a case where the firmware is just reporting bogus values. What device are you plugging to the Type-C connector? Does it really have all 30 altmodes? What do you see in /sys/class/typec directory when you connect the device? ls /sys/class/typec Actually, do this: grep . /sys/class/typec/port*-partner/port*-partner.*/svid and tell what you get. thanks,
Hi Heikki, On Wed, 9 Sep 2020, Heikki Krogerus wrote: > On Thu, Sep 03, 2020 at 02:10:50PM +0300, Heikki Krogerus wrote: > > Hi Zwane, > > > > Sorry to keep you waiting. I'm trying to find a board I can use to > > test these... > > I've now tested this (these) with ThinkPad X280, and there is no > regression, however, now that (I think) I understand what's going on, > I would not try to fix the issue like you do. I would simply make sure > the alternate mode arrays are NULL terminated. For example with > something like this: > > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h > index cba6f77bea61b..7e66e4d232996 100644 > --- a/drivers/usb/typec/ucsi/ucsi.h > +++ b/drivers/usb/typec/ucsi/ucsi.h > @@ -317,8 +317,8 @@ struct ucsi_connector { > struct typec_port *port; > struct typec_partner *partner; > > - struct typec_altmode *port_altmode[UCSI_MAX_ALTMODES]; > - struct typec_altmode *partner_altmode[UCSI_MAX_ALTMODES]; > + struct typec_altmode *port_altmode[UCSI_MAX_ALTMODES + 1]; > + struct typec_altmode *partner_altmode[UCSI_MAX_ALTMODES + 1]; > > struct typec_capability typec_cap; > > Though I'm not sure we should need even that. Try it out in any case. > > Even if that works, I have a feeling there is something odd going on. > What kinds of device has all 30 modes supported (or even more)? I want > to know if this is a case where the firmware is just reporting bogus > values. > > What device are you plugging to the Type-C connector? Does it really > have all 30 altmodes? What do you see in /sys/class/typec directory > when you connect the device? > > ls /sys/class/typec > > Actually, do this: > > grep . /sys/class/typec/port*-partner/port*-partner.*/svid > > and tell what you get. Thanks for digging into it, the device being plugged in is a Lenovo TB dock (https://www.lenovo.com/ca/en/accessories-and-monitors/home-office/Thunderbolt-Dock-Gen-2-US/p/40AN0135US); thinkpad :: ~ » ls /sys/class/typec port0 thinkpad :: /sys/class/typec » grep . /sys/class/typec/port*-partner/port*-partner.*/svid zsh: no matches found: /sys/class/typec/port*-partner/port*-partner.*/svid thinkpad :: /sys/class/typec » find port0/ port0/ port0/uevent port0/vconn_source port0/supported_accessory_modes port0/power_role port0/data_role port0/preferred_role port0/firmware_node port0/power port0/power/runtime_active_time port0/power/runtime_active_kids port0/power/runtime_usage port0/power/runtime_status port0/power/autosuspend_delay_ms port0/power/async port0/power/runtime_suspended_time port0/power/runtime_enabled port0/power/control port0/power_operation_mode port0/usb_power_delivery_revision port0/device port0/subsystem port0/usb_typec_revision port0/port0.0 port0/port0.0/uevent port0/port0.0/svid port0/port0.0/vdo port0/port0.0/mode port0/port0.0/power port0/port0.0/power/runtime_active_time port0/port0.0/power/runtime_active_kids port0/port0.0/power/runtime_usage port0/port0.0/power/runtime_status port0/port0.0/power/autosuspend_delay_ms port0/port0.0/power/async port0/port0.0/power/runtime_suspended_time port0/port0.0/power/runtime_enabled port0/port0.0/power/control port0/port0.0/mode1 port0/port0.0/mode1/description port0/port0.0/mode1/vdo port0/port0.0/mode1/supported_roles port0/port0.0/mode1/active port0/port0.0/active
On Thu, Sep 10, 2020 at 12:52:05AM -0700, Zwane Mwaikambo wrote: > Hi Heikki, > > On Wed, 9 Sep 2020, Heikki Krogerus wrote: > > > On Thu, Sep 03, 2020 at 02:10:50PM +0300, Heikki Krogerus wrote: > > > Hi Zwane, > > > > > > Sorry to keep you waiting. I'm trying to find a board I can use to > > > test these... > > > > I've now tested this (these) with ThinkPad X280, and there is no > > regression, however, now that (I think) I understand what's going on, > > I would not try to fix the issue like you do. I would simply make sure > > the alternate mode arrays are NULL terminated. For example with > > something like this: > > > > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h > > index cba6f77bea61b..7e66e4d232996 100644 > > --- a/drivers/usb/typec/ucsi/ucsi.h > > +++ b/drivers/usb/typec/ucsi/ucsi.h > > @@ -317,8 +317,8 @@ struct ucsi_connector { > > struct typec_port *port; > > struct typec_partner *partner; > > > > - struct typec_altmode *port_altmode[UCSI_MAX_ALTMODES]; > > - struct typec_altmode *partner_altmode[UCSI_MAX_ALTMODES]; > > + struct typec_altmode *port_altmode[UCSI_MAX_ALTMODES + 1]; > > + struct typec_altmode *partner_altmode[UCSI_MAX_ALTMODES + 1]; > > > > struct typec_capability typec_cap; > > > > Though I'm not sure we should need even that. Try it out in any case. > > > > Even if that works, I have a feeling there is something odd going on. > > What kinds of device has all 30 modes supported (or even more)? I want > > to know if this is a case where the firmware is just reporting bogus > > values. > > > > What device are you plugging to the Type-C connector? Does it really > > have all 30 altmodes? What do you see in /sys/class/typec directory > > when you connect the device? > > > > ls /sys/class/typec > > > > Actually, do this: > > > > grep . /sys/class/typec/port*-partner/port*-partner.*/svid > > > > and tell what you get. > > Thanks for digging into it, the device being plugged in is a Lenovo TB > dock > (https://www.lenovo.com/ca/en/accessories-and-monitors/home-office/Thunderbolt-Dock-Gen-2-US/p/40AN0135US); It has TBT, DP, and probable the Lenovo vendor specific mode. So two or three modes, no more, so not 30. > thinkpad :: ~ » ls /sys/class/typec > port0 > > thinkpad :: /sys/class/typec » grep . /sys/class/typec/port*-partner/port*-partner.*/svid > zsh: no matches found: /sys/class/typec/port*-partner/port*-partner.*/svid How can you have partner change notifications without a partner? I'm probable still missing something. I wonder what exactly do you have in the partner alternate mode array? I think your patche(s) are working around some deeper issue. We really need to figure out what that is. Let's check the trace output. You need to build the UCSI drivers as modules. Then: modprobe -r ucsi_acpi modprobe typec_ucsi mount debugfs -t debugfs /sys/kernel/debug cd /sys/kernel/debug/tracing echo 1 > events/ucsi/enable modprobe ucsi_acpi Wait one minute and: cat trace thanks,
On Thu, 10 Sep 2020, Heikki Krogerus wrote: > On Thu, Sep 10, 2020 at 12:52:05AM -0700, Zwane Mwaikambo wrote: > > Hi Heikki, > > > > On Wed, 9 Sep 2020, Heikki Krogerus wrote: > > > > > On Thu, Sep 03, 2020 at 02:10:50PM +0300, Heikki Krogerus wrote: > > > > Hi Zwane, > > > > > > > > Sorry to keep you waiting. I'm trying to find a board I can use to > > > > test these... > > > > > > I've now tested this (these) with ThinkPad X280, and there is no > > > regression, however, now that (I think) I understand what's going on, > > > I would not try to fix the issue like you do. I would simply make sure > > > the alternate mode arrays are NULL terminated. For example with > > > something like this: > > > > > > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h > > > index cba6f77bea61b..7e66e4d232996 100644 > > > --- a/drivers/usb/typec/ucsi/ucsi.h > > > +++ b/drivers/usb/typec/ucsi/ucsi.h > > > @@ -317,8 +317,8 @@ struct ucsi_connector { > > > struct typec_port *port; > > > struct typec_partner *partner; > > > > > > - struct typec_altmode *port_altmode[UCSI_MAX_ALTMODES]; > > > - struct typec_altmode *partner_altmode[UCSI_MAX_ALTMODES]; > > > + struct typec_altmode *port_altmode[UCSI_MAX_ALTMODES + 1]; > > > + struct typec_altmode *partner_altmode[UCSI_MAX_ALTMODES + 1]; > > > > > > struct typec_capability typec_cap; > > > > > > Though I'm not sure we should need even that. Try it out in any case. > > > > > > Even if that works, I have a feeling there is something odd going on. > > > What kinds of device has all 30 modes supported (or even more)? I want > > > to know if this is a case where the firmware is just reporting bogus > > > values. > > > > > > What device are you plugging to the Type-C connector? Does it really > > > have all 30 altmodes? What do you see in /sys/class/typec directory > > > when you connect the device? > > > > > > ls /sys/class/typec > > > > > > Actually, do this: > > > > > > grep . /sys/class/typec/port*-partner/port*-partner.*/svid > > > > > > and tell what you get. > > > > Thanks for digging into it, the device being plugged in is a Lenovo TB > > dock > > (https://www.lenovo.com/ca/en/accessories-and-monitors/home-office/Thunderbolt-Dock-Gen-2-US/p/40AN0135US); > > It has TBT, DP, and probable the Lenovo vendor specific mode. So two > or three modes, no more, so not 30. > > > thinkpad :: ~ » ls /sys/class/typec > > port0 > > > > thinkpad :: /sys/class/typec » grep . /sys/class/typec/port*-partner/port*-partner.*/svid > > zsh: no matches found: /sys/class/typec/port*-partner/port*-partner.*/svid > > How can you have partner change notifications without a partner? I'm > probable still missing something. I wonder what exactly do you have in > the partner alternate mode array? I think your patche(s) are working > around some deeper issue. We really need to figure out what that is. > > Let's check the trace output. You need to build the UCSI drivers as > modules. Then: > > modprobe -r ucsi_acpi > modprobe typec_ucsi > mount debugfs -t debugfs /sys/kernel/debug > cd /sys/kernel/debug/tracing > echo 1 > events/ucsi/enable > modprobe ucsi_acpi > > Wait one minute and: > > cat trace FYI: The below trace is with the v5 patch i have applied. # tracer: nop # # entries-in-buffer/entries-written: 66/66 #P:8 # # _-----=> irqs-off # / _----=> need-resched # | / _---=> hardirq/softirq # || / _--=> preempt-depth # ||| / delay # TASK-PID CPU# |||| TIMESTAMP FUNCTION # | | | |||| | | <...>-158117 [001] .... 364352.017115: ucsi_register_altmode: port alt mode: svid 17ef, mode 1 vdo 1 <...>-158117 [001] .... 364352.176246: ucsi_register_port: port0 status: change=5200, opmode=0, connected=0, sourcing=0, partner_flags=0, partner_type=1, request_data_obj=00000000, BC status=1 <...>-158675 [000] .... 364385.849034: ucsi_connector_change: port0 status: change=5200, opmode=3, connected=1, sourcing=0, partner_flags=2, partner_type=2, request_data_obj=53851545, BC status=1 <...>-160424 [000] .... 364386.331802: ucsi_register_altmode: partner alt mode: svid 8087, mode 1 vdo ffffffff <...>-160424 [000] .... 364386.331840: ucsi_register_altmode: partner alt mode: svid ff01, mode 1 vdo ffffffff <...>-160424 [000] .... 364386.448424: ucsi_register_altmode: partner alt mode: svid 8087, mode 2 vdo ffffffff <...>-160424 [000] .... 364386.448454: ucsi_register_altmode: partner alt mode: svid ff01, mode 2 vdo ffffffff <...>-160424 [000] .... 364386.549487: ucsi_register_altmode: partner alt mode: svid 8087, mode 3 vdo ffffffff <...>-160424 [000] .... 364386.549503: ucsi_register_altmode: partner alt mode: svid ff01, mode 3 vdo ffffffff <...>-160424 [000] .... 364386.638980: ucsi_register_altmode: partner alt mode: svid 8087, mode 4 vdo ffffffff <...>-160424 [000] .... 364386.638998: ucsi_register_altmode: partner alt mode: svid ff01, mode 4 vdo ffffffff <...>-160424 [000] .... 364386.746501: ucsi_register_altmode: partner alt mode: svid 8087, mode 5 vdo ffffffff <...>-160424 [000] .... 364386.746533: ucsi_register_altmode: partner alt mode: svid ff01, mode 5 vdo ffffffff <...>-160424 [000] .... 364386.851931: ucsi_register_altmode: partner alt mode: svid 8087, mode 6 vdo ffffffff <...>-160424 [000] .... 364386.851970: ucsi_register_altmode: partner alt mode: svid ff01, mode 6 vdo ffffffff <...>-160424 [000] .... 364386.962212: ucsi_register_altmode: partner alt mode: svid 8087, mode 7 vdo ffffffff <...>-160424 [000] .... 364386.962243: ucsi_register_altmode: partner alt mode: svid ff01, mode 7 vdo ffffffff <...>-160424 [000] .... 364387.081056: ucsi_register_altmode: partner alt mode: svid 8087, mode 8 vdo ffffffff <...>-160424 [000] .... 364387.081100: ucsi_register_altmode: partner alt mode: svid ff01, mode 8 vdo ffffffff <...>-160424 [000] .... 364387.213591: ucsi_register_altmode: partner alt mode: svid 8087, mode 9 vdo ffffffff <...>-160424 [000] .... 364387.213621: ucsi_register_altmode: partner alt mode: svid ff01, mode 9 vdo ffffffff <...>-160424 [000] .... 364387.335754: ucsi_register_altmode: partner alt mode: svid 8087, mode 10 vdo ffffffff <...>-160424 [000] .... 364387.335770: ucsi_register_altmode: partner alt mode: svid ff01, mode 10 vdo ffffffff <...>-160424 [000] .... 364387.446308: ucsi_register_altmode: partner alt mode: svid 8087, mode 11 vdo ffffffff <...>-160424 [000] .... 364387.446334: ucsi_register_altmode: partner alt mode: svid ff01, mode 11 vdo ffffffff <...>-160424 [000] .... 364387.548723: ucsi_register_altmode: partner alt mode: svid 8087, mode 12 vdo ffffffff <...>-160424 [000] .... 364387.548740: ucsi_register_altmode: partner alt mode: svid ff01, mode 12 vdo ffffffff <...>-160424 [000] .... 364387.661713: ucsi_register_altmode: partner alt mode: svid 8087, mode 13 vdo ffffffff <...>-160424 [000] .... 364387.661792: ucsi_register_altmode: partner alt mode: svid ff01, mode 13 vdo ffffffff <...>-160424 [000] .... 364387.765664: ucsi_register_altmode: partner alt mode: svid 8087, mode 14 vdo ffffffff <...>-160424 [000] .... 364387.765678: ucsi_register_altmode: partner alt mode: svid ff01, mode 14 vdo ffffffff <...>-160424 [000] .... 364387.874643: ucsi_register_altmode: partner alt mode: svid 8087, mode 15 vdo ffffffff <...>-160424 [000] .... 364387.874664: ucsi_register_altmode: partner alt mode: svid ff01, mode 15 vdo ffffffff <...>-160424 [000] .... 364387.999382: ucsi_connector_change: port0 status: change=1b60, opmode=3, connected=1, sourcing=0, partner_flags=2, partner_type=2, request_data_obj=53851545, BC status=1 <...>-160424 [000] .... 364422.947503: ucsi_register_altmode: port alt mode: svid 17ef, mode 1 vdo 1 <...>-160424 [000] .... 364423.323834: ucsi_register_altmode: partner alt mode: svid 8087, mode 1 vdo ffffffff <...>-160424 [000] .... 364423.323903: ucsi_register_altmode: partner alt mode: svid ff01, mode 1 vdo ffffffff <...>-160424 [000] .... 364423.464644: ucsi_register_altmode: partner alt mode: svid 8087, mode 2 vdo ffffffff <...>-160424 [000] .... 364423.464659: ucsi_register_altmode: partner alt mode: svid ff01, mode 2 vdo ffffffff <...>-160424 [000] .... 364423.591779: ucsi_register_altmode: partner alt mode: svid 8087, mode 3 vdo ffffffff <...>-160424 [000] .... 364423.592074: ucsi_register_altmode: partner alt mode: svid ff01, mode 3 vdo ffffffff <...>-160424 [000] .... 364423.717445: ucsi_register_altmode: partner alt mode: svid 8087, mode 4 vdo ffffffff <...>-160424 [000] .... 364423.717684: ucsi_register_altmode: partner alt mode: svid ff01, mode 4 vdo ffffffff <...>-160424 [000] .... 364423.825988: ucsi_register_altmode: partner alt mode: svid 8087, mode 5 vdo ffffffff <...>-160424 [000] .... 364423.826017: ucsi_register_altmode: partner alt mode: svid ff01, mode 5 vdo ffffffff <...>-160424 [000] .... 364423.948363: ucsi_register_altmode: partner alt mode: svid 8087, mode 6 vdo ffffffff <...>-160424 [000] .... 364423.948425: ucsi_register_altmode: partner alt mode: svid ff01, mode 6 vdo ffffffff <...>-160424 [000] .... 364424.059488: ucsi_register_altmode: partner alt mode: svid 8087, mode 7 vdo ffffffff <...>-160424 [000] .... 364424.059528: ucsi_register_altmode: partner alt mode: svid ff01, mode 7 vdo ffffffff <...>-160424 [000] .... 364424.192603: ucsi_register_altmode: partner alt mode: svid 8087, mode 8 vdo ffffffff <...>-160424 [000] .... 364424.192630: ucsi_register_altmode: partner alt mode: svid ff01, mode 8 vdo ffffffff <...>-160424 [000] .... 364424.301329: ucsi_register_altmode: partner alt mode: svid 8087, mode 9 vdo ffffffff <...>-160424 [000] .... 364424.301373: ucsi_register_altmode: partner alt mode: svid ff01, mode 9 vdo ffffffff <...>-160424 [000] .... 364424.428156: ucsi_register_altmode: partner alt mode: svid 8087, mode 10 vdo ffffffff <...>-160424 [000] .... 364424.428277: ucsi_register_altmode: partner alt mode: svid ff01, mode 10 vdo ffffffff <...>-160424 [000] .... 364424.557797: ucsi_register_altmode: partner alt mode: svid 8087, mode 11 vdo ffffffff <...>-160424 [000] .... 364424.557835: ucsi_register_altmode: partner alt mode: svid ff01, mode 11 vdo ffffffff <...>-160424 [000] .... 364424.696307: ucsi_register_altmode: partner alt mode: svid 8087, mode 12 vdo ffffffff <...>-160424 [000] .... 364424.696379: ucsi_register_altmode: partner alt mode: svid ff01, mode 12 vdo ffffffff <...>-160424 [000] .... 364424.806302: ucsi_register_altmode: partner alt mode: svid 8087, mode 13 vdo ffffffff <...>-160424 [000] .... 364424.806359: ucsi_register_altmode: partner alt mode: svid ff01, mode 13 vdo ffffffff <...>-160424 [000] .... 364424.937869: ucsi_register_altmode: partner alt mode: svid 8087, mode 14 vdo ffffffff <...>-160424 [000] .... 364424.937949: ucsi_register_altmode: partner alt mode: svid ff01, mode 14 vdo ffffffff <...>-160424 [000] .... 364425.058839: ucsi_register_altmode: partner alt mode: svid 8087, mode 15 vdo ffffffff <...>-160424 [000] .... 364425.058856: ucsi_register_altmode: partner alt mode: svid ff01, mode 15 vdo ffffffff <...>-160424 [000] .... 364425.181786: ucsi_register_port: port0 status: change=0000, opmode=3, connected=1, sourcing=0, partner_flags=2, partner_type=2, request_data_obj=53851545, BC status=1
Hi, On Fri, Sep 11, 2020 at 04:56:22PM +0300, Heikki Krogerus wrote: > Looks like the firmware does not terminate the list of alternate modes > at all. It's just returning the two supported modes over and over > again, regardless of the requested mode offset... I need to think how > that should be handled. Since we can't rely on the data that the firmware returns, we also have to check that the mode index does not exceed MODE_DISCOVER_MAX. Can you test if the attached patch fixes the issue for you? thanks,
On Mon, 14 Sep 2020, Heikki Krogerus wrote: > Hi, > > On Fri, Sep 11, 2020 at 04:56:22PM +0300, Heikki Krogerus wrote: > > Looks like the firmware does not terminate the list of alternate modes > > at all. It's just returning the two supported modes over and over > > again, regardless of the requested mode offset... I need to think how > > that should be handled. > > Since we can't rely on the data that the firmware returns, we also > have to check that the mode index does not exceed MODE_DISCOVER_MAX. > Can you test if the attached patch fixes the issue for you? Sadly that's not entirely surprising :/ i tested your patch and i was able to plugin and unplug the USB-C dock with a working display multiple times. Thanks! Zwane
On Mon, Sep 14, 2020 at 10:56:56AM -0700, Zwane Mwaikambo wrote: > On Mon, 14 Sep 2020, Heikki Krogerus wrote: > > > Hi, > > > > On Fri, Sep 11, 2020 at 04:56:22PM +0300, Heikki Krogerus wrote: > > > Looks like the firmware does not terminate the list of alternate modes > > > at all. It's just returning the two supported modes over and over > > > again, regardless of the requested mode offset... I need to think how > > > that should be handled. > > > > Since we can't rely on the data that the firmware returns, we also > > have to check that the mode index does not exceed MODE_DISCOVER_MAX. > > Can you test if the attached patch fixes the issue for you? > > Sadly that's not entirely surprising :/ i tested your patch and i was able > to plugin and unplug the USB-C dock with a working display multiple > times. OK. Let's fix the issue with this at this stage. thanks, -- heikki
On Wed, 16 Sep 2020, Heikki Krogerus wrote: > On Mon, Sep 14, 2020 at 10:56:56AM -0700, Zwane Mwaikambo wrote: > > On Mon, 14 Sep 2020, Heikki Krogerus wrote: > > > > > Hi, > > > > > > On Fri, Sep 11, 2020 at 04:56:22PM +0300, Heikki Krogerus wrote: > > > > Looks like the firmware does not terminate the list of alternate modes > > > > at all. It's just returning the two supported modes over and over > > > > again, regardless of the requested mode offset... I need to think how > > > > that should be handled. > > > > > > Since we can't rely on the data that the firmware returns, we also > > > have to check that the mode index does not exceed MODE_DISCOVER_MAX. > > > Can you test if the attached patch fixes the issue for you? > > > > Sadly that's not entirely surprising :/ i tested your patch and i was able > > to plugin and unplug the USB-C dock with a working display multiple > > times. > > OK. Let's fix the issue with this at this stage. Thanks for digging into the issue and resolving it! Zwane
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c index affd024190c9..16906519c173 100644 --- a/drivers/usb/typec/ucsi/ucsi.c +++ b/drivers/usb/typec/ucsi/ucsi.c @@ -218,9 +218,10 @@ void ucsi_altmode_update_active(struct ucsi_connector *con) if (cur < UCSI_MAX_ALTMODES) altmode = typec_altmode_get_partner(con->port_altmode[cur]); - for (i = 0; con->partner_altmode[i]; i++) - typec_altmode_update_active(con->partner_altmode[i], - con->partner_altmode[i] == altmode); + for (i = 0; i < UCSI_MAX_ALTMODES; i++) + if (con->partner_altmode[i]) + typec_altmode_update_active(con->partner_altmode[i], + con->partner_altmode[i] == altmode); } static u8 ucsi_altmode_next_mode(struct typec_altmode **alt, u16 svid)
con->partner_altmode[i] ends up with the value 0x2 in the call to typec_altmode_update_active because the array has been accessed out of bounds causing a random memory read. This patch fixes the first occurrence and 2/2 the second. [ 565.452023] BUG: kernel NULL pointer dereference, address: 00000000000002fe [ 565.452025] #PF: supervisor read access in kernel mode [ 565.452026] #PF: error_code(0x0000) - not-present page [ 565.452026] PGD 0 P4D 0 [ 565.452028] Oops: 0000 [#1] PREEMPT SMP NOPTI [ 565.452030] CPU: 0 PID: 502 Comm: kworker/0:3 Not tainted 5.8.0-rc3+ #1 [ 565.452031] Hardware name: LENOVO 20RD002VUS/20RD002VUS, BIOS R16ET25W (1.11 ) 04/21/2020 [ 565.452034] Workqueue: events ucsi_handle_connector_change [typec_ucsi] [ 565.452039] RIP: 0010:typec_altmode_update_active+0x1f/0x100 [typec] [ 565.452040] Code: 0f 1f 84 00 00 00 00 00 0f 1f 00 0f 1f 44 00 00 55 48 89 e5 41 54 53 48 83 ec 10 65 48 8b 04 25 28 00 00 00 48 89 45 e8 31 c0 <0f> b6 87 fc 02 00 00 83 e0 01 40 38 f0 0f 84 95 00 00 00 48 8b 47 [ 565.452041] RSP: 0018:ffffb729c066bdb0 EFLAGS: 00010246 [ 565.452042] RAX: 0000000000000000 RBX: ffffa067c3e64a70 RCX: 0000000000000000 [ 565.452043] RDX: ffffb729c066bd20 RSI: 0000000000000000 RDI: 0000000000000002 [ 565.452044] RBP: ffffb729c066bdd0 R08: 00000083a7910a4f R09: 0000000000000000 [ 565.452044] R10: ffffffffa106a220 R11: 0000000000000000 R12: 0000000000000000 [ 565.452045] R13: ffffa067c3e64a98 R14: ffffa067c3e64810 R15: ffffa067c3e64800 [ 565.452046] FS: 0000000000000000(0000) GS:ffffa067d1400000(0000) knlGS:0000000000000000 [ 565.452047] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 565.452048] CR2: 00000000000002fe CR3: 00000001b060a003 CR4: 00000000003606f0 [ 565.452048] Call Trace: [ 565.452052] ucsi_altmode_update_active+0x83/0xd0 [typec_ucsi] [ 565.452054] ucsi_handle_connector_change+0x1dc/0x320 [typec_ucsi] [ 565.452057] process_one_work+0x1df/0x3d0 [ 565.452059] worker_thread+0x4d/0x3d0 [ 565.452060] ? process_one_work+0x3d0/0x3d0 [ 565.452062] kthread+0x127/0x170 [ 565.452063] ? kthread_park+0x90/0x90 [ 565.452065] ret_from_fork+0x1f/0x30 The failing instruction is; 0x0000000000000380 <+0>: callq 0x385 <typec_altmode_update_active+5> 0x0000000000000385 <+5>: push %rbp 0x0000000000000386 <+6>: mov %rsp,%rbp 0x0000000000000389 <+9>: push %r12 0x000000000000038b <+11>: push %rbx 0x000000000000038c <+12>: sub $0x10,%rsp 0x0000000000000390 <+16>: mov %gs:0x28,%rax 0x0000000000000399 <+25>: mov %rax,-0x18(%rbp) 0x000000000000039d <+29>: xor %eax,%eax 0x000000000000039f <+31>: movzbl 0x2fc(%rdi),%eax <====== 0x00000000000003a6 <+38>: and $0x1,%eax (gdb) list *typec_altmode_update_active+0x1f 0x39f is in typec_altmode_update_active (drivers/usb/typec/class.c:221). 216 */ 217 void typec_altmode_update_active(struct typec_altmode *adev, bool active) 218 { 219 char dir[6]; 220 221 if (adev->active == active) 222 return; 223 224 if (!is_typec_port(adev->dev.parent) && adev->dev.driver) { 225 if (!active) (gdb) list *ucsi_altmode_update_active+0x83 0x12a3 is in ucsi_altmode_update_active (drivers/usb/typec/ucsi/ucsi.c:221). 216 } 217 218 if (cur < UCSI_MAX_ALTMODES) 219 altmode = typec_altmode_get_partner(con->port_altmode[cur]); 220 221 for (i = 0; con->partner_altmode[i]; i++) 222 typec_altmode_update_active(con->partner_altmode[i], 223 con->partner_altmode[i] == altmode); 224 } Signed-off-by: Zwane Mwaikambo <zwane@yosper.io> --- This v4 addresses patch formatting and submission issues with the previous versions.