[PATCH v3 1/2] usb/typec: fix array overruns in ucsi.c partner_altmode[]

[PATCH v3 1/2] usb/typec: fix array overruns in ucsi.c partner_altmode[]

[Zwane Mwaikambo]

This v3 addresses patch formatting and submission issues with the 
previous versions.

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 occurence 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>
---

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index d0c63afaf..79061705e 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)
 

[PATCH v3 2/2] usb/typec: fix array overruns in ucsi.c partner_altmode[]

[Zwane Mwaikambo]

This fixes the second array overrun occurence (similar failure mode to 
the first), this time in ucsi_unregister_altmodes.

[ 4373.153246] BUG: kernel NULL pointer dereference, address: 
00000000000002f2
[ 4373.153267] #PF: supervisor read access in kernel mode
[ 4373.153271] #PF: error_code(0x0000) - not-present page
[ 4373.153275] PGD 0 P4D 0 
[ 4373.153284] Oops: 0000 [#2] PREEMPT SMP NOPTI
[ 4373.153292] CPU: 0 PID: 13242 Comm: kworker/0:0 Tainted: G      D           
5.8.0-rc6+ #1
[ 4373.153296] Hardware name: LENOVO 20RD002VUS/20RD002VUS, BIOS R16ET25W 
(1.11 ) 04/21/2020
[ 4373.153308] Workqueue: events ucsi_handle_connector_change [typec_ucsi]
[ 4373.153320] RIP: 0010:ucsi_unregister_altmodes+0x5f/0xa0 [typec_ucsi]
[ 4373.153326] Code: 54 48 8b 3b 41 83 c4 01 e8 9e f9 0c 00 49 63 c4 48 c7 
03 00 00 00 00 49 8d 5c c5 00 48 8b 3b 48 85 ff 74 31 41 80 fe 01 75 d7 
<0f> b7 87 f0 02 00 00 66 3d 01 ff 74 0f 66 3d 55 09 75 c4 83 bf f8
[ 4373.153332] RSP: 0018:ffffb2ef036b3dc8 EFLAGS: 00010246
[ 4373.153338] RAX: 000000000000001e RBX: ffff94268b006a60 RCX: 
0000000080800067
[ 4373.153342] RDX: 0000000080800068 RSI: 0000000000000001 RDI: 
0000000000000002
[ 4373.153347] RBP: ffffb2ef036b3de8 R08: 0000000000000000 R09: 
ffffffff8dc65400
[ 4373.153351] R10: ffff9426678d7200 R11: 0000000000000001 R12: 
000000000000001e
[ 4373.153355] R13: ffff94268b006970 R14: 0000000000000001 R15: 
ffff94268b006800
[ 4373.153361] FS:  0000000000000000(0000) GS:ffff942691400000(0000) 
knlGS:0000000000000000
[ 4373.153366] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 4373.153371] CR2: 00000000000002f2 CR3: 00000004445a6005 CR4: 
00000000003606f0
[ 4373.153375] Call Trace:
[ 4373.153389]  ucsi_unregister_partner.part.0+0x17/0x30 [typec_ucsi]
[ 4373.153400]  ucsi_handle_connector_change+0x25c/0x320 [typec_ucsi]
[ 4373.153418]  process_one_work+0x1df/0x3d0
[ 4373.153428]  worker_thread+0x4a/0x3d0
[ 4373.153436]  ? process_one_work+0x3d0/0x3d0
[ 4373.153444]  kthread+0x127/0x170
[ 4373.153451]  ? kthread_park+0x90/0x90
[ 4373.153461]  ret_from_fork+0x1f/0x30
[ 4373.153661] CR2: 00000000000002f2

Signed-off-by: Zwane Mwaikambo <zwane@yosper.io>
---

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index d0c63afaf..79061705e 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -479,7 +480,10 @@ static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient)
 		return;
 	}
 
-	while (adev[i]) {
+	for (i = 0; i < UCSI_MAX_ALTMODES; i++) {
+		if (!adev[i])
+			break;
+
 		if (recipient == UCSI_RECIPIENT_SOP &&
 		    (adev[i]->svid == USB_TYPEC_DP_SID ||
 			(adev[i]->svid == USB_TYPEC_NVIDIA_VLINK_SID &&
@@ -488,7 +492,7 @@ static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient)
 			ucsi_displayport_remove_partner((void *)pdev);
 		}
 		typec_unregister_altmode(adev[i]);
-		adev[i++] = NULL;
+		adev[i] = NULL;
 	}
 }
 

Re: [PATCH v3 1/2] usb/typec: fix array overruns in ucsi.c partner_altmode[]

[Randy Dunlap]

On 8/27/20 10:49 AM, Zwane Mwaikambo wrote:
> This v3 addresses patch formatting and submission issues with the 
> previous versions.

That info goes after the "---" line.

> 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 occurence and 2/2 the second.

occurrence

> 
> Signed-off-by: Zwane Mwaikambo <zwane@yosper.io>
> ---
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index d0c63afaf..79061705e 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);

What happened to the indentation here?  Too much.

>  }
>  
>  static u8 ucsi_altmode_next_mode(struct typec_altmode **alt, u16 svid)
>  
> 


-- 
~Randy

Re: [PATCH v3 1/2] usb/typec: fix array overruns in ucsi.c partner_altmode[]

[Zwane Mwaikambo]

On Thu, 27 Aug 2020, Randy Dunlap wrote:

> On 8/27/20 10:49 AM, Zwane Mwaikambo wrote:
> > This v3 addresses patch formatting and submission issues with the 
> > previous versions.
> 
> That info goes after the "---" line.

Got it, i misunderstood Greg's comment.

> > 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 occurence and 2/2 the second.
> 
> occurrence

Corrected

> > Signed-off-by: Zwane Mwaikambo <zwane@yosper.io>
> > ---
> > 
> > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> > index d0c63afaf..79061705e 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);
> 
> What happened to the indentation here?  Too much.

It was tabs to line up the parameters, i'll update it to just one 
indentation level from the function.

Thanks,
	Zwane

[PATCH v4 1/2] usb/typec: fix array overruns in ucsi.c partner_altmode[]

[Zwane Mwaikambo]

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.


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)
 

[PATCH v4 2/2] usb/typec: fix array overruns in ucsi.c partner_altmode[]

[Zwane Mwaikambo]

This fixes the second array overrun occurrence (similar failure mode to 
the first), this time in ucsi_unregister_altmodes.

[ 4373.153246] BUG: kernel NULL pointer dereference, address: 
00000000000002f2
[ 4373.153267] #PF: supervisor read access in kernel mode
[ 4373.153271] #PF: error_code(0x0000) - not-present page
[ 4373.153275] PGD 0 P4D 0 
[ 4373.153284] Oops: 0000 [#2] PREEMPT SMP NOPTI
[ 4373.153292] CPU: 0 PID: 13242 Comm: kworker/0:0 Tainted: G      D           
5.8.0-rc6+ #1
[ 4373.153296] Hardware name: LENOVO 20RD002VUS/20RD002VUS, BIOS R16ET25W 
(1.11 ) 04/21/2020
[ 4373.153308] Workqueue: events ucsi_handle_connector_change [typec_ucsi]
[ 4373.153320] RIP: 0010:ucsi_unregister_altmodes+0x5f/0xa0 [typec_ucsi]
[ 4373.153326] Code: 54 48 8b 3b 41 83 c4 01 e8 9e f9 0c 00 49 63 c4 48 c7 
03 00 00 00 00 49 8d 5c c5 00 48 8b 3b 48 85 ff 74 31 41 80 fe 01 75 d7 
<0f> b7 87 f0 02 00 00 66 3d 01 ff 74 0f 66 3d 55 09 75 c4 83 bf f8
[ 4373.153332] RSP: 0018:ffffb2ef036b3dc8 EFLAGS: 00010246
[ 4373.153338] RAX: 000000000000001e RBX: ffff94268b006a60 RCX: 
0000000080800067
[ 4373.153342] RDX: 0000000080800068 RSI: 0000000000000001 RDI: 
0000000000000002
[ 4373.153347] RBP: ffffb2ef036b3de8 R08: 0000000000000000 R09: 
ffffffff8dc65400
[ 4373.153351] R10: ffff9426678d7200 R11: 0000000000000001 R12: 
000000000000001e
[ 4373.153355] R13: ffff94268b006970 R14: 0000000000000001 R15: 
ffff94268b006800
[ 4373.153361] FS:  0000000000000000(0000) GS:ffff942691400000(0000) 
knlGS:0000000000000000
[ 4373.153366] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 4373.153371] CR2: 00000000000002f2 CR3: 00000004445a6005 CR4: 
00000000003606f0
[ 4373.153375] Call Trace:
[ 4373.153389]  ucsi_unregister_partner.part.0+0x17/0x30 [typec_ucsi]
[ 4373.153400]  ucsi_handle_connector_change+0x25c/0x320 [typec_ucsi]
[ 4373.153418]  process_one_work+0x1df/0x3d0
[ 4373.153428]  worker_thread+0x4a/0x3d0
[ 4373.153436]  ? process_one_work+0x3d0/0x3d0
[ 4373.153444]  kthread+0x127/0x170
[ 4373.153451]  ? kthread_park+0x90/0x90
[ 4373.153461]  ret_from_fork+0x1f/0x30
[ 4373.153661] CR2: 00000000000002f2

Signed-off-by: Zwane Mwaikambo <zwane@yosper.io>
---

This v4 addresses patch formatting and submission issues with the 
previous versions.

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index d0c63afaf..79061705e 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -479,7 +480,10 @@ static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient)
 		return;
 	}
 
-	while (adev[i]) {
+	for (i = 0; i < UCSI_MAX_ALTMODES; i++) {
+		if (!adev[i])
+			break;
+
 		if (recipient == UCSI_RECIPIENT_SOP &&
 		    (adev[i]->svid == USB_TYPEC_DP_SID ||
 			(adev[i]->svid == USB_TYPEC_NVIDIA_VLINK_SID &&
@@ -488,7 +492,7 @@ static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient)
 			ucsi_displayport_remove_partner((void *)pdev);
 		}
 		typec_unregister_altmode(adev[i]);
-		adev[i++] = NULL;
+		adev[i] = NULL;
 	}
 }
 

Re: [PATCH v4 1/2] usb/typec: fix array overruns in ucsi.c partner_altmode[]

[Heikki Krogerus]

On Thu, Aug 27, 2020 at 11:34: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.
> 
> [  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 needs Fixes tag, and the "Cc: stable..." tag. I think
ad74b8649bea is what you are fixing here. Please double check that for
example with git blame, but if that's the commit then these are your
tags:

        Fixes: ad74b8649bea ("usb: typec: ucsi: Preliminary support for alternate modes")
        Cc: stable@vger.kernel.org
        Signed-off-by: Zwane Mwaikambo <zwane@yosper.io>

> ---
> 
> This v4 addresses patch formatting and submission issues with the 
> previous versions.

Please include the whole history.

        v4: This and that was changed.
        v3: That and this was changed.
        v2: ...

> 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,

-- 
heikki

Re: [PATCH v4 2/2] usb/typec: fix array overruns in ucsi.c partner_altmode[]

[Heikki Krogerus]

On Thu, Aug 27, 2020 at 11:35:53AM -0700, Zwane Mwaikambo wrote:
> This fixes the second array overrun occurrence (similar failure mode to 
> the first), this time in ucsi_unregister_altmodes.
> 
> [ 4373.153246] BUG: kernel NULL pointer dereference, address: 
> 00000000000002f2
> [ 4373.153267] #PF: supervisor read access in kernel mode
> [ 4373.153271] #PF: error_code(0x0000) - not-present page
> [ 4373.153275] PGD 0 P4D 0 
> [ 4373.153284] Oops: 0000 [#2] PREEMPT SMP NOPTI
> [ 4373.153292] CPU: 0 PID: 13242 Comm: kworker/0:0 Tainted: G      D           
> 5.8.0-rc6+ #1
> [ 4373.153296] Hardware name: LENOVO 20RD002VUS/20RD002VUS, BIOS R16ET25W 
> (1.11 ) 04/21/2020
> [ 4373.153308] Workqueue: events ucsi_handle_connector_change [typec_ucsi]
> [ 4373.153320] RIP: 0010:ucsi_unregister_altmodes+0x5f/0xa0 [typec_ucsi]
> [ 4373.153326] Code: 54 48 8b 3b 41 83 c4 01 e8 9e f9 0c 00 49 63 c4 48 c7 
> 03 00 00 00 00 49 8d 5c c5 00 48 8b 3b 48 85 ff 74 31 41 80 fe 01 75 d7 
> <0f> b7 87 f0 02 00 00 66 3d 01 ff 74 0f 66 3d 55 09 75 c4 83 bf f8
> [ 4373.153332] RSP: 0018:ffffb2ef036b3dc8 EFLAGS: 00010246
> [ 4373.153338] RAX: 000000000000001e RBX: ffff94268b006a60 RCX: 
> 0000000080800067
> [ 4373.153342] RDX: 0000000080800068 RSI: 0000000000000001 RDI: 
> 0000000000000002
> [ 4373.153347] RBP: ffffb2ef036b3de8 R08: 0000000000000000 R09: 
> ffffffff8dc65400
> [ 4373.153351] R10: ffff9426678d7200 R11: 0000000000000001 R12: 
> 000000000000001e
> [ 4373.153355] R13: ffff94268b006970 R14: 0000000000000001 R15: 
> ffff94268b006800
> [ 4373.153361] FS:  0000000000000000(0000) GS:ffff942691400000(0000) 
> knlGS:0000000000000000
> [ 4373.153366] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 4373.153371] CR2: 00000000000002f2 CR3: 00000004445a6005 CR4: 
> 00000000003606f0
> [ 4373.153375] Call Trace:
> [ 4373.153389]  ucsi_unregister_partner.part.0+0x17/0x30 [typec_ucsi]
> [ 4373.153400]  ucsi_handle_connector_change+0x25c/0x320 [typec_ucsi]
> [ 4373.153418]  process_one_work+0x1df/0x3d0
> [ 4373.153428]  worker_thread+0x4a/0x3d0
> [ 4373.153436]  ? process_one_work+0x3d0/0x3d0
> [ 4373.153444]  kthread+0x127/0x170
> [ 4373.153451]  ? kthread_park+0x90/0x90
> [ 4373.153461]  ret_from_fork+0x1f/0x30
> [ 4373.153661] CR2: 00000000000002f2
> 
> Signed-off-by: Zwane Mwaikambo <zwane@yosper.io>

This is also fixing something.

        Fixes: ad74b8649bea ("usb: typec: ucsi: Preliminary support for alternate modes")
        Cc: stable@vger.kernel.org
        Signed-off-by: Zwane Mwaikambo <zwane@yosper.io>

> ---
> 
> This v4 addresses patch formatting and submission issues with the 
> previous versions.
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index d0c63afaf..79061705e 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -479,7 +480,10 @@ static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient)
>  		return;
>  	}
>  
> -	while (adev[i]) {
> +	for (i = 0; i < UCSI_MAX_ALTMODES; i++) {
> +		if (!adev[i])
> +			break;
> +
>  		if (recipient == UCSI_RECIPIENT_SOP &&
>  		    (adev[i]->svid == USB_TYPEC_DP_SID ||
>  			(adev[i]->svid == USB_TYPEC_NVIDIA_VLINK_SID &&
> @@ -488,7 +492,7 @@ static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient)
>  			ucsi_displayport_remove_partner((void *)pdev);
>  		}
>  		typec_unregister_altmode(adev[i]);
> -		adev[i++] = NULL;
> +		adev[i] = NULL;
>  	}
>  }
>  

-- 
heikki

[PATCH v5 1/2] usb/typec: fix array overruns in ucsi.c partner_altmode[]

[Zwane Mwaikambo]

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     }

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)
 

[PATCH v5 2/2] usb/typec: fix array overruns in ucsi.c partner_altmode[]

[Zwane Mwaikambo]

This fixes the second array overrun occurrence (similar failure mode to 
the first), this time in ucsi_unregister_altmodes.

[ 4373.153246] BUG: kernel NULL pointer dereference, address: 
00000000000002f2
[ 4373.153267] #PF: supervisor read access in kernel mode
[ 4373.153271] #PF: error_code(0x0000) - not-present page
[ 4373.153275] PGD 0 P4D 0 
[ 4373.153284] Oops: 0000 [#2] PREEMPT SMP NOPTI
[ 4373.153292] CPU: 0 PID: 13242 Comm: kworker/0:0 Tainted: G      D           
5.8.0-rc6+ #1
[ 4373.153296] Hardware name: LENOVO 20RD002VUS/20RD002VUS, BIOS R16ET25W 
(1.11 ) 04/21/2020
[ 4373.153308] Workqueue: events ucsi_handle_connector_change [typec_ucsi]
[ 4373.153320] RIP: 0010:ucsi_unregister_altmodes+0x5f/0xa0 [typec_ucsi]
[ 4373.153326] Code: 54 48 8b 3b 41 83 c4 01 e8 9e f9 0c 00 49 63 c4 48 c7 
03 00 00 00 00 49 8d 5c c5 00 48 8b 3b 48 85 ff 74 31 41 80 fe 01 75 d7 
<0f> b7 87 f0 02 00 00 66 3d 01 ff 74 0f 66 3d 55 09 75 c4 83 bf f8
[ 4373.153332] RSP: 0018:ffffb2ef036b3dc8 EFLAGS: 00010246
[ 4373.153338] RAX: 000000000000001e RBX: ffff94268b006a60 RCX: 
0000000080800067
[ 4373.153342] RDX: 0000000080800068 RSI: 0000000000000001 RDI: 
0000000000000002
[ 4373.153347] RBP: ffffb2ef036b3de8 R08: 0000000000000000 R09: 
ffffffff8dc65400
[ 4373.153351] R10: ffff9426678d7200 R11: 0000000000000001 R12: 
000000000000001e
[ 4373.153355] R13: ffff94268b006970 R14: 0000000000000001 R15: 
ffff94268b006800
[ 4373.153361] FS:  0000000000000000(0000) GS:ffff942691400000(0000) 
knlGS:0000000000000000
[ 4373.153366] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 4373.153371] CR2: 00000000000002f2 CR3: 00000004445a6005 CR4: 
00000000003606f0
[ 4373.153375] Call Trace:
[ 4373.153389]  ucsi_unregister_partner.part.0+0x17/0x30 [typec_ucsi]
[ 4373.153400]  ucsi_handle_connector_change+0x25c/0x320 [typec_ucsi]
[ 4373.153418]  process_one_work+0x1df/0x3d0
[ 4373.153428]  worker_thread+0x4a/0x3d0
[ 4373.153436]  ? process_one_work+0x3d0/0x3d0
[ 4373.153444]  kthread+0x127/0x170
[ 4373.153451]  ? kthread_park+0x90/0x90
[ 4373.153461]  ret_from_fork+0x1f/0x30
[ 4373.153661] CR2: 00000000000002f2

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 d0c63afaf..79061705e 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -479,7 +480,10 @@ static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient)
 		return;
 	}
 
-	while (adev[i]) {
+	for (i = 0; i < UCSI_MAX_ALTMODES; i++) {
+		if (!adev[i])
+			break;
+
 		if (recipient == UCSI_RECIPIENT_SOP &&
 		    (adev[i]->svid == USB_TYPEC_DP_SID ||
 			(adev[i]->svid == USB_TYPEC_NVIDIA_VLINK_SID &&
@@ -488,7 +492,7 @@ static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient)
 			ucsi_displayport_remove_partner((void *)pdev);
 		}
 		typec_unregister_altmode(adev[i]);
-		adev[i++] = NULL;
+		adev[i] = NULL;
 	}
 }
 

Re: [PATCH v5 1/2] usb/typec: fix array overruns in ucsi.c partner_altmode[]

[Heikki Krogerus]

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,

-- 
heikki

Re: [PATCH v5 1/2] usb/typec: fix array overruns in ucsi.c partner_altmode[]

[Zwane Mwaikambo]

On Thu, Sep 3, 2020 at 4:10 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> 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.


True :( I can resend a v6 to fix that if needed.

Thanks!

>
> > [  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,
>
> --
> heikki

Re: [PATCH v5 2/2] usb/typec: fix array overruns in ucsi.c partner_altmode[]

[Heikki Krogerus]

Hi,

One nitpick below...

On Sun, Aug 30, 2020 at 02:28:39AM -0700, Zwane Mwaikambo wrote:
> This fixes the second array overrun occurrence (similar failure mode to 
> the first), this time in ucsi_unregister_altmodes.
> 
> [ 4373.153246] BUG: kernel NULL pointer dereference, address: 
> 00000000000002f2
> [ 4373.153267] #PF: supervisor read access in kernel mode
> [ 4373.153271] #PF: error_code(0x0000) - not-present page
> [ 4373.153275] PGD 0 P4D 0 
> [ 4373.153284] Oops: 0000 [#2] PREEMPT SMP NOPTI
> [ 4373.153292] CPU: 0 PID: 13242 Comm: kworker/0:0 Tainted: G      D           
> 5.8.0-rc6+ #1
> [ 4373.153296] Hardware name: LENOVO 20RD002VUS/20RD002VUS, BIOS R16ET25W 
> (1.11 ) 04/21/2020
> [ 4373.153308] Workqueue: events ucsi_handle_connector_change [typec_ucsi]
> [ 4373.153320] RIP: 0010:ucsi_unregister_altmodes+0x5f/0xa0 [typec_ucsi]
> [ 4373.153326] Code: 54 48 8b 3b 41 83 c4 01 e8 9e f9 0c 00 49 63 c4 48 c7 
> 03 00 00 00 00 49 8d 5c c5 00 48 8b 3b 48 85 ff 74 31 41 80 fe 01 75 d7 
> <0f> b7 87 f0 02 00 00 66 3d 01 ff 74 0f 66 3d 55 09 75 c4 83 bf f8
> [ 4373.153332] RSP: 0018:ffffb2ef036b3dc8 EFLAGS: 00010246
> [ 4373.153338] RAX: 000000000000001e RBX: ffff94268b006a60 RCX: 
> 0000000080800067
> [ 4373.153342] RDX: 0000000080800068 RSI: 0000000000000001 RDI: 
> 0000000000000002
> [ 4373.153347] RBP: ffffb2ef036b3de8 R08: 0000000000000000 R09: 
> ffffffff8dc65400
> [ 4373.153351] R10: ffff9426678d7200 R11: 0000000000000001 R12: 
> 000000000000001e
> [ 4373.153355] R13: ffff94268b006970 R14: 0000000000000001 R15: 
> ffff94268b006800
> [ 4373.153361] FS:  0000000000000000(0000) GS:ffff942691400000(0000) 
> knlGS:0000000000000000
> [ 4373.153366] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 4373.153371] CR2: 00000000000002f2 CR3: 00000004445a6005 CR4: 
> 00000000003606f0
> [ 4373.153375] Call Trace:
> [ 4373.153389]  ucsi_unregister_partner.part.0+0x17/0x30 [typec_ucsi]
> [ 4373.153400]  ucsi_handle_connector_change+0x25c/0x320 [typec_ucsi]
> [ 4373.153418]  process_one_work+0x1df/0x3d0
> [ 4373.153428]  worker_thread+0x4a/0x3d0
> [ 4373.153436]  ? process_one_work+0x3d0/0x3d0
> [ 4373.153444]  kthread+0x127/0x170
> [ 4373.153451]  ? kthread_park+0x90/0x90
> [ 4373.153461]  ret_from_fork+0x1f/0x30
> [ 4373.153661] CR2: 00000000000002f2
> 
> 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 d0c63afaf..79061705e 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -479,7 +480,10 @@ static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient)
>  		return;
>  	}
>  
> -	while (adev[i]) {
> +	for (i = 0; i < UCSI_MAX_ALTMODES; i++) {
> +		if (!adev[i])
> +			break;

        for (i = 0; i < UCSI_MAX_ALTMODES && adev[u]; i++) {

>  		if (recipient == UCSI_RECIPIENT_SOP &&
>  		    (adev[i]->svid == USB_TYPEC_DP_SID ||
>  			(adev[i]->svid == USB_TYPEC_NVIDIA_VLINK_SID &&
> @@ -488,7 +492,7 @@ static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient)
>  			ucsi_displayport_remove_partner((void *)pdev);
>  		}
>  		typec_unregister_altmode(adev[i]);
> -		adev[i++] = NULL;
> +		adev[i] = NULL;
>  	}
>  }
>  

thanks,

-- 
heikki

Re: [PATCH v5 1/2] usb/typec: fix array overruns in ucsi.c partner_altmode[]

[Heikki Krogerus]

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,

-- 
heikki

Re: [PATCH v5 2/2] usb/typec: fix array overruns in ucsi.c partner_altmode[]

[Randy Dunlap]

On 9/9/20 5:33 AM, Heikki Krogerus wrote:
> Hi,
> 
> One nitpick below...
> 
> On Sun, Aug 30, 2020 at 02:28:39AM -0700, Zwane Mwaikambo wrote:
>> This fixes the second array overrun occurrence (similar failure mode to 
>> the first), this time in ucsi_unregister_altmodes.
>>
>> [ 4373.153246] BUG: kernel NULL pointer dereference, address: 
>> 00000000000002f2
>> [ 4373.153267] #PF: supervisor read access in kernel mode
>> [ 4373.153271] #PF: error_code(0x0000) - not-present page
>> [ 4373.153275] PGD 0 P4D 0 
>> [ 4373.153284] Oops: 0000 [#2] PREEMPT SMP NOPTI
>> [ 4373.153292] CPU: 0 PID: 13242 Comm: kworker/0:0 Tainted: G      D           
>> 5.8.0-rc6+ #1
>> [ 4373.153296] Hardware name: LENOVO 20RD002VUS/20RD002VUS, BIOS R16ET25W 
>> (1.11 ) 04/21/2020
>> [ 4373.153308] Workqueue: events ucsi_handle_connector_change [typec_ucsi]
>> [ 4373.153320] RIP: 0010:ucsi_unregister_altmodes+0x5f/0xa0 [typec_ucsi]
>> [ 4373.153326] Code: 54 48 8b 3b 41 83 c4 01 e8 9e f9 0c 00 49 63 c4 48 c7 
>> 03 00 00 00 00 49 8d 5c c5 00 48 8b 3b 48 85 ff 74 31 41 80 fe 01 75 d7 
>> <0f> b7 87 f0 02 00 00 66 3d 01 ff 74 0f 66 3d 55 09 75 c4 83 bf f8
>> [ 4373.153332] RSP: 0018:ffffb2ef036b3dc8 EFLAGS: 00010246
>> [ 4373.153338] RAX: 000000000000001e RBX: ffff94268b006a60 RCX: 
>> 0000000080800067
>> [ 4373.153342] RDX: 0000000080800068 RSI: 0000000000000001 RDI: 
>> 0000000000000002
>> [ 4373.153347] RBP: ffffb2ef036b3de8 R08: 0000000000000000 R09: 
>> ffffffff8dc65400
>> [ 4373.153351] R10: ffff9426678d7200 R11: 0000000000000001 R12: 
>> 000000000000001e
>> [ 4373.153355] R13: ffff94268b006970 R14: 0000000000000001 R15: 
>> ffff94268b006800
>> [ 4373.153361] FS:  0000000000000000(0000) GS:ffff942691400000(0000) 
>> knlGS:0000000000000000
>> [ 4373.153366] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [ 4373.153371] CR2: 00000000000002f2 CR3: 00000004445a6005 CR4: 
>> 00000000003606f0
>> [ 4373.153375] Call Trace:
>> [ 4373.153389]  ucsi_unregister_partner.part.0+0x17/0x30 [typec_ucsi]
>> [ 4373.153400]  ucsi_handle_connector_change+0x25c/0x320 [typec_ucsi]
>> [ 4373.153418]  process_one_work+0x1df/0x3d0
>> [ 4373.153428]  worker_thread+0x4a/0x3d0
>> [ 4373.153436]  ? process_one_work+0x3d0/0x3d0
>> [ 4373.153444]  kthread+0x127/0x170
>> [ 4373.153451]  ? kthread_park+0x90/0x90
>> [ 4373.153461]  ret_from_fork+0x1f/0x30
>> [ 4373.153661] CR2: 00000000000002f2
>>
>> 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 d0c63afaf..79061705e 100644
>> --- a/drivers/usb/typec/ucsi/ucsi.c
>> +++ b/drivers/usb/typec/ucsi/ucsi.c
>> @@ -479,7 +480,10 @@ static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient)
>>  		return;
>>  	}
>>  
>> -	while (adev[i]) {
>> +	for (i = 0; i < UCSI_MAX_ALTMODES; i++) {
>> +		if (!adev[i])
>> +			break;
> 
>         for (i = 0; i < UCSI_MAX_ALTMODES && adev[u]; i++) {

make that                                      adev[i]

>>  		if (recipient == UCSI_RECIPIENT_SOP &&
>>  		    (adev[i]->svid == USB_TYPEC_DP_SID ||
>>  			(adev[i]->svid == USB_TYPEC_NVIDIA_VLINK_SID &&
>> @@ -488,7 +492,7 @@ static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient)
>>  			ucsi_displayport_remove_partner((void *)pdev);
>>  		}
>>  		typec_unregister_altmode(adev[i]);
>> -		adev[i++] = NULL;
>> +		adev[i] = NULL;
>>  	}
>>  }
>>  
> 
> thanks,
> 


-- 
~Randy
Reported-by: Randy Dunlap <rdunlap@infradead.org>

Re: [PATCH v5 2/2] usb/typec: fix array overruns in ucsi.c partner_altmode[]

[Zwane Mwaikambo]

On Wed, 9 Sep 2020, Randy Dunlap wrote:

> On 9/9/20 5:33 AM, Heikki Krogerus wrote:
> > Hi,
> > 
> > One nitpick below...
> > 
> > On Sun, Aug 30, 2020 at 02:28:39AM -0700, Zwane Mwaikambo wrote:
> >> This fixes the second array overrun occurrence (similar failure mode to 
> >> the first), this time in ucsi_unregister_altmodes.
> >>
> >> [ 4373.153246] BUG: kernel NULL pointer dereference, address: 
> >> 00000000000002f2
> >> [ 4373.153267] #PF: supervisor read access in kernel mode
> >> [ 4373.153271] #PF: error_code(0x0000) - not-present page
> >> [ 4373.153275] PGD 0 P4D 0 
> >> [ 4373.153284] Oops: 0000 [#2] PREEMPT SMP NOPTI
> >> [ 4373.153292] CPU: 0 PID: 13242 Comm: kworker/0:0 Tainted: G      D           
> >> 5.8.0-rc6+ #1
> >> [ 4373.153296] Hardware name: LENOVO 20RD002VUS/20RD002VUS, BIOS R16ET25W 
> >> (1.11 ) 04/21/2020
> >> [ 4373.153308] Workqueue: events ucsi_handle_connector_change [typec_ucsi]
> >> [ 4373.153320] RIP: 0010:ucsi_unregister_altmodes+0x5f/0xa0 [typec_ucsi]
> >> [ 4373.153326] Code: 54 48 8b 3b 41 83 c4 01 e8 9e f9 0c 00 49 63 c4 48 c7 
> >> 03 00 00 00 00 49 8d 5c c5 00 48 8b 3b 48 85 ff 74 31 41 80 fe 01 75 d7 
> >> <0f> b7 87 f0 02 00 00 66 3d 01 ff 74 0f 66 3d 55 09 75 c4 83 bf f8
> >> [ 4373.153332] RSP: 0018:ffffb2ef036b3dc8 EFLAGS: 00010246
> >> [ 4373.153338] RAX: 000000000000001e RBX: ffff94268b006a60 RCX: 
> >> 0000000080800067
> >> [ 4373.153342] RDX: 0000000080800068 RSI: 0000000000000001 RDI: 
> >> 0000000000000002
> >> [ 4373.153347] RBP: ffffb2ef036b3de8 R08: 0000000000000000 R09: 
> >> ffffffff8dc65400
> >> [ 4373.153351] R10: ffff9426678d7200 R11: 0000000000000001 R12: 
> >> 000000000000001e
> >> [ 4373.153355] R13: ffff94268b006970 R14: 0000000000000001 R15: 
> >> ffff94268b006800
> >> [ 4373.153361] FS:  0000000000000000(0000) GS:ffff942691400000(0000) 
> >> knlGS:0000000000000000
> >> [ 4373.153366] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >> [ 4373.153371] CR2: 00000000000002f2 CR3: 00000004445a6005 CR4: 
> >> 00000000003606f0
> >> [ 4373.153375] Call Trace:
> >> [ 4373.153389]  ucsi_unregister_partner.part.0+0x17/0x30 [typec_ucsi]
> >> [ 4373.153400]  ucsi_handle_connector_change+0x25c/0x320 [typec_ucsi]
> >> [ 4373.153418]  process_one_work+0x1df/0x3d0
> >> [ 4373.153428]  worker_thread+0x4a/0x3d0
> >> [ 4373.153436]  ? process_one_work+0x3d0/0x3d0
> >> [ 4373.153444]  kthread+0x127/0x170
> >> [ 4373.153451]  ? kthread_park+0x90/0x90
> >> [ 4373.153461]  ret_from_fork+0x1f/0x30
> >> [ 4373.153661] CR2: 00000000000002f2
> >>
> >> 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 d0c63afaf..79061705e 100644
> >> --- a/drivers/usb/typec/ucsi/ucsi.c
> >> +++ b/drivers/usb/typec/ucsi/ucsi.c
> >> @@ -479,7 +480,10 @@ static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient)
> >>  		return;
> >>  	}
> >>  
> >> -	while (adev[i]) {
> >> +	for (i = 0; i < UCSI_MAX_ALTMODES; i++) {
> >> +		if (!adev[i])
> >> +			break;
> > 
> >         for (i = 0; i < UCSI_MAX_ALTMODES && adev[u]; i++) {
> 
> make that                                      adev[i]

Agreed, that does read better than the break.

	Zwane

Re: [PATCH v5 1/2] usb/typec: fix array overruns in ucsi.c partner_altmode[]

[Zwane Mwaikambo]

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

 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

Re: [PATCH v5 1/2] usb/typec: fix array overruns in ucsi.c partner_altmode[]

[Heikki Krogerus]

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,

-- 
heikki

Re: [PATCH v5 1/2] usb/typec: fix array overruns in ucsi.c partner_altmode[]

[Zwane Mwaikambo]

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

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

Re: [PATCH v5 1/2] usb/typec: fix array overruns in ucsi.c partner_altmode[]

[Heikki Krogerus]

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

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,

-- 
heikki

[-- Attachment #2: 0001-usb-typec-ucsi-Prevent-mode-overrun.patch --]
[-- Type: text/plain, Size: 2329 bytes --]

From 50506dd61c6a086cb883e429312f4c6f0809e379 Mon Sep 17 00:00:00 2001
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Date: Mon, 14 Sep 2020 16:22:07 +0300
Subject: [PATCH] usb: typec: ucsi: Prevent mode overrun

Sometimes the embedded controller firmware does not
terminate the list of alternate modes that the partner
supports in its response to the GET_ALTERNATE_MODES command.
Instead the firmware returns the supported alternate modes
over and over again until the driver stops requesting them.

If that happens, the number of modes for each alternate mode
will exceed the maximum 6 that is defined in the USB Power
Delivery specification. Making sure that can't happen by
adding a check for it.

Not-Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/ucsi/ucsi.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index e680fcfdee609..758b988ac518a 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -216,14 +216,18 @@ void ucsi_altmode_update_active(struct ucsi_connector *con)
 					    con->partner_altmode[i] == altmode);
 }
 
-static u8 ucsi_altmode_next_mode(struct typec_altmode **alt, u16 svid)
+static int ucsi_altmode_next_mode(struct typec_altmode **alt, u16 svid)
 {
 	u8 mode = 1;
 	int i;
 
-	for (i = 0; alt[i]; i++)
+	for (i = 0; alt[i]; i++) {
+		if (i > MODE_DISCOVERY_MAX)
+			return -ERANGE;
+
 		if (alt[i]->svid == svid)
 			mode++;
+	}
 
 	return mode;
 }
@@ -258,8 +262,11 @@ static int ucsi_register_altmode(struct ucsi_connector *con,
 			goto err;
 		}
 
-		desc->mode = ucsi_altmode_next_mode(con->port_altmode,
-						    desc->svid);
+		ret = ucsi_altmode_next_mode(con->port_altmode, desc->svid);
+		if (ret < 0)
+			return ret;
+
+		desc->mode = ret;
 
 		switch (desc->svid) {
 		case USB_TYPEC_DP_SID:
@@ -292,8 +299,11 @@ static int ucsi_register_altmode(struct ucsi_connector *con,
 			goto err;
 		}
 
-		desc->mode = ucsi_altmode_next_mode(con->partner_altmode,
-						    desc->svid);
+		ret = ucsi_altmode_next_mode(con->partner_altmode, desc->svid);
+		if (ret < 0)
+			return ret;
+
+		desc->mode = ret;
 
 		alt = typec_partner_register_altmode(con->partner, desc);
 		if (IS_ERR(alt)) {
-- 
2.28.0

Re: [PATCH v5 1/2] usb/typec: fix array overruns in ucsi.c partner_altmode[]

[Zwane Mwaikambo]

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

Re: [PATCH v5 1/2] usb/typec: fix array overruns in ucsi.c partner_altmode[]

[Heikki Krogerus]

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

Re: [PATCH v5 1/2] usb/typec: fix array overruns in ucsi.c partner_altmode[]

[Zwane Mwaikambo]

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