diff mbox series

[v3] usb: typec: bus: verify partner exists in typec_altmode_attention

Message ID 20230811213732.3325896-1-rdbabiera@google.com (mailing list archive)
State Superseded
Headers show
Series [v3] usb: typec: bus: verify partner exists in typec_altmode_attention | expand

Commit Message

RD Babiera Aug. 11, 2023, 9:37 p.m. UTC 
Some usb hubs will negotiate DisplayPort Alt mode with the device
but will then negotiate a data role swap after entering the alt
mode. The data role swap causes the device to unregister all alt
modes, however the usb hub will still send Attention messages
even after failing to reregister the Alt Mode. type_altmode_attention
currently does not verify whether or not a device's altmode partner
exists, which results in a NULL pointer error when dereferencing
the typec_altmode and typec_altmode_ops belonging to the altmode
partner.

Verify the presence of a device's altmode partner before sending
the Attention message to the Alt Mode driver.

Fixes: 8a37d87d72f0 ("usb: typec: Bus type for alternate modes")
Cc: stable@vger.kernel.org
Signed-off-by: RD Babiera <rdbabiera@google.com>
---
Changes since v1:
* Only assigns pdev if altmode partner exists in typec_altmode_attention
* Removed error return in typec_altmode_attention if Alt Mode does
  not implement Attention messages.
* Changed tcpm_log message to indicate that altmode partner does not exist,
  as it only logs in that case.
---
Changes since v2:
* Changed tcpm_log message to accurately reflect error
* Revised commit message
---
 drivers/usb/typec/bus.c           | 12 ++++++++++--
 drivers/usb/typec/tcpm/tcpm.c     |  5 ++++-
 include/linux/usb/typec_altmode.h |  2 +-
 3 files changed, 15 insertions(+), 4 deletions(-)


base-commit: f176638af476c6d46257cc3303f5c7cf47d5967d

Comments

Guenter Roeck Aug. 11, 2023, 10:39 p.m. UTC | #1 
On 8/11/23 14:37, RD Babiera wrote:
> Some usb hubs will negotiate DisplayPort Alt mode with the device
> but will then negotiate a data role swap after entering the alt
> mode. The data role swap causes the device to unregister all alt
> modes, however the usb hub will still send Attention messages
> even after failing to reregister the Alt Mode. type_altmode_attention
> currently does not verify whether or not a device's altmode partner
> exists, which results in a NULL pointer error when dereferencing
> the typec_altmode and typec_altmode_ops belonging to the altmode
> partner.
> 
> Verify the presence of a device's altmode partner before sending
> the Attention message to the Alt Mode driver.
> 
> Fixes: 8a37d87d72f0 ("usb: typec: Bus type for alternate modes")
> Cc: stable@vger.kernel.org
> Signed-off-by: RD Babiera <rdbabiera@google.com>
> ---
> Changes since v1:
> * Only assigns pdev if altmode partner exists in typec_altmode_attention
> * Removed error return in typec_altmode_attention if Alt Mode does
>    not implement Attention messages.
> * Changed tcpm_log message to indicate that altmode partner does not exist,
>    as it only logs in that case.
> ---
> Changes since v2:
> * Changed tcpm_log message to accurately reflect error
> * Revised commit message

IMO the log message should be "no port partner", but I don't want this
to go on forever.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/usb/typec/bus.c           | 12 ++++++++++--
>   drivers/usb/typec/tcpm/tcpm.c     |  5 ++++-
>   include/linux/usb/typec_altmode.h |  2 +-
>   3 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/typec/bus.c b/drivers/usb/typec/bus.c
> index fe5b9a2e61f5..e95ec7e382bb 100644
> --- a/drivers/usb/typec/bus.c
> +++ b/drivers/usb/typec/bus.c
> @@ -183,12 +183,20 @@ EXPORT_SYMBOL_GPL(typec_altmode_exit);
>    *
>    * Notifies the partner of @adev about Attention command.
>    */
> -void typec_altmode_attention(struct typec_altmode *adev, u32 vdo)
> +int typec_altmode_attention(struct typec_altmode *adev, u32 vdo)
>   {
> -	struct typec_altmode *pdev = &to_altmode(adev)->partner->adev;
> +	struct altmode *partner = to_altmode(adev)->partner;
> +	struct typec_altmode *pdev;
> +
> +	if (!partner)
> +		return -ENODEV;
> +
> +	pdev = &partner->adev;
>   
>   	if (pdev->ops && pdev->ops->attention)
>   		pdev->ops->attention(pdev, vdo);
> +
> +	return 0;
>   }
>   EXPORT_SYMBOL_GPL(typec_altmode_attention);
>   
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 5a7d8cc04628..97b7b22e9cf1 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -1791,6 +1791,7 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port,
>   	u32 p[PD_MAX_PAYLOAD];
>   	u32 response[8] = { };
>   	int i, rlen = 0;
> +	int ret;
>   
>   	for (i = 0; i < cnt; i++)
>   		p[i] = le32_to_cpu(payload[i]);
> @@ -1877,7 +1878,9 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port,
>   			}
>   			break;
>   		case ADEV_ATTENTION:
> -			typec_altmode_attention(adev, p[1]);
> +			ret = typec_altmode_attention(adev, p[1]);
> +			if (ret)
> +				tcpm_log(port, "typec_altmode_attention NULL port partner altmode");
>   			break;
>   		}
>   	}
> diff --git a/include/linux/usb/typec_altmode.h b/include/linux/usb/typec_altmode.h
> index 350d49012659..28aeef8f9e7b 100644
> --- a/include/linux/usb/typec_altmode.h
> +++ b/include/linux/usb/typec_altmode.h
> @@ -67,7 +67,7 @@ struct typec_altmode_ops {
>   
>   int typec_altmode_enter(struct typec_altmode *altmode, u32 *vdo);
>   int typec_altmode_exit(struct typec_altmode *altmode);
> -void typec_altmode_attention(struct typec_altmode *altmode, u32 vdo);
> +int typec_altmode_attention(struct typec_altmode *altmode, u32 vdo);
>   int typec_altmode_vdm(struct typec_altmode *altmode,
>   		      const u32 header, const u32 *vdo, int count);
>   int typec_altmode_notify(struct typec_altmode *altmode, unsigned long conf,
> 
> base-commit: f176638af476c6d46257cc3303f5c7cf47d5967d
Heikki Krogerus Aug. 14, 2023, 1:36 p.m. UTC | #2 
On Fri, Aug 11, 2023 at 09:37:32PM +0000, RD Babiera wrote:
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 5a7d8cc04628..97b7b22e9cf1 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -1791,6 +1791,7 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port,
>  	u32 p[PD_MAX_PAYLOAD];
>  	u32 response[8] = { };
>  	int i, rlen = 0;
> +	int ret;
>  
>  	for (i = 0; i < cnt; i++)
>  		p[i] = le32_to_cpu(payload[i]);
> @@ -1877,7 +1878,9 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port,
>  			}
>  			break;
>  		case ADEV_ATTENTION:
> -			typec_altmode_attention(adev, p[1]);
> +			ret = typec_altmode_attention(adev, p[1]);
> +			if (ret)
> +				tcpm_log(port, "typec_altmode_attention NULL port partner altmode");

I don't think you need that ret variable:

                        if (typec_altmode_attention(adev, p[1]))
				tcpm_log(port, "typec_altmode_attention NULL port partner altmode");

>  			break;
>  		}
>  	}

thanks,
RD Babiera Aug. 14, 2023, 5:45 p.m. UTC | #3 
On Mon, Aug 14, 2023 at 6:36 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
> I don't think you need that ret variable:

Will fix this and Guenter's tcpm_log comment as well.

---
thanks,
RD
diff mbox series

Patch

diff --git a/drivers/usb/typec/bus.c b/drivers/usb/typec/bus.c
index fe5b9a2e61f5..e95ec7e382bb 100644
--- a/drivers/usb/typec/bus.c
+++ b/drivers/usb/typec/bus.c
@@ -183,12 +183,20 @@  EXPORT_SYMBOL_GPL(typec_altmode_exit);
  *
  * Notifies the partner of @adev about Attention command.
  */
-void typec_altmode_attention(struct typec_altmode *adev, u32 vdo)
+int typec_altmode_attention(struct typec_altmode *adev, u32 vdo)
 {
-	struct typec_altmode *pdev = &to_altmode(adev)->partner->adev;
+	struct altmode *partner = to_altmode(adev)->partner;
+	struct typec_altmode *pdev;
+
+	if (!partner)
+		return -ENODEV;
+
+	pdev = &partner->adev;
 
 	if (pdev->ops && pdev->ops->attention)
 		pdev->ops->attention(pdev, vdo);
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(typec_altmode_attention);
 
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 5a7d8cc04628..97b7b22e9cf1 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -1791,6 +1791,7 @@  static void tcpm_handle_vdm_request(struct tcpm_port *port,
 	u32 p[PD_MAX_PAYLOAD];
 	u32 response[8] = { };
 	int i, rlen = 0;
+	int ret;
 
 	for (i = 0; i < cnt; i++)
 		p[i] = le32_to_cpu(payload[i]);
@@ -1877,7 +1878,9 @@  static void tcpm_handle_vdm_request(struct tcpm_port *port,
 			}
 			break;
 		case ADEV_ATTENTION:
-			typec_altmode_attention(adev, p[1]);
+			ret = typec_altmode_attention(adev, p[1]);
+			if (ret)
+				tcpm_log(port, "typec_altmode_attention NULL port partner altmode");
 			break;
 		}
 	}
diff --git a/include/linux/usb/typec_altmode.h b/include/linux/usb/typec_altmode.h
index 350d49012659..28aeef8f9e7b 100644
--- a/include/linux/usb/typec_altmode.h
+++ b/include/linux/usb/typec_altmode.h
@@ -67,7 +67,7 @@  struct typec_altmode_ops {
 
 int typec_altmode_enter(struct typec_altmode *altmode, u32 *vdo);
 int typec_altmode_exit(struct typec_altmode *altmode);
-void typec_altmode_attention(struct typec_altmode *altmode, u32 vdo);
+int typec_altmode_attention(struct typec_altmode *altmode, u32 vdo);
 int typec_altmode_vdm(struct typec_altmode *altmode,
 		      const u32 header, const u32 *vdo, int count);
 int typec_altmode_notify(struct typec_altmode *altmode, unsigned long conf,