[PATCH 1/3] usb: ucsi: Fix NULL pointer deref in ucsi_connector_change()

[PATCH 1/3] usb: ucsi: Fix NULL pointer deref in ucsi_connector_change()

[Hans de Goede]

When ucsi_init() fails, ucsi->connector is NULL, yet in case of
ucsi_acpi we may still get events which cause the ucs_acpi code to call
ucsi_connector_change(), which then derefs the NULL ucsi->connector
pointer.

Fix this by adding a check for ucsi->connector being NULL, as is
already done in ucsi_resume() for similar reasons.

Fixes: bdc62f2bae8f ("usb: typec: ucsi: Simplified registration and I/O API")
Cc: stable@vger.kernel.org
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/typec/ucsi/ucsi.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 1cf8947c6d66..e762897cb25a 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -842,7 +842,13 @@ static void ucsi_handle_connector_change(struct work_struct *work)
  */
 void ucsi_connector_change(struct ucsi *ucsi, u8 num)
 {
-	struct ucsi_connector *con = &ucsi->connector[num - 1];
+	struct ucsi_connector *con;
+
+	/* Check for ucsi_init() failure */
+	if (!ucsi->connector)
+		return;
+
+	con = &ucsi->connector[num - 1];
 
 	if (!(ucsi->ntfy & UCSI_ENABLE_NTFY_CONNECTOR_CHANGE)) {
 		dev_dbg(ucsi->dev, "Bogus connector change event\n");
-- 
2.39.1

[PATCH 2/3] usb: ucsi: Fix ucsi->connector race

[Hans de Goede]

ucsi_init() which runs from a workqueue sets ucsi->connector and
on an error will clear it again.

ucsi->connector gets dereferenced by both ucsi_connector_change() and
ucsi_resume(), both check for ucsi->connector being NULL in case
ucsi_init() has not finished yet; or in case ucsi_init() has failed.

ucsi_init() setting ucsi->connector and then clearing it again on
an error creates a race where the check in the consumers may pass,
only to have ucsi->connector free-ed underneath them when ucsi_init()
hits an error.

Fix this race by making ucsi_init() store the connector array in
a local variable and only assign it to ucsi->connector on success.

Fixes: bdc62f2bae8f ("usb: typec: ucsi: Simplified registration and I/O API")
Cc: stable@vger.kernel.org
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/typec/ucsi/ucsi.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index e762897cb25a..796ae230c60b 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -1045,9 +1045,8 @@ static struct fwnode_handle *ucsi_find_fwnode(struct ucsi_connector *con)
 	return NULL;
 }
 
-static int ucsi_register_port(struct ucsi *ucsi, int index)
+static int ucsi_register_port(struct ucsi *ucsi, int index, struct ucsi_connector *con)
 {
-	struct ucsi_connector *con = &ucsi->connector[index];
 	struct typec_capability *cap = &con->typec_cap;
 	enum typec_accessory *accessory = cap->accessory;
 	enum usb_role u_role = USB_ROLE_NONE;
@@ -1210,7 +1209,7 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
  */
 static int ucsi_init(struct ucsi *ucsi)
 {
-	struct ucsi_connector *con;
+	struct ucsi_connector *con, *connector;
 	u64 command;
 	int ret;
 	int i;
@@ -1241,16 +1240,15 @@ static int ucsi_init(struct ucsi *ucsi)
 	}
 
 	/* Allocate the connectors. Released in ucsi_unregister() */
-	ucsi->connector = kcalloc(ucsi->cap.num_connectors + 1,
-				  sizeof(*ucsi->connector), GFP_KERNEL);
-	if (!ucsi->connector) {
+	connector = kcalloc(ucsi->cap.num_connectors + 1, sizeof(*connector), GFP_KERNEL);
+	if (!connector) {
 		ret = -ENOMEM;
 		goto err_reset;
 	}
 
 	/* Register all connectors */
 	for (i = 0; i < ucsi->cap.num_connectors; i++) {
-		ret = ucsi_register_port(ucsi, i);
+		ret = ucsi_register_port(ucsi, i, &connector[i]);
 		if (ret)
 			goto err_unregister;
 	}
@@ -1262,10 +1260,11 @@ static int ucsi_init(struct ucsi *ucsi)
 	if (ret < 0)
 		goto err_unregister;
 
+	ucsi->connector = connector;
 	return 0;
 
 err_unregister:
-	for (con = ucsi->connector; con->port; con++) {
+	for (con = connector; con->port; con++) {
 		ucsi_unregister_partner(con);
 		ucsi_unregister_altmodes(con, UCSI_RECIPIENT_CON);
 		ucsi_unregister_port_psy(con);
@@ -1274,10 +1273,7 @@ static int ucsi_init(struct ucsi *ucsi)
 		typec_unregister_port(con->port);
 		con->port = NULL;
 	}
-
-	kfree(ucsi->connector);
-	ucsi->connector = NULL;
-
+	kfree(connector);
 err_reset:
 	memset(&ucsi->cap, 0, sizeof(ucsi->cap));
 	ucsi_reset_ppm(ucsi);
-- 
2.39.1

[PATCH 3/3] usb: ucsi_acpi: Increase the command completion timeout

[Hans de Goede]

Commit 130a96d698d7 ("usb: typec: ucsi: acpi: Increase command
completion timeout value") increased the timeout from 5 seconds
to 60 seconds due to issues related to alternate mode discovery.

After the alternate mode discovery switch to polled mode
the timeout was reduced, but instead of being set back to
5 seconds it was reduced to 1 second.

This is causing problems when using a Lenovo ThinkPad X1 yoga gen7
connected over Type-C to a LG 27UL850-W (charging DP over Type-C).

When the monitor is already connected at boot the following error
is logged: "PPM init failed (-110)", /sys/class/typec is empty and
on unplugging the NULL pointer deref fixed earlier in this series
happens.

When the monitor is connected after boot the following error
is logged instead: "GET_CONNECTOR_STATUS failed (-110)".

Setting the timeout back to 5 seconds fixes both cases.

Fixes: e08065069fc7 ("usb: typec: ucsi: acpi: Reduce the command completion timeout")
Cc: stable@vger.kernel.org
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/typec/ucsi/ucsi_acpi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
index ce0c8ef80c04..62206a6b8ea7 100644
--- a/drivers/usb/typec/ucsi/ucsi_acpi.c
+++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
@@ -78,7 +78,7 @@ static int ucsi_acpi_sync_write(struct ucsi *ucsi, unsigned int offset,
 	if (ret)
 		goto out_clear_bit;
 
-	if (!wait_for_completion_timeout(&ua->complete, HZ))
+	if (!wait_for_completion_timeout(&ua->complete, 5 * HZ))
 		ret = -ETIMEDOUT;
 
 out_clear_bit:
-- 
2.39.1

Re: [PATCH 1/3] usb: ucsi: Fix NULL pointer deref in ucsi_connector_change()

[Greg Kroah-Hartman]

On Tue, Feb 28, 2023 at 10:03:03AM +0100, Hans de Goede wrote:
> When ucsi_init() fails, ucsi->connector is NULL, yet in case of
> ucsi_acpi we may still get events which cause the ucs_acpi code to call
> ucsi_connector_change(), which then derefs the NULL ucsi->connector
> pointer.
> 
> Fix this by adding a check for ucsi->connector being NULL, as is
> already done in ucsi_resume() for similar reasons.
> 
> Fixes: bdc62f2bae8f ("usb: typec: ucsi: Simplified registration and I/O API")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/usb/typec/ucsi/ucsi.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 1cf8947c6d66..e762897cb25a 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -842,7 +842,13 @@ static void ucsi_handle_connector_change(struct work_struct *work)
>   */
>  void ucsi_connector_change(struct ucsi *ucsi, u8 num)
>  {
> -	struct ucsi_connector *con = &ucsi->connector[num - 1];
> +	struct ucsi_connector *con;
> +
> +	/* Check for ucsi_init() failure */
> +	if (!ucsi->connector)
> +		return;
> +
> +	con = &ucsi->connector[num - 1];

What prevents ->connector from changing to NULL right after you check
this and before you dereference it again?

thanks,

greg k-h

Re: [PATCH 1/3] usb: ucsi: Fix NULL pointer deref in ucsi_connector_change()

[Heikki Krogerus]

On Tue, Feb 28, 2023 at 10:03:03AM +0100, Hans de Goede wrote:
> When ucsi_init() fails, ucsi->connector is NULL, yet in case of
> ucsi_acpi we may still get events which cause the ucs_acpi code to call
> ucsi_connector_change(), which then derefs the NULL ucsi->connector
> pointer.
> 
> Fix this by adding a check for ucsi->connector being NULL, as is
> already done in ucsi_resume() for similar reasons.
> 
> Fixes: bdc62f2bae8f ("usb: typec: ucsi: Simplified registration and I/O API")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/usb/typec/ucsi/ucsi.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 1cf8947c6d66..e762897cb25a 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -842,7 +842,13 @@ static void ucsi_handle_connector_change(struct work_struct *work)
>   */
>  void ucsi_connector_change(struct ucsi *ucsi, u8 num)
>  {
> -	struct ucsi_connector *con = &ucsi->connector[num - 1];
> +	struct ucsi_connector *con;
> +
> +	/* Check for ucsi_init() failure */
> +	if (!ucsi->connector)
> +		return;
> +
> +	con = &ucsi->connector[num - 1];
>  
>  	if (!(ucsi->ntfy & UCSI_ENABLE_NTFY_CONNECTOR_CHANGE)) {
>  		dev_dbg(ucsi->dev, "Bogus connector change event\n");

I think we should try to rely on that ucsi->ntfy. Would this work:

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index fe1963e328378..0da1e9c66971a 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -928,15 +928,13 @@ static void ucsi_handle_connector_change(struct work_struct *work)
  */
 void ucsi_connector_change(struct ucsi *ucsi, u8 num)
 {
-       struct ucsi_connector *con = &ucsi->connector[num - 1];
-
        if (!(ucsi->ntfy & UCSI_ENABLE_NTFY_CONNECTOR_CHANGE)) {
                dev_dbg(ucsi->dev, "Bogus connector change event\n");
                return;
        }
 
        if (!test_and_set_bit(EVENT_PENDING, &ucsi->flags))
-               schedule_work(&con->work);
+               schedule_work(&ucsi->connector[num - 1].work);
 }
 EXPORT_SYMBOL_GPL(ucsi_connector_change);
 
@@ -1404,6 +1402,7 @@ static int ucsi_init(struct ucsi *ucsi)
        ucsi->connector = NULL;
 
 err_reset:
+       ucsi->ntfy = 0;
        memset(&ucsi->cap, 0, sizeof(ucsi->cap));
        ucsi_reset_ppm(ucsi);
 err:


-- 
heikki

Re: [PATCH 1/3] usb: ucsi: Fix NULL pointer deref in ucsi_connector_change()

[Hans de Goede]

Hi,

On 2/28/23 11:05, Heikki Krogerus wrote:
> On Tue, Feb 28, 2023 at 10:03:03AM +0100, Hans de Goede wrote:
>> When ucsi_init() fails, ucsi->connector is NULL, yet in case of
>> ucsi_acpi we may still get events which cause the ucs_acpi code to call
>> ucsi_connector_change(), which then derefs the NULL ucsi->connector
>> pointer.
>>
>> Fix this by adding a check for ucsi->connector being NULL, as is
>> already done in ucsi_resume() for similar reasons.
>>
>> Fixes: bdc62f2bae8f ("usb: typec: ucsi: Simplified registration and I/O API")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/usb/typec/ucsi/ucsi.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
>> index 1cf8947c6d66..e762897cb25a 100644
>> --- a/drivers/usb/typec/ucsi/ucsi.c
>> +++ b/drivers/usb/typec/ucsi/ucsi.c
>> @@ -842,7 +842,13 @@ static void ucsi_handle_connector_change(struct work_struct *work)
>>   */
>>  void ucsi_connector_change(struct ucsi *ucsi, u8 num)
>>  {
>> -	struct ucsi_connector *con = &ucsi->connector[num - 1];
>> +	struct ucsi_connector *con;
>> +
>> +	/* Check for ucsi_init() failure */
>> +	if (!ucsi->connector)
>> +		return;
>> +
>> +	con = &ucsi->connector[num - 1];
>>  
>>  	if (!(ucsi->ntfy & UCSI_ENABLE_NTFY_CONNECTOR_CHANGE)) {
>>  		dev_dbg(ucsi->dev, "Bogus connector change event\n");
> 
> I think we should try to rely on that ucsi->ntfy. Would this work:
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index fe1963e328378..0da1e9c66971a 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -928,15 +928,13 @@ static void ucsi_handle_connector_change(struct work_struct *work)
>   */
>  void ucsi_connector_change(struct ucsi *ucsi, u8 num)
>  {
> -       struct ucsi_connector *con = &ucsi->connector[num - 1];
> -
>         if (!(ucsi->ntfy & UCSI_ENABLE_NTFY_CONNECTOR_CHANGE)) {
>                 dev_dbg(ucsi->dev, "Bogus connector change event\n");
>                 return;
>         }
>  
>         if (!test_and_set_bit(EVENT_PENDING, &ucsi->flags))
> -               schedule_work(&con->work);
> +               schedule_work(&ucsi->connector[num - 1].work);
>  }
>  EXPORT_SYMBOL_GPL(ucsi_connector_change);
>  

This hunk is not necessary, the con pointer pointing to lala land is
not an issue as long as we don't deref it. The &ucsi->connector[num - 1];
does not deref ucsi->connector it it simply adds an offset to it and
stores that in con (the backtrace I got pointed to the schedule_work call).

But I guess your way does make it more obvious that we don't
deref ucsi->connector.

> @@ -1404,6 +1402,7 @@ static int ucsi_init(struct ucsi *ucsi)
>         ucsi->connector = NULL;
>  
>  err_reset:
> +       ucsi->ntfy = 0;
>         memset(&ucsi->cap, 0, sizeof(ucsi->cap));
>         ucsi_reset_ppm(ucsi);
>  err:

In would expect this to fix things, but I only have access to the monitor
triggering this on Mondays, so I can only 100% confirm next Monday.

Note this does open the race I try to fix in patch 2/3 again.

So what should be done here is to make ntfy a local variable and only
store it in ucsi->ntfy on success.

Regards,

Hans

Re: [PATCH 1/3] usb: ucsi: Fix NULL pointer deref in ucsi_connector_change()

[Heikki Krogerus]

On Tue, Feb 28, 2023 at 11:15:46AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 2/28/23 11:05, Heikki Krogerus wrote:
> > On Tue, Feb 28, 2023 at 10:03:03AM +0100, Hans de Goede wrote:
> >> When ucsi_init() fails, ucsi->connector is NULL, yet in case of
> >> ucsi_acpi we may still get events which cause the ucs_acpi code to call
> >> ucsi_connector_change(), which then derefs the NULL ucsi->connector
> >> pointer.
> >>
> >> Fix this by adding a check for ucsi->connector being NULL, as is
> >> already done in ucsi_resume() for similar reasons.
> >>
> >> Fixes: bdc62f2bae8f ("usb: typec: ucsi: Simplified registration and I/O API")
> >> Cc: stable@vger.kernel.org
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >>  drivers/usb/typec/ucsi/ucsi.c | 8 +++++++-
> >>  1 file changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> >> index 1cf8947c6d66..e762897cb25a 100644
> >> --- a/drivers/usb/typec/ucsi/ucsi.c
> >> +++ b/drivers/usb/typec/ucsi/ucsi.c
> >> @@ -842,7 +842,13 @@ static void ucsi_handle_connector_change(struct work_struct *work)
> >>   */
> >>  void ucsi_connector_change(struct ucsi *ucsi, u8 num)
> >>  {
> >> -	struct ucsi_connector *con = &ucsi->connector[num - 1];
> >> +	struct ucsi_connector *con;
> >> +
> >> +	/* Check for ucsi_init() failure */
> >> +	if (!ucsi->connector)
> >> +		return;
> >> +
> >> +	con = &ucsi->connector[num - 1];
> >>  
> >>  	if (!(ucsi->ntfy & UCSI_ENABLE_NTFY_CONNECTOR_CHANGE)) {
> >>  		dev_dbg(ucsi->dev, "Bogus connector change event\n");
> > 
> > I think we should try to rely on that ucsi->ntfy. Would this work:
> > 
> > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> > index fe1963e328378..0da1e9c66971a 100644
> > --- a/drivers/usb/typec/ucsi/ucsi.c
> > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > @@ -928,15 +928,13 @@ static void ucsi_handle_connector_change(struct work_struct *work)
> >   */
> >  void ucsi_connector_change(struct ucsi *ucsi, u8 num)
> >  {
> > -       struct ucsi_connector *con = &ucsi->connector[num - 1];
> > -
> >         if (!(ucsi->ntfy & UCSI_ENABLE_NTFY_CONNECTOR_CHANGE)) {
> >                 dev_dbg(ucsi->dev, "Bogus connector change event\n");
> >                 return;
> >         }
> >  
> >         if (!test_and_set_bit(EVENT_PENDING, &ucsi->flags))
> > -               schedule_work(&con->work);
> > +               schedule_work(&ucsi->connector[num - 1].work);
> >  }
> >  EXPORT_SYMBOL_GPL(ucsi_connector_change);
> >  
> 
> This hunk is not necessary, the con pointer pointing to lala land is
> not an issue as long as we don't deref it. The &ucsi->connector[num - 1];
> does not deref ucsi->connector it it simply adds an offset to it and
> stores that in con (the backtrace I got pointed to the schedule_work call).
> 
> But I guess your way does make it more obvious that we don't
> deref ucsi->connector.
> 
> > @@ -1404,6 +1402,7 @@ static int ucsi_init(struct ucsi *ucsi)
> >         ucsi->connector = NULL;
> >  
> >  err_reset:
> > +       ucsi->ntfy = 0;
> >         memset(&ucsi->cap, 0, sizeof(ucsi->cap));
> >         ucsi_reset_ppm(ucsi);
> >  err:
> 
> In would expect this to fix things, but I only have access to the monitor
> triggering this on Mondays, so I can only 100% confirm next Monday.
> 
> Note this does open the race I try to fix in patch 2/3 again.
> 
> So what should be done here is to make ntfy a local variable and only
> store it in ucsi->ntfy on success.

OK.

thanks,

-- 
heikki