linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH] usb: typec: tcpm: Ignore unsupported/unknown alternate mode requests
@ 2019-07-25  4:30 Guenter Roeck
  2019-07-26 15:21 ` Douglas Gilbert
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Guenter Roeck @ 2019-07-25  4:30 UTC (permalink / raw)
  To: Heikki Krogerus, Greg Kroah-Hartman
  Cc: linux-usb, linux-kernel, Guenter Roeck, Douglas Gilbert

TCPM may receive PD messages associated with unknown or unsupported
alternate modes. If that happens, calls to typec_match_altmode()
will return NULL. The tcpm code does not currently take this into
account. This results in crashes.

Unable to handle kernel NULL pointer dereference at virtual address 000001f0
pgd = 41dad9a1
[000001f0] *pgd=00000000
Internal error: Oops: 5 [#1] THUMB2
Modules linked in: tcpci tcpm
CPU: 0 PID: 2338 Comm: kworker/u2:0 Not tainted 5.1.18-sama5-armv7-r2 #6
Hardware name: Atmel SAMA5
Workqueue: 2-0050 tcpm_pd_rx_handler [tcpm]
PC is at typec_altmode_attention+0x0/0x14
LR is at tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm]
...
[<c03fbee8>] (typec_altmode_attention) from [<bf8030fb>]
				(tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm])
[<bf8030fb>] (tcpm_pd_rx_handler [tcpm]) from [<c012082b>]
				(process_one_work+0x123/0x2a8)
[<c012082b>] (process_one_work) from [<c0120a6d>]
				(worker_thread+0xbd/0x3b0)
[<c0120a6d>] (worker_thread) from [<c012431f>] (kthread+0xcf/0xf4)
[<c012431f>] (kthread) from [<c01010f9>] (ret_from_fork+0x11/0x38)

Ignore PD messages if the asociated alternate mode is not supported.

Reported-by: Douglas Gilbert <dgilbert@interlog.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Fixes: e9576fe8e605c ("usb: typec: tcpm: Support for Alternate Modes")
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
Taking a stab at the problem. I don't really know if this is the correct
fix, or even if my understanding of the problem is correct, thus marking
the patch as RFC.

 drivers/usb/typec/tcpm/tcpm.c | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 77f71f602f73..1df2844469aa 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -1096,7 +1096,8 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt,
 			break;
 		case CMD_ATTENTION:
 			/* Attention command does not have response */
-			typec_altmode_attention(adev, p[1]);
+			if (adev)
+				typec_altmode_attention(adev, p[1]);
 			return 0;
 		default:
 			break;
@@ -1148,20 +1149,26 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt,
 			}
 			break;
 		case CMD_ENTER_MODE:
-			typec_altmode_update_active(pdev, true);
-
-			if (typec_altmode_vdm(adev, p[0], &p[1], cnt)) {
-				response[0] = VDO(adev->svid, 1, CMD_EXIT_MODE);
-				response[0] |= VDO_OPOS(adev->mode);
-				return 1;
+			if (adev && pdev) {
+				typec_altmode_update_active(pdev, true);
+
+				if (typec_altmode_vdm(adev, p[0], &p[1], cnt)) {
+					response[0] = VDO(adev->svid, 1,
+							  CMD_EXIT_MODE);
+					response[0] |= VDO_OPOS(adev->mode);
+					return 1;
+				}
 			}
 			return 0;
 		case CMD_EXIT_MODE:
-			typec_altmode_update_active(pdev, false);
+			if (adev && pdev) {
+				typec_altmode_update_active(pdev, false);
 
-			/* Back to USB Operation */
-			WARN_ON(typec_altmode_notify(adev, TYPEC_STATE_USB,
-						     NULL));
+				/* Back to USB Operation */
+				WARN_ON(typec_altmode_notify(adev,
+							     TYPEC_STATE_USB,
+							     NULL));
+			}
 			break;
 		default:
 			break;
@@ -1171,8 +1178,10 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt,
 		switch (cmd) {
 		case CMD_ENTER_MODE:
 			/* Back to USB Operation */
-			WARN_ON(typec_altmode_notify(adev, TYPEC_STATE_USB,
-						     NULL));
+			if (adev)
+				WARN_ON(typec_altmode_notify(adev,
+							     TYPEC_STATE_USB,
+							     NULL));
 			break;
 		default:
 			break;
@@ -1183,7 +1192,8 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt,
 	}
 
 	/* Informing the alternate mode drivers about everything */
-	typec_altmode_vdm(adev, p[0], &p[1], cnt);
+	if (adev)
+		typec_altmode_vdm(adev, p[0], &p[1], cnt);
 
 	return rlen;
 }
-- 
2.7.4


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC PATCH] usb: typec: tcpm: Ignore unsupported/unknown alternate mode requests
  2019-07-25  4:30 [RFC PATCH] usb: typec: tcpm: Ignore unsupported/unknown alternate mode requests Guenter Roeck
@ 2019-07-26 15:21 ` Douglas Gilbert
  2019-07-29 14:04 ` Heikki Krogerus
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 12+ messages in thread
From: Douglas Gilbert @ 2019-07-26 15:21 UTC (permalink / raw)
  To: Guenter Roeck, Heikki Krogerus, Greg Kroah-Hartman
  Cc: linux-usb, linux-kernel

On 2019-07-25 12:30 a.m., Guenter Roeck wrote:
> TCPM may receive PD messages associated with unknown or unsupported
> alternate modes. If that happens, calls to typec_match_altmode()
> will return NULL. The tcpm code does not currently take this into
> account. This results in crashes.
> 
> Unable to handle kernel NULL pointer dereference at virtual address 000001f0
> pgd = 41dad9a1
> [000001f0] *pgd=00000000
> Internal error: Oops: 5 [#1] THUMB2
> Modules linked in: tcpci tcpm
> CPU: 0 PID: 2338 Comm: kworker/u2:0 Not tainted 5.1.18-sama5-armv7-r2 #6
> Hardware name: Atmel SAMA5
> Workqueue: 2-0050 tcpm_pd_rx_handler [tcpm]
> PC is at typec_altmode_attention+0x0/0x14
> LR is at tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm]
> ...
> [<c03fbee8>] (typec_altmode_attention) from [<bf8030fb>]
> 				(tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm])
> [<bf8030fb>] (tcpm_pd_rx_handler [tcpm]) from [<c012082b>]
> 				(process_one_work+0x123/0x2a8)
> [<c012082b>] (process_one_work) from [<c0120a6d>]
> 				(worker_thread+0xbd/0x3b0)
> [<c0120a6d>] (worker_thread) from [<c012431f>] (kthread+0xcf/0xf4)
> [<c012431f>] (kthread) from [<c01010f9>] (ret_from_fork+0x11/0x38)
> 
> Ignore PD messages if the asociated alternate mode is not supported.
> 
> Reported-by: Douglas Gilbert <dgilbert@interlog.com>
> Cc: Douglas Gilbert <dgilbert@interlog.com>
> Fixes: e9576fe8e605c ("usb: typec: tcpm: Support for Alternate Modes")
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> Taking a stab at the problem. I don't really know if this is the correct
> fix, or even if my understanding of the problem is correct, thus marking
> the patch as RFC.

Tested-by: Douglas Gilbert <dgilbert@interlog.com>

Took a little while because I was changing my Linux end to being based
on a Acme Systems Arietta (at91sam9g25). Reason: on the sama5d2xplained
I can't have the shield plugged into the Arduino sockets because it
drives the pin used by the OM13588 for its interrupt (nalert). So I
need to "jump" the pins needed (SDA, SCL, nALERT, 3v3 and ground) to
get this working (and 'cat /proc/interrupts' shows me its working
with >0 interrupts on "2-0050").

Anyway the DUT is a OM13588+sama5d2xplained running lk 5.1.18 and the
other end of the USB-C cable is a OM13588+KL27Z running NXP PD
software. Both ends can "play" sink or source (power). The oops
occurred when the DUT was source and the other end sent a "send
attention" ("l") command. With this patch I do not see the oops.


BTW I have tried to run lk 5.2.1 on the sama5d2xplained and the
kernel wouldn't start :-( Zilch after u-boot said it was loading
the kernel.

>   drivers/usb/typec/tcpm/tcpm.c | 38 ++++++++++++++++++++++++--------------
>   1 file changed, 24 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 77f71f602f73..1df2844469aa 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -1096,7 +1096,8 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt,
>   			break;
>   		case CMD_ATTENTION:
>   			/* Attention command does not have response */
> -			typec_altmode_attention(adev, p[1]);
> +			if (adev)
> +				typec_altmode_attention(adev, p[1]);
>   			return 0;
>   		default:
>   			break;
> @@ -1148,20 +1149,26 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt,
>   			}
>   			break;
>   		case CMD_ENTER_MODE:
> -			typec_altmode_update_active(pdev, true);
> -
> -			if (typec_altmode_vdm(adev, p[0], &p[1], cnt)) {
> -				response[0] = VDO(adev->svid, 1, CMD_EXIT_MODE);
> -				response[0] |= VDO_OPOS(adev->mode);
> -				return 1;
> +			if (adev && pdev) {
> +				typec_altmode_update_active(pdev, true);
> +
> +				if (typec_altmode_vdm(adev, p[0], &p[1], cnt)) {
> +					response[0] = VDO(adev->svid, 1,
> +							  CMD_EXIT_MODE);
> +					response[0] |= VDO_OPOS(adev->mode);
> +					return 1;
> +				}
>   			}
>   			return 0;
>   		case CMD_EXIT_MODE:
> -			typec_altmode_update_active(pdev, false);
> +			if (adev && pdev) {
> +				typec_altmode_update_active(pdev, false);
>   
> -			/* Back to USB Operation */
> -			WARN_ON(typec_altmode_notify(adev, TYPEC_STATE_USB,
> -						     NULL));
> +				/* Back to USB Operation */
> +				WARN_ON(typec_altmode_notify(adev,
> +							     TYPEC_STATE_USB,
> +							     NULL));
> +			}
>   			break;
>   		default:
>   			break;
> @@ -1171,8 +1178,10 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt,
>   		switch (cmd) {
>   		case CMD_ENTER_MODE:
>   			/* Back to USB Operation */
> -			WARN_ON(typec_altmode_notify(adev, TYPEC_STATE_USB,
> -						     NULL));
> +			if (adev)
> +				WARN_ON(typec_altmode_notify(adev,
> +							     TYPEC_STATE_USB,
> +							     NULL));
>   			break;
>   		default:
>   			break;
> @@ -1183,7 +1192,8 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt,
>   	}
>   
>   	/* Informing the alternate mode drivers about everything */
> -	typec_altmode_vdm(adev, p[0], &p[1], cnt);
> +	if (adev)
> +		typec_altmode_vdm(adev, p[0], &p[1], cnt);
>   
>   	return rlen;
>   }
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC PATCH] usb: typec: tcpm: Ignore unsupported/unknown alternate mode requests
  2019-07-25  4:30 [RFC PATCH] usb: typec: tcpm: Ignore unsupported/unknown alternate mode requests Guenter Roeck
  2019-07-26 15:21 ` Douglas Gilbert
@ 2019-07-29 14:04 ` Heikki Krogerus
  2019-07-29 17:31   ` Guenter Roeck
  2019-08-02 11:42 ` Greg Kroah-Hartman
  2019-08-02 15:25 ` Douglas Gilbert
  3 siblings, 1 reply; 12+ messages in thread
From: Heikki Krogerus @ 2019-07-29 14:04 UTC (permalink / raw)
  To: Guenter Roeck, Douglas Gilbert
  Cc: Greg Kroah-Hartman, linux-usb, linux-kernel

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

Hi,

On Wed, Jul 24, 2019 at 09:30:37PM -0700, Guenter Roeck wrote:
> TCPM may receive PD messages associated with unknown or unsupported
> alternate modes. If that happens, calls to typec_match_altmode()
> will return NULL. The tcpm code does not currently take this into
> account. This results in crashes.
> 
> Unable to handle kernel NULL pointer dereference at virtual address 000001f0
> pgd = 41dad9a1
> [000001f0] *pgd=00000000
> Internal error: Oops: 5 [#1] THUMB2
> Modules linked in: tcpci tcpm
> CPU: 0 PID: 2338 Comm: kworker/u2:0 Not tainted 5.1.18-sama5-armv7-r2 #6
> Hardware name: Atmel SAMA5
> Workqueue: 2-0050 tcpm_pd_rx_handler [tcpm]
> PC is at typec_altmode_attention+0x0/0x14
> LR is at tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm]
> ...
> [<c03fbee8>] (typec_altmode_attention) from [<bf8030fb>]
> 				(tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm])
> [<bf8030fb>] (tcpm_pd_rx_handler [tcpm]) from [<c012082b>]
> 				(process_one_work+0x123/0x2a8)
> [<c012082b>] (process_one_work) from [<c0120a6d>]
> 				(worker_thread+0xbd/0x3b0)
> [<c0120a6d>] (worker_thread) from [<c012431f>] (kthread+0xcf/0xf4)
> [<c012431f>] (kthread) from [<c01010f9>] (ret_from_fork+0x11/0x38)
> 
> Ignore PD messages if the asociated alternate mode is not supported.
> 
> Reported-by: Douglas Gilbert <dgilbert@interlog.com>
> Cc: Douglas Gilbert <dgilbert@interlog.com>
> Fixes: e9576fe8e605c ("usb: typec: tcpm: Support for Alternate Modes")
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> Taking a stab at the problem. I don't really know if this is the correct
> fix, or even if my understanding of the problem is correct, thus marking
> the patch as RFC.

My guess is that typec_match_altmode() is the real culprit. We can't
rely on the partner mode index number when identifying the port alt
mode.

Douglas, can you test the attached hack instead of this patch?


thanks,

-- 
heikki

[-- Attachment #2: tcpm_hack.diff --]
[-- Type: text/plain, Size: 676 bytes --]

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index ec525811a9eb..033dc097ba83 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -1067,12 +1067,11 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt,
 
 	modep = &port->mode_data;
 
-	adev = typec_match_altmode(port->port_altmode, ALTMODE_DISCOVERY_MAX,
-				   PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0]));
-
 	pdev = typec_match_altmode(port->partner_altmode, ALTMODE_DISCOVERY_MAX,
 				   PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0]));
 
+	adev = (void *)typec_altmode_get_partner(pdev);
+
 	switch (cmd_type) {
 	case CMDT_INIT:
 		switch (cmd) {

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC PATCH] usb: typec: tcpm: Ignore unsupported/unknown alternate mode requests
  2019-07-29 14:04 ` Heikki Krogerus
@ 2019-07-29 17:31   ` Guenter Roeck
  2019-07-30 12:07     ` Heikki Krogerus
  0 siblings, 1 reply; 12+ messages in thread
From: Guenter Roeck @ 2019-07-29 17:31 UTC (permalink / raw)
  To: Heikki Krogerus
  Cc: Douglas Gilbert, Greg Kroah-Hartman, linux-usb, linux-kernel

On Mon, Jul 29, 2019 at 05:04:57PM +0300, Heikki Krogerus wrote:
> Hi,
> 
> On Wed, Jul 24, 2019 at 09:30:37PM -0700, Guenter Roeck wrote:
> > TCPM may receive PD messages associated with unknown or unsupported
> > alternate modes. If that happens, calls to typec_match_altmode()
> > will return NULL. The tcpm code does not currently take this into
> > account. This results in crashes.
> > 
> > Unable to handle kernel NULL pointer dereference at virtual address 000001f0
> > pgd = 41dad9a1
> > [000001f0] *pgd=00000000
> > Internal error: Oops: 5 [#1] THUMB2
> > Modules linked in: tcpci tcpm
> > CPU: 0 PID: 2338 Comm: kworker/u2:0 Not tainted 5.1.18-sama5-armv7-r2 #6
> > Hardware name: Atmel SAMA5
> > Workqueue: 2-0050 tcpm_pd_rx_handler [tcpm]
> > PC is at typec_altmode_attention+0x0/0x14
> > LR is at tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm]
> > ...
> > [<c03fbee8>] (typec_altmode_attention) from [<bf8030fb>]
> > 				(tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm])
> > [<bf8030fb>] (tcpm_pd_rx_handler [tcpm]) from [<c012082b>]
> > 				(process_one_work+0x123/0x2a8)
> > [<c012082b>] (process_one_work) from [<c0120a6d>]
> > 				(worker_thread+0xbd/0x3b0)
> > [<c0120a6d>] (worker_thread) from [<c012431f>] (kthread+0xcf/0xf4)
> > [<c012431f>] (kthread) from [<c01010f9>] (ret_from_fork+0x11/0x38)
> > 
> > Ignore PD messages if the asociated alternate mode is not supported.
> > 
> > Reported-by: Douglas Gilbert <dgilbert@interlog.com>
> > Cc: Douglas Gilbert <dgilbert@interlog.com>
> > Fixes: e9576fe8e605c ("usb: typec: tcpm: Support for Alternate Modes")
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> > Taking a stab at the problem. I don't really know if this is the correct
> > fix, or even if my understanding of the problem is correct, thus marking
> > the patch as RFC.
> 
> My guess is that typec_match_altmode() is the real culprit. We can't
> rely on the partner mode index number when identifying the port alt
> mode.
> 
> Douglas, can you test the attached hack instead of this patch?
> 
> 
> thanks,
> 
> -- 
> heikki

> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index ec525811a9eb..033dc097ba83 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -1067,12 +1067,11 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt,
>  
>  	modep = &port->mode_data;
>  
> -	adev = typec_match_altmode(port->port_altmode, ALTMODE_DISCOVERY_MAX,
> -				   PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0]));
> -
>  	pdev = typec_match_altmode(port->partner_altmode, ALTMODE_DISCOVERY_MAX,
>  				   PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0]));
>  
> +	adev = (void *)typec_altmode_get_partner(pdev);
> +

I understand that typec_altmode_get_partner() returns a const *;
maybe adev should be declared as const struct typec_altmode *
instead of using a typecast.

Also, typec_altmode_get_partner() can return NULL as well if pdev is NULL.
Is it guaranteed that typec_match_altmode() never returns NULL for pdev ?

Thanks,
Guenter

>  	switch (cmd_type) {
>  	case CMDT_INIT:
>  		switch (cmd) {


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC PATCH] usb: typec: tcpm: Ignore unsupported/unknown alternate mode requests
  2019-07-29 17:31   ` Guenter Roeck
@ 2019-07-30 12:07     ` Heikki Krogerus
  2019-07-30 12:17       ` Douglas Gilbert
  2019-07-30 13:28       ` Guenter Roeck
  0 siblings, 2 replies; 12+ messages in thread
From: Heikki Krogerus @ 2019-07-30 12:07 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Douglas Gilbert, Greg Kroah-Hartman, linux-usb, linux-kernel

On Mon, Jul 29, 2019 at 10:31:04AM -0700, Guenter Roeck wrote:
> On Mon, Jul 29, 2019 at 05:04:57PM +0300, Heikki Krogerus wrote:
> > Hi,
> > 
> > On Wed, Jul 24, 2019 at 09:30:37PM -0700, Guenter Roeck wrote:
> > > TCPM may receive PD messages associated with unknown or unsupported
> > > alternate modes. If that happens, calls to typec_match_altmode()
> > > will return NULL. The tcpm code does not currently take this into
> > > account. This results in crashes.
> > > 
> > > Unable to handle kernel NULL pointer dereference at virtual address 000001f0
> > > pgd = 41dad9a1
> > > [000001f0] *pgd=00000000
> > > Internal error: Oops: 5 [#1] THUMB2
> > > Modules linked in: tcpci tcpm
> > > CPU: 0 PID: 2338 Comm: kworker/u2:0 Not tainted 5.1.18-sama5-armv7-r2 #6
> > > Hardware name: Atmel SAMA5
> > > Workqueue: 2-0050 tcpm_pd_rx_handler [tcpm]
> > > PC is at typec_altmode_attention+0x0/0x14
> > > LR is at tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm]
> > > ...
> > > [<c03fbee8>] (typec_altmode_attention) from [<bf8030fb>]
> > > 				(tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm])
> > > [<bf8030fb>] (tcpm_pd_rx_handler [tcpm]) from [<c012082b>]
> > > 				(process_one_work+0x123/0x2a8)
> > > [<c012082b>] (process_one_work) from [<c0120a6d>]
> > > 				(worker_thread+0xbd/0x3b0)
> > > [<c0120a6d>] (worker_thread) from [<c012431f>] (kthread+0xcf/0xf4)
> > > [<c012431f>] (kthread) from [<c01010f9>] (ret_from_fork+0x11/0x38)
> > > 
> > > Ignore PD messages if the asociated alternate mode is not supported.
> > > 
> > > Reported-by: Douglas Gilbert <dgilbert@interlog.com>
> > > Cc: Douglas Gilbert <dgilbert@interlog.com>
> > > Fixes: e9576fe8e605c ("usb: typec: tcpm: Support for Alternate Modes")
> > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > ---
> > > Taking a stab at the problem. I don't really know if this is the correct
> > > fix, or even if my understanding of the problem is correct, thus marking
> > > the patch as RFC.
> > 
> > My guess is that typec_match_altmode() is the real culprit. We can't
> > rely on the partner mode index number when identifying the port alt
> > mode.
> > 
> > Douglas, can you test the attached hack instead of this patch?
> > 
> > 
> > thanks,
> > 
> > -- 
> > heikki
> 
> > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> > index ec525811a9eb..033dc097ba83 100644
> > --- a/drivers/usb/typec/tcpm/tcpm.c
> > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > @@ -1067,12 +1067,11 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt,
> >  
> >  	modep = &port->mode_data;
> >  
> > -	adev = typec_match_altmode(port->port_altmode, ALTMODE_DISCOVERY_MAX,
> > -				   PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0]));
> > -
> >  	pdev = typec_match_altmode(port->partner_altmode, ALTMODE_DISCOVERY_MAX,
> >  				   PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0]));
> >  
> > +	adev = (void *)typec_altmode_get_partner(pdev);
> > +
> 
> I understand that typec_altmode_get_partner() returns a const *;
> maybe adev should be declared as const struct typec_altmode *
> instead of using a typecast.

Yes...

> Also, typec_altmode_get_partner() can return NULL as well if pdev is NULL.
> Is it guaranteed that typec_match_altmode() never returns NULL for pdev ?

...and probable no. But I don't think we can receive Attention to a
mode that hasn't been entered.

I'm not proposing that as a patch. It's just a hunch. That's why I'm
calling it a "hack". Before we prepare anything finalized, I would
like to here from Douglas if he's able to test that or not?


thanks,

-- 
heikki

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC PATCH] usb: typec: tcpm: Ignore unsupported/unknown alternate mode requests
  2019-07-30 12:07     ` Heikki Krogerus
@ 2019-07-30 12:17       ` Douglas Gilbert
  2019-07-30 13:28       ` Guenter Roeck
  1 sibling, 0 replies; 12+ messages in thread
From: Douglas Gilbert @ 2019-07-30 12:17 UTC (permalink / raw)
  To: Heikki Krogerus, Guenter Roeck
  Cc: Greg Kroah-Hartman, linux-usb, linux-kernel

On 2019-07-30 2:07 p.m., Heikki Krogerus wrote:
> On Mon, Jul 29, 2019 at 10:31:04AM -0700, Guenter Roeck wrote:
>> On Mon, Jul 29, 2019 at 05:04:57PM +0300, Heikki Krogerus wrote:
>>> Hi,
>>>
>>> On Wed, Jul 24, 2019 at 09:30:37PM -0700, Guenter Roeck wrote:
>>>> TCPM may receive PD messages associated with unknown or unsupported
>>>> alternate modes. If that happens, calls to typec_match_altmode()
>>>> will return NULL. The tcpm code does not currently take this into
>>>> account. This results in crashes.
>>>>
>>>> Unable to handle kernel NULL pointer dereference at virtual address 000001f0
>>>> pgd = 41dad9a1
>>>> [000001f0] *pgd=00000000
>>>> Internal error: Oops: 5 [#1] THUMB2
>>>> Modules linked in: tcpci tcpm
>>>> CPU: 0 PID: 2338 Comm: kworker/u2:0 Not tainted 5.1.18-sama5-armv7-r2 #6
>>>> Hardware name: Atmel SAMA5
>>>> Workqueue: 2-0050 tcpm_pd_rx_handler [tcpm]
>>>> PC is at typec_altmode_attention+0x0/0x14
>>>> LR is at tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm]
>>>> ...
>>>> [<c03fbee8>] (typec_altmode_attention) from [<bf8030fb>]
>>>> 				(tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm])
>>>> [<bf8030fb>] (tcpm_pd_rx_handler [tcpm]) from [<c012082b>]
>>>> 				(process_one_work+0x123/0x2a8)
>>>> [<c012082b>] (process_one_work) from [<c0120a6d>]
>>>> 				(worker_thread+0xbd/0x3b0)
>>>> [<c0120a6d>] (worker_thread) from [<c012431f>] (kthread+0xcf/0xf4)
>>>> [<c012431f>] (kthread) from [<c01010f9>] (ret_from_fork+0x11/0x38)
>>>>
>>>> Ignore PD messages if the asociated alternate mode is not supported.
>>>>
>>>> Reported-by: Douglas Gilbert <dgilbert@interlog.com>
>>>> Cc: Douglas Gilbert <dgilbert@interlog.com>
>>>> Fixes: e9576fe8e605c ("usb: typec: tcpm: Support for Alternate Modes")
>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>> ---
>>>> Taking a stab at the problem. I don't really know if this is the correct
>>>> fix, or even if my understanding of the problem is correct, thus marking
>>>> the patch as RFC.
>>>
>>> My guess is that typec_match_altmode() is the real culprit. We can't
>>> rely on the partner mode index number when identifying the port alt
>>> mode.
>>>
>>> Douglas, can you test the attached hack instead of this patch?
>>>
>>>
>>> thanks,
>>>
>>> -- 
>>> heikki
>>
>>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
>>> index ec525811a9eb..033dc097ba83 100644
>>> --- a/drivers/usb/typec/tcpm/tcpm.c
>>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>>> @@ -1067,12 +1067,11 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt,
>>>   
>>>   	modep = &port->mode_data;
>>>   
>>> -	adev = typec_match_altmode(port->port_altmode, ALTMODE_DISCOVERY_MAX,
>>> -				   PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0]));
>>> -
>>>   	pdev = typec_match_altmode(port->partner_altmode, ALTMODE_DISCOVERY_MAX,
>>>   				   PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0]));
>>>   
>>> +	adev = (void *)typec_altmode_get_partner(pdev);
>>> +
>>
>> I understand that typec_altmode_get_partner() returns a const *;
>> maybe adev should be declared as const struct typec_altmode *
>> instead of using a typecast.
> 
> Yes...
> 
>> Also, typec_altmode_get_partner() can return NULL as well if pdev is NULL.
>> Is it guaranteed that typec_match_altmode() never returns NULL for pdev ?
> 
> ...and probable no. But I don't think we can receive Attention to a
> mode that hasn't been entered.
> 
> I'm not proposing that as a patch. It's just a hunch. That's why I'm
> calling it a "hack". Before we prepare anything finalized, I would
> like to here from Douglas if he's able to test that or not?

Hi,
I'm an ocean away from my test rig at the moment, won't be back home till
after August 10. Maybe I can set up something here as I have one OM13588.
Will try later today. Also, switching from an sama5d2_xplained to an Acme
Systems Arietta which is at91sam9g25 based to run Linux. So it won't be
exactly the same hardware.

Doug Gilbert


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC PATCH] usb: typec: tcpm: Ignore unsupported/unknown alternate mode requests
  2019-07-30 12:07     ` Heikki Krogerus
  2019-07-30 12:17       ` Douglas Gilbert
@ 2019-07-30 13:28       ` Guenter Roeck
  2019-07-31  9:55         ` Heikki Krogerus
  1 sibling, 1 reply; 12+ messages in thread
From: Guenter Roeck @ 2019-07-30 13:28 UTC (permalink / raw)
  To: Heikki Krogerus
  Cc: Douglas Gilbert, Greg Kroah-Hartman, linux-usb, linux-kernel

On 7/30/19 5:07 AM, Heikki Krogerus wrote:
> On Mon, Jul 29, 2019 at 10:31:04AM -0700, Guenter Roeck wrote:
>> On Mon, Jul 29, 2019 at 05:04:57PM +0300, Heikki Krogerus wrote:
>>> Hi,
>>>
>>> On Wed, Jul 24, 2019 at 09:30:37PM -0700, Guenter Roeck wrote:
>>>> TCPM may receive PD messages associated with unknown or unsupported
>>>> alternate modes. If that happens, calls to typec_match_altmode()
>>>> will return NULL. The tcpm code does not currently take this into
>>>> account. This results in crashes.
>>>>
>>>> Unable to handle kernel NULL pointer dereference at virtual address 000001f0
>>>> pgd = 41dad9a1
>>>> [000001f0] *pgd=00000000
>>>> Internal error: Oops: 5 [#1] THUMB2
>>>> Modules linked in: tcpci tcpm
>>>> CPU: 0 PID: 2338 Comm: kworker/u2:0 Not tainted 5.1.18-sama5-armv7-r2 #6
>>>> Hardware name: Atmel SAMA5
>>>> Workqueue: 2-0050 tcpm_pd_rx_handler [tcpm]
>>>> PC is at typec_altmode_attention+0x0/0x14
>>>> LR is at tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm]
>>>> ...
>>>> [<c03fbee8>] (typec_altmode_attention) from [<bf8030fb>]
>>>> 				(tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm])
>>>> [<bf8030fb>] (tcpm_pd_rx_handler [tcpm]) from [<c012082b>]
>>>> 				(process_one_work+0x123/0x2a8)
>>>> [<c012082b>] (process_one_work) from [<c0120a6d>]
>>>> 				(worker_thread+0xbd/0x3b0)
>>>> [<c0120a6d>] (worker_thread) from [<c012431f>] (kthread+0xcf/0xf4)
>>>> [<c012431f>] (kthread) from [<c01010f9>] (ret_from_fork+0x11/0x38)
>>>>
>>>> Ignore PD messages if the asociated alternate mode is not supported.
>>>>
>>>> Reported-by: Douglas Gilbert <dgilbert@interlog.com>
>>>> Cc: Douglas Gilbert <dgilbert@interlog.com>
>>>> Fixes: e9576fe8e605c ("usb: typec: tcpm: Support for Alternate Modes")
>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>> ---
>>>> Taking a stab at the problem. I don't really know if this is the correct
>>>> fix, or even if my understanding of the problem is correct, thus marking
>>>> the patch as RFC.
>>>
>>> My guess is that typec_match_altmode() is the real culprit. We can't
>>> rely on the partner mode index number when identifying the port alt
>>> mode.
>>>
>>> Douglas, can you test the attached hack instead of this patch?
>>>
>>>
>>> thanks,
>>>
>>> -- 
>>> heikki
>>
>>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
>>> index ec525811a9eb..033dc097ba83 100644
>>> --- a/drivers/usb/typec/tcpm/tcpm.c
>>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>>> @@ -1067,12 +1067,11 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt,
>>>   
>>>   	modep = &port->mode_data;
>>>   
>>> -	adev = typec_match_altmode(port->port_altmode, ALTMODE_DISCOVERY_MAX,
>>> -				   PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0]));
>>> -
>>>   	pdev = typec_match_altmode(port->partner_altmode, ALTMODE_DISCOVERY_MAX,
>>>   				   PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0]));
>>>   
>>> +	adev = (void *)typec_altmode_get_partner(pdev);
>>> +
>>
>> I understand that typec_altmode_get_partner() returns a const *;
>> maybe adev should be declared as const struct typec_altmode *
>> instead of using a typecast.
> 
> Yes...
> 
>> Also, typec_altmode_get_partner() can return NULL as well if pdev is NULL.
>> Is it guaranteed that typec_match_altmode() never returns NULL for pdev ?
> 
> ...and probable no. But I don't think we can receive Attention to a
> mode that hasn't been entered.
> 

If I understand correctly, the Attention was generated by a test system.
What prevents badly implemented code in the connected system from sending
such an Attention message ?

Thanks,
Guenter

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC PATCH] usb: typec: tcpm: Ignore unsupported/unknown alternate mode requests
  2019-07-30 13:28       ` Guenter Roeck
@ 2019-07-31  9:55         ` Heikki Krogerus
  2019-08-01 16:07           ` Greg Kroah-Hartman
  0 siblings, 1 reply; 12+ messages in thread
From: Heikki Krogerus @ 2019-07-31  9:55 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Douglas Gilbert, Greg Kroah-Hartman, linux-usb, linux-kernel

On Tue, Jul 30, 2019 at 06:28:52AM -0700, Guenter Roeck wrote:
> On 7/30/19 5:07 AM, Heikki Krogerus wrote:
> > On Mon, Jul 29, 2019 at 10:31:04AM -0700, Guenter Roeck wrote:
> > > On Mon, Jul 29, 2019 at 05:04:57PM +0300, Heikki Krogerus wrote:
> > > > Hi,
> > > > 
> > > > On Wed, Jul 24, 2019 at 09:30:37PM -0700, Guenter Roeck wrote:
> > > > > TCPM may receive PD messages associated with unknown or unsupported
> > > > > alternate modes. If that happens, calls to typec_match_altmode()
> > > > > will return NULL. The tcpm code does not currently take this into
> > > > > account. This results in crashes.
> > > > > 
> > > > > Unable to handle kernel NULL pointer dereference at virtual address 000001f0
> > > > > pgd = 41dad9a1
> > > > > [000001f0] *pgd=00000000
> > > > > Internal error: Oops: 5 [#1] THUMB2
> > > > > Modules linked in: tcpci tcpm
> > > > > CPU: 0 PID: 2338 Comm: kworker/u2:0 Not tainted 5.1.18-sama5-armv7-r2 #6
> > > > > Hardware name: Atmel SAMA5
> > > > > Workqueue: 2-0050 tcpm_pd_rx_handler [tcpm]
> > > > > PC is at typec_altmode_attention+0x0/0x14
> > > > > LR is at tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm]
> > > > > ...
> > > > > [<c03fbee8>] (typec_altmode_attention) from [<bf8030fb>]
> > > > > 				(tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm])
> > > > > [<bf8030fb>] (tcpm_pd_rx_handler [tcpm]) from [<c012082b>]
> > > > > 				(process_one_work+0x123/0x2a8)
> > > > > [<c012082b>] (process_one_work) from [<c0120a6d>]
> > > > > 				(worker_thread+0xbd/0x3b0)
> > > > > [<c0120a6d>] (worker_thread) from [<c012431f>] (kthread+0xcf/0xf4)
> > > > > [<c012431f>] (kthread) from [<c01010f9>] (ret_from_fork+0x11/0x38)
> > > > > 
> > > > > Ignore PD messages if the asociated alternate mode is not supported.
> > > > > 
> > > > > Reported-by: Douglas Gilbert <dgilbert@interlog.com>
> > > > > Cc: Douglas Gilbert <dgilbert@interlog.com>
> > > > > Fixes: e9576fe8e605c ("usb: typec: tcpm: Support for Alternate Modes")
> > > > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > > > ---
> > > > > Taking a stab at the problem. I don't really know if this is the correct
> > > > > fix, or even if my understanding of the problem is correct, thus marking
> > > > > the patch as RFC.
> > > > 
> > > > My guess is that typec_match_altmode() is the real culprit. We can't
> > > > rely on the partner mode index number when identifying the port alt
> > > > mode.
> > > > 
> > > > Douglas, can you test the attached hack instead of this patch?
> > > > 
> > > > 
> > > > thanks,
> > > > 
> > > > -- 
> > > > heikki
> > > 
> > > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> > > > index ec525811a9eb..033dc097ba83 100644
> > > > --- a/drivers/usb/typec/tcpm/tcpm.c
> > > > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > > > @@ -1067,12 +1067,11 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt,
> > > >   	modep = &port->mode_data;
> > > > -	adev = typec_match_altmode(port->port_altmode, ALTMODE_DISCOVERY_MAX,
> > > > -				   PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0]));
> > > > -
> > > >   	pdev = typec_match_altmode(port->partner_altmode, ALTMODE_DISCOVERY_MAX,
> > > >   				   PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0]));
> > > > +	adev = (void *)typec_altmode_get_partner(pdev);
> > > > +
> > > 
> > > I understand that typec_altmode_get_partner() returns a const *;
> > > maybe adev should be declared as const struct typec_altmode *
> > > instead of using a typecast.
> > 
> > Yes...
> > 
> > > Also, typec_altmode_get_partner() can return NULL as well if pdev is NULL.
> > > Is it guaranteed that typec_match_altmode() never returns NULL for pdev ?
> > 
> > ...and probable no. But I don't think we can receive Attention to a
> > mode that hasn't been entered.
> > 
> 
> If I understand correctly, the Attention was generated by a test system.
> What prevents badly implemented code in the connected system from sending
> such an Attention message ?

Oh, if that is the case, then I don't think my change has any effect.
I misunderstood the scenario. Sorry for that.

I think we should use your patch to fix this issue.

thanks,

-- 
heikki

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC PATCH] usb: typec: tcpm: Ignore unsupported/unknown alternate mode requests
  2019-07-31  9:55         ` Heikki Krogerus
@ 2019-08-01 16:07           ` Greg Kroah-Hartman
  2019-08-02  8:01             ` Heikki Krogerus
  0 siblings, 1 reply; 12+ messages in thread
From: Greg Kroah-Hartman @ 2019-08-01 16:07 UTC (permalink / raw)
  To: Heikki Krogerus; +Cc: Guenter Roeck, Douglas Gilbert, linux-usb, linux-kernel

On Wed, Jul 31, 2019 at 12:55:55PM +0300, Heikki Krogerus wrote:
> On Tue, Jul 30, 2019 at 06:28:52AM -0700, Guenter Roeck wrote:
> > On 7/30/19 5:07 AM, Heikki Krogerus wrote:
> > > On Mon, Jul 29, 2019 at 10:31:04AM -0700, Guenter Roeck wrote:
> > > > On Mon, Jul 29, 2019 at 05:04:57PM +0300, Heikki Krogerus wrote:
> > > > > Hi,
> > > > > 
> > > > > On Wed, Jul 24, 2019 at 09:30:37PM -0700, Guenter Roeck wrote:
> > > > > > TCPM may receive PD messages associated with unknown or unsupported
> > > > > > alternate modes. If that happens, calls to typec_match_altmode()
> > > > > > will return NULL. The tcpm code does not currently take this into
> > > > > > account. This results in crashes.
> > > > > > 
> > > > > > Unable to handle kernel NULL pointer dereference at virtual address 000001f0
> > > > > > pgd = 41dad9a1
> > > > > > [000001f0] *pgd=00000000
> > > > > > Internal error: Oops: 5 [#1] THUMB2
> > > > > > Modules linked in: tcpci tcpm
> > > > > > CPU: 0 PID: 2338 Comm: kworker/u2:0 Not tainted 5.1.18-sama5-armv7-r2 #6
> > > > > > Hardware name: Atmel SAMA5
> > > > > > Workqueue: 2-0050 tcpm_pd_rx_handler [tcpm]
> > > > > > PC is at typec_altmode_attention+0x0/0x14
> > > > > > LR is at tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm]
> > > > > > ...
> > > > > > [<c03fbee8>] (typec_altmode_attention) from [<bf8030fb>]
> > > > > > 				(tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm])
> > > > > > [<bf8030fb>] (tcpm_pd_rx_handler [tcpm]) from [<c012082b>]
> > > > > > 				(process_one_work+0x123/0x2a8)
> > > > > > [<c012082b>] (process_one_work) from [<c0120a6d>]
> > > > > > 				(worker_thread+0xbd/0x3b0)
> > > > > > [<c0120a6d>] (worker_thread) from [<c012431f>] (kthread+0xcf/0xf4)
> > > > > > [<c012431f>] (kthread) from [<c01010f9>] (ret_from_fork+0x11/0x38)
> > > > > > 
> > > > > > Ignore PD messages if the asociated alternate mode is not supported.
> > > > > > 
> > > > > > Reported-by: Douglas Gilbert <dgilbert@interlog.com>
> > > > > > Cc: Douglas Gilbert <dgilbert@interlog.com>
> > > > > > Fixes: e9576fe8e605c ("usb: typec: tcpm: Support for Alternate Modes")
> > > > > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > > > > ---
> > > > > > Taking a stab at the problem. I don't really know if this is the correct
> > > > > > fix, or even if my understanding of the problem is correct, thus marking
> > > > > > the patch as RFC.
> > > > > 
> > > > > My guess is that typec_match_altmode() is the real culprit. We can't
> > > > > rely on the partner mode index number when identifying the port alt
> > > > > mode.
> > > > > 
> > > > > Douglas, can you test the attached hack instead of this patch?
> > > > > 
> > > > > 
> > > > > thanks,
> > > > > 
> > > > > -- 
> > > > > heikki
> > > > 
> > > > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> > > > > index ec525811a9eb..033dc097ba83 100644
> > > > > --- a/drivers/usb/typec/tcpm/tcpm.c
> > > > > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > > > > @@ -1067,12 +1067,11 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt,
> > > > >   	modep = &port->mode_data;
> > > > > -	adev = typec_match_altmode(port->port_altmode, ALTMODE_DISCOVERY_MAX,
> > > > > -				   PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0]));
> > > > > -
> > > > >   	pdev = typec_match_altmode(port->partner_altmode, ALTMODE_DISCOVERY_MAX,
> > > > >   				   PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0]));
> > > > > +	adev = (void *)typec_altmode_get_partner(pdev);
> > > > > +
> > > > 
> > > > I understand that typec_altmode_get_partner() returns a const *;
> > > > maybe adev should be declared as const struct typec_altmode *
> > > > instead of using a typecast.
> > > 
> > > Yes...
> > > 
> > > > Also, typec_altmode_get_partner() can return NULL as well if pdev is NULL.
> > > > Is it guaranteed that typec_match_altmode() never returns NULL for pdev ?
> > > 
> > > ...and probable no. But I don't think we can receive Attention to a
> > > mode that hasn't been entered.
> > > 
> > 
> > If I understand correctly, the Attention was generated by a test system.
> > What prevents badly implemented code in the connected system from sending
> > such an Attention message ?
> 
> Oh, if that is the case, then I don't think my change has any effect.
> I misunderstood the scenario. Sorry for that.
> 
> I think we should use your patch to fix this issue.

So is this an "ack"?  I'm confused as to if this patch should be applied
or not...

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC PATCH] usb: typec: tcpm: Ignore unsupported/unknown alternate mode requests
  2019-08-01 16:07           ` Greg Kroah-Hartman
@ 2019-08-02  8:01             ` Heikki Krogerus
  0 siblings, 0 replies; 12+ messages in thread
From: Heikki Krogerus @ 2019-08-02  8:01 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Guenter Roeck, Douglas Gilbert, linux-usb, linux-kernel

On Thu, Aug 01, 2019 at 06:07:09PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Jul 31, 2019 at 12:55:55PM +0300, Heikki Krogerus wrote:
> > On Tue, Jul 30, 2019 at 06:28:52AM -0700, Guenter Roeck wrote:
> > > On 7/30/19 5:07 AM, Heikki Krogerus wrote:
> > > > On Mon, Jul 29, 2019 at 10:31:04AM -0700, Guenter Roeck wrote:
> > > > > On Mon, Jul 29, 2019 at 05:04:57PM +0300, Heikki Krogerus wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > On Wed, Jul 24, 2019 at 09:30:37PM -0700, Guenter Roeck wrote:
> > > > > > > TCPM may receive PD messages associated with unknown or unsupported
> > > > > > > alternate modes. If that happens, calls to typec_match_altmode()
> > > > > > > will return NULL. The tcpm code does not currently take this into
> > > > > > > account. This results in crashes.
> > > > > > > 
> > > > > > > Unable to handle kernel NULL pointer dereference at virtual address 000001f0
> > > > > > > pgd = 41dad9a1
> > > > > > > [000001f0] *pgd=00000000
> > > > > > > Internal error: Oops: 5 [#1] THUMB2
> > > > > > > Modules linked in: tcpci tcpm
> > > > > > > CPU: 0 PID: 2338 Comm: kworker/u2:0 Not tainted 5.1.18-sama5-armv7-r2 #6
> > > > > > > Hardware name: Atmel SAMA5
> > > > > > > Workqueue: 2-0050 tcpm_pd_rx_handler [tcpm]
> > > > > > > PC is at typec_altmode_attention+0x0/0x14
> > > > > > > LR is at tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm]
> > > > > > > ...
> > > > > > > [<c03fbee8>] (typec_altmode_attention) from [<bf8030fb>]
> > > > > > > 				(tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm])
> > > > > > > [<bf8030fb>] (tcpm_pd_rx_handler [tcpm]) from [<c012082b>]
> > > > > > > 				(process_one_work+0x123/0x2a8)
> > > > > > > [<c012082b>] (process_one_work) from [<c0120a6d>]
> > > > > > > 				(worker_thread+0xbd/0x3b0)
> > > > > > > [<c0120a6d>] (worker_thread) from [<c012431f>] (kthread+0xcf/0xf4)
> > > > > > > [<c012431f>] (kthread) from [<c01010f9>] (ret_from_fork+0x11/0x38)
> > > > > > > 
> > > > > > > Ignore PD messages if the asociated alternate mode is not supported.
> > > > > > > 
> > > > > > > Reported-by: Douglas Gilbert <dgilbert@interlog.com>
> > > > > > > Cc: Douglas Gilbert <dgilbert@interlog.com>
> > > > > > > Fixes: e9576fe8e605c ("usb: typec: tcpm: Support for Alternate Modes")
> > > > > > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > > > > > ---
> > > > > > > Taking a stab at the problem. I don't really know if this is the correct
> > > > > > > fix, or even if my understanding of the problem is correct, thus marking
> > > > > > > the patch as RFC.
> > > > > > 
> > > > > > My guess is that typec_match_altmode() is the real culprit. We can't
> > > > > > rely on the partner mode index number when identifying the port alt
> > > > > > mode.
> > > > > > 
> > > > > > Douglas, can you test the attached hack instead of this patch?
> > > > > > 
> > > > > > 
> > > > > > thanks,
> > > > > > 
> > > > > > -- 
> > > > > > heikki
> > > > > 
> > > > > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> > > > > > index ec525811a9eb..033dc097ba83 100644
> > > > > > --- a/drivers/usb/typec/tcpm/tcpm.c
> > > > > > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > > > > > @@ -1067,12 +1067,11 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt,
> > > > > >   	modep = &port->mode_data;
> > > > > > -	adev = typec_match_altmode(port->port_altmode, ALTMODE_DISCOVERY_MAX,
> > > > > > -				   PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0]));
> > > > > > -
> > > > > >   	pdev = typec_match_altmode(port->partner_altmode, ALTMODE_DISCOVERY_MAX,
> > > > > >   				   PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0]));
> > > > > > +	adev = (void *)typec_altmode_get_partner(pdev);
> > > > > > +
> > > > > 
> > > > > I understand that typec_altmode_get_partner() returns a const *;
> > > > > maybe adev should be declared as const struct typec_altmode *
> > > > > instead of using a typecast.
> > > > 
> > > > Yes...
> > > > 
> > > > > Also, typec_altmode_get_partner() can return NULL as well if pdev is NULL.
> > > > > Is it guaranteed that typec_match_altmode() never returns NULL for pdev ?
> > > > 
> > > > ...and probable no. But I don't think we can receive Attention to a
> > > > mode that hasn't been entered.
> > > > 
> > > 
> > > If I understand correctly, the Attention was generated by a test system.
> > > What prevents badly implemented code in the connected system from sending
> > > such an Attention message ?
> > 
> > Oh, if that is the case, then I don't think my change has any effect.
> > I misunderstood the scenario. Sorry for that.
> > 
> > I think we should use your patch to fix this issue.
> 
> So is this an "ack"?  I'm confused as to if this patch should be applied
> or not...

I meant to ack it, sorry. Here:

Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

-- 
heikki

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC PATCH] usb: typec: tcpm: Ignore unsupported/unknown alternate mode requests
  2019-07-25  4:30 [RFC PATCH] usb: typec: tcpm: Ignore unsupported/unknown alternate mode requests Guenter Roeck
  2019-07-26 15:21 ` Douglas Gilbert
  2019-07-29 14:04 ` Heikki Krogerus
@ 2019-08-02 11:42 ` Greg Kroah-Hartman
  2019-08-02 15:25 ` Douglas Gilbert
  3 siblings, 0 replies; 12+ messages in thread
From: Greg Kroah-Hartman @ 2019-08-02 11:42 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: Heikki Krogerus, linux-usb, linux-kernel, Douglas Gilbert

On Wed, Jul 24, 2019 at 09:30:37PM -0700, Guenter Roeck wrote:
> TCPM may receive PD messages associated with unknown or unsupported
> alternate modes. If that happens, calls to typec_match_altmode()
> will return NULL. The tcpm code does not currently take this into
> account. This results in crashes.
> 
> Unable to handle kernel NULL pointer dereference at virtual address 000001f0
> pgd = 41dad9a1
> [000001f0] *pgd=00000000
> Internal error: Oops: 5 [#1] THUMB2
> Modules linked in: tcpci tcpm
> CPU: 0 PID: 2338 Comm: kworker/u2:0 Not tainted 5.1.18-sama5-armv7-r2 #6
> Hardware name: Atmel SAMA5
> Workqueue: 2-0050 tcpm_pd_rx_handler [tcpm]
> PC is at typec_altmode_attention+0x0/0x14
> LR is at tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm]
> ...
> [<c03fbee8>] (typec_altmode_attention) from [<bf8030fb>]
> 				(tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm])
> [<bf8030fb>] (tcpm_pd_rx_handler [tcpm]) from [<c012082b>]
> 				(process_one_work+0x123/0x2a8)
> [<c012082b>] (process_one_work) from [<c0120a6d>]
> 				(worker_thread+0xbd/0x3b0)
> [<c0120a6d>] (worker_thread) from [<c012431f>] (kthread+0xcf/0xf4)
> [<c012431f>] (kthread) from [<c01010f9>] (ret_from_fork+0x11/0x38)
> 
> Ignore PD messages if the asociated alternate mode is not supported.
> 
> Reported-by: Douglas Gilbert <dgilbert@interlog.com>
> Cc: Douglas Gilbert <dgilbert@interlog.com>
> Fixes: e9576fe8e605c ("usb: typec: tcpm: Support for Alternate Modes")
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> Taking a stab at the problem. I don't really know if this is the correct
> fix, or even if my understanding of the problem is correct, thus marking
> the patch as RFC.

Now that I think everyone agrees this is a "real" fix, can you resend it
in a format that I can apply it in, with the various acks added?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC PATCH] usb: typec: tcpm: Ignore unsupported/unknown alternate mode requests
  2019-07-25  4:30 [RFC PATCH] usb: typec: tcpm: Ignore unsupported/unknown alternate mode requests Guenter Roeck
                   ` (2 preceding siblings ...)
  2019-08-02 11:42 ` Greg Kroah-Hartman
@ 2019-08-02 15:25 ` Douglas Gilbert
  3 siblings, 0 replies; 12+ messages in thread
From: Douglas Gilbert @ 2019-08-02 15:25 UTC (permalink / raw)
  To: Guenter Roeck, Heikki Krogerus, Greg Kroah-Hartman
  Cc: linux-usb, linux-kernel

On 2019-07-25 6:30 a.m., Guenter Roeck wrote:
> TCPM may receive PD messages associated with unknown or unsupported
> alternate modes. If that happens, calls to typec_match_altmode()
> will return NULL. The tcpm code does not currently take this into
> account. This results in crashes.
> 
> Unable to handle kernel NULL pointer dereference at virtual address 000001f0
> pgd = 41dad9a1
> [000001f0] *pgd=00000000
> Internal error: Oops: 5 [#1] THUMB2
> Modules linked in: tcpci tcpm
> CPU: 0 PID: 2338 Comm: kworker/u2:0 Not tainted 5.1.18-sama5-armv7-r2 #6
> Hardware name: Atmel SAMA5
> Workqueue: 2-0050 tcpm_pd_rx_handler [tcpm]
> PC is at typec_altmode_attention+0x0/0x14
> LR is at tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm]
> ...
> [<c03fbee8>] (typec_altmode_attention) from [<bf8030fb>]
> 				(tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm])
> [<bf8030fb>] (tcpm_pd_rx_handler [tcpm]) from [<c012082b>]
> 				(process_one_work+0x123/0x2a8)
> [<c012082b>] (process_one_work) from [<c0120a6d>]
> 				(worker_thread+0xbd/0x3b0)
> [<c0120a6d>] (worker_thread) from [<c012431f>] (kthread+0xcf/0xf4)
> [<c012431f>] (kthread) from [<c01010f9>] (ret_from_fork+0x11/0x38)
> 
> Ignore PD messages if the asociated alternate mode is not supported.
> 
> Reported-by: Douglas Gilbert <dgilbert@interlog.com>
> Cc: Douglas Gilbert <dgilbert@interlog.com>
> Fixes: e9576fe8e605c ("usb: typec: tcpm: Support for Alternate Modes")
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> Taking a stab at the problem. I don't really know if this is the correct
> fix, or even if my understanding of the problem is correct, thus marking
> the patch as RFC.

Tested-by: Douglas Gilbert <dgilbert@interlog.com>

Tested on different hardware (Acme Arietta based on at91sam9g25) and
lk 5.2.5 . Without this patch the oops was at
    PC is at typec_altmode_attention+0x0/0x24 [typec]

The DUT was a Arietta with a OM13588 attached acting as a PD sink.
At the other end of the type C cable was a KL27Z+OM13588 running
NXP's usb_pd_bm app and it was a PD source. The other end did a
"send attention" to cause the oops.

With this patch nothing happened. Notable there was no oops.
The other end (KL27Z+OM13588 reported "send attention result:
success" but it also did that in the previous "oops" case.

Is there something to replace "tail -f" that would make it
easier to monitor what is being sent to
     /sys/kernel/debug/tcpm/1-0050
?

Doug Gilbert

>   drivers/usb/typec/tcpm/tcpm.c | 38 ++++++++++++++++++++++++--------------
>   1 file changed, 24 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 77f71f602f73..1df2844469aa 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -1096,7 +1096,8 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt,
>   			break;
>   		case CMD_ATTENTION:
>   			/* Attention command does not have response */
> -			typec_altmode_attention(adev, p[1]);
> +			if (adev)
> +				typec_altmode_attention(adev, p[1]);
>   			return 0;
>   		default:
>   			break;
> @@ -1148,20 +1149,26 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt,
>   			}
>   			break;
>   		case CMD_ENTER_MODE:
> -			typec_altmode_update_active(pdev, true);
> -
> -			if (typec_altmode_vdm(adev, p[0], &p[1], cnt)) {
> -				response[0] = VDO(adev->svid, 1, CMD_EXIT_MODE);
> -				response[0] |= VDO_OPOS(adev->mode);
> -				return 1;
> +			if (adev && pdev) {
> +				typec_altmode_update_active(pdev, true);
> +
> +				if (typec_altmode_vdm(adev, p[0], &p[1], cnt)) {
> +					response[0] = VDO(adev->svid, 1,
> +							  CMD_EXIT_MODE);
> +					response[0] |= VDO_OPOS(adev->mode);
> +					return 1;
> +				}
>   			}
>   			return 0;
>   		case CMD_EXIT_MODE:
> -			typec_altmode_update_active(pdev, false);
> +			if (adev && pdev) {
> +				typec_altmode_update_active(pdev, false);
>   
> -			/* Back to USB Operation */
> -			WARN_ON(typec_altmode_notify(adev, TYPEC_STATE_USB,
> -						     NULL));
> +				/* Back to USB Operation */
> +				WARN_ON(typec_altmode_notify(adev,
> +							     TYPEC_STATE_USB,
> +							     NULL));
> +			}
>   			break;
>   		default:
>   			break;
> @@ -1171,8 +1178,10 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt,
>   		switch (cmd) {
>   		case CMD_ENTER_MODE:
>   			/* Back to USB Operation */
> -			WARN_ON(typec_altmode_notify(adev, TYPEC_STATE_USB,
> -						     NULL));
> +			if (adev)
> +				WARN_ON(typec_altmode_notify(adev,
> +							     TYPEC_STATE_USB,
> +							     NULL));
>   			break;
>   		default:
>   			break;
> @@ -1183,7 +1192,8 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt,
>   	}
>   
>   	/* Informing the alternate mode drivers about everything */
> -	typec_altmode_vdm(adev, p[0], &p[1], cnt);
> +	if (adev)
> +		typec_altmode_vdm(adev, p[0], &p[1], cnt);
>   
>   	return rlen;
>   }
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2019-08-02 15:25 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-25  4:30 [RFC PATCH] usb: typec: tcpm: Ignore unsupported/unknown alternate mode requests Guenter Roeck
2019-07-26 15:21 ` Douglas Gilbert
2019-07-29 14:04 ` Heikki Krogerus
2019-07-29 17:31   ` Guenter Roeck
2019-07-30 12:07     ` Heikki Krogerus
2019-07-30 12:17       ` Douglas Gilbert
2019-07-30 13:28       ` Guenter Roeck
2019-07-31  9:55         ` Heikki Krogerus
2019-08-01 16:07           ` Greg Kroah-Hartman
2019-08-02  8:01             ` Heikki Krogerus
2019-08-02 11:42 ` Greg Kroah-Hartman
2019-08-02 15:25 ` Douglas Gilbert

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).