[RFC PATCH] usb: typec: tcpm: Ignore unsupported/unknown alternate mode requests

[RFC PATCH] usb: typec: tcpm: Ignore unsupported/unknown alternate mode requests

[Guenter Roeck]

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

Re: [RFC PATCH] usb: typec: tcpm: Ignore unsupported/unknown alternate mode requests

[Douglas Gilbert]

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

Re: [RFC PATCH] usb: typec: tcpm: Ignore unsupported/unknown alternate mode requests

[Heikki Krogerus]

[-- 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) {

Re: [RFC PATCH] usb: typec: tcpm: Ignore unsupported/unknown alternate mode requests

[Guenter Roeck]

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) {

Re: [RFC PATCH] usb: typec: tcpm: Ignore unsupported/unknown alternate mode requests

[Heikki Krogerus]

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

Re: [RFC PATCH] usb: typec: tcpm: Ignore unsupported/unknown alternate mode requests

[Douglas Gilbert]

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

Re: [RFC PATCH] usb: typec: tcpm: Ignore unsupported/unknown alternate mode requests

[Guenter Roeck]

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

Re: [RFC PATCH] usb: typec: tcpm: Ignore unsupported/unknown alternate mode requests

[Heikki Krogerus]

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

Re: [RFC PATCH] usb: typec: tcpm: Ignore unsupported/unknown alternate mode requests

[Greg Kroah-Hartman]

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

Re: [RFC PATCH] usb: typec: tcpm: Ignore unsupported/unknown alternate mode requests

[Heikki Krogerus]

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

Re: [RFC PATCH] usb: typec: tcpm: Ignore unsupported/unknown alternate mode requests

[Greg Kroah-Hartman]

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

Re: [RFC PATCH] usb: typec: tcpm: Ignore unsupported/unknown alternate mode requests

[Douglas Gilbert]

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