* [PATCH v2 0/3] usb: ucsi: 3 bug fixes
@ 2023-03-06 10:33 Hans de Goede
2023-03-06 10:33 ` [PATCH v2 1/3] usb: ucsi: Fix NULL pointer deref in ucsi_connector_change() Hans de Goede
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Hans de Goede @ 2023-03-06 10:33 UTC (permalink / raw
To: Greg Kroah-Hartman, Heikki Krogerus; +Cc: Hans de Goede, linux-usb
Hi Heikki,
Here is v2 of my ucsi bugfix series.
Changes in v2:
-Delay setting ucsi->ntfy in ucsi_init() instead of adding a NULL pointer
check to ucsi_connector_change()
Regards,
Hans
Hans de Goede (3):
usb: ucsi: Fix NULL pointer deref in ucsi_connector_change()
usb: ucsi: Fix ucsi->connector race
usb: ucsi_acpi: Increase the command completion timeout
drivers/usb/typec/ucsi/ucsi.c | 31 ++++++++++++++----------------
drivers/usb/typec/ucsi/ucsi_acpi.c | 2 +-
2 files changed, 15 insertions(+), 18 deletions(-)
--
2.39.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/3] usb: ucsi: Fix NULL pointer deref in ucsi_connector_change()
2023-03-06 10:33 [PATCH v2 0/3] usb: ucsi: 3 bug fixes Hans de Goede
@ 2023-03-06 10:33 ` Hans de Goede
2023-03-07 7:32 ` Heikki Krogerus
2023-03-07 9:17 ` Heikki Krogerus
2023-03-06 10:33 ` [PATCH v2 2/3] usb: ucsi: Fix ucsi->connector race Hans de Goede
2023-03-06 10:33 ` [PATCH v2 3/3] usb: ucsi_acpi: Increase the command completion timeout Hans de Goede
2 siblings, 2 replies; 9+ messages in thread
From: Hans de Goede @ 2023-03-06 10:33 UTC (permalink / raw
To: Greg Kroah-Hartman, Heikki Krogerus; +Cc: Hans de Goede, linux-usb, stable
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 not setting ucsi->ntfy inside ucsi_init() until ucsi_init()
has succeeded, so that ucsi_connector_change() ignores the events
because UCSI_ENABLE_NTFY_CONNECTOR_CHANGE is not set in the ntfy mask.
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>
---
Changes in v2:
-Delay setting ucsi->ntfy in ucsi_init() instead of adding a NULL pointer
check to ucsi_connector_change()
---
drivers/usb/typec/ucsi/ucsi.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 1cf8947c6d66..8cbbb002fefe 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -1205,7 +1205,7 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
static int ucsi_init(struct ucsi *ucsi)
{
struct ucsi_connector *con;
- u64 command;
+ u64 command, ntfy;
int ret;
int i;
@@ -1217,8 +1217,8 @@ static int ucsi_init(struct ucsi *ucsi)
}
/* Enable basic notifications */
- ucsi->ntfy = UCSI_ENABLE_NTFY_CMD_COMPLETE | UCSI_ENABLE_NTFY_ERROR;
- command = UCSI_SET_NOTIFICATION_ENABLE | ucsi->ntfy;
+ ntfy = UCSI_ENABLE_NTFY_CMD_COMPLETE | UCSI_ENABLE_NTFY_ERROR;
+ command = UCSI_SET_NOTIFICATION_ENABLE | ntfy;
ret = ucsi_send_command(ucsi, command, NULL, 0);
if (ret < 0)
goto err_reset;
@@ -1250,12 +1250,13 @@ static int ucsi_init(struct ucsi *ucsi)
}
/* Enable all notifications */
- ucsi->ntfy = UCSI_ENABLE_NTFY_ALL;
- command = UCSI_SET_NOTIFICATION_ENABLE | ucsi->ntfy;
+ ntfy = UCSI_ENABLE_NTFY_ALL;
+ command = UCSI_SET_NOTIFICATION_ENABLE | ntfy;
ret = ucsi_send_command(ucsi, command, NULL, 0);
if (ret < 0)
goto err_unregister;
+ ucsi->ntfy = ntfy;
return 0;
err_unregister:
--
2.39.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 2/3] usb: ucsi: Fix ucsi->connector race
2023-03-06 10:33 [PATCH v2 0/3] usb: ucsi: 3 bug fixes Hans de Goede
2023-03-06 10:33 ` [PATCH v2 1/3] usb: ucsi: Fix NULL pointer deref in ucsi_connector_change() Hans de Goede
@ 2023-03-06 10:33 ` Hans de Goede
2023-03-07 7:46 ` Heikki Krogerus
2023-03-06 10:33 ` [PATCH v2 3/3] usb: ucsi_acpi: Increase the command completion timeout Hans de Goede
2 siblings, 1 reply; 9+ messages in thread
From: Hans de Goede @ 2023-03-06 10:33 UTC (permalink / raw
To: Greg Kroah-Hartman, Heikki Krogerus; +Cc: Hans de Goede, linux-usb, stable
ucsi_init() which runs from a workqueue sets ucsi->connector and
on an error will clear it again.
ucsi->connector gets dereferenced by ucsi_resume(), this checks 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 ucsi_resume() may pass,
only to have ucsi->connector free-ed underneath it 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 8cbbb002fefe..15a2c91581a8 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -1039,9 +1039,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;
@@ -1204,7 +1203,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, ntfy;
int ret;
int i;
@@ -1235,16 +1234,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;
}
@@ -1256,11 +1254,12 @@ static int ucsi_init(struct ucsi *ucsi)
if (ret < 0)
goto err_unregister;
+ ucsi->connector = connector;
ucsi->ntfy = ntfy;
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);
@@ -1269,10 +1268,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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 3/3] usb: ucsi_acpi: Increase the command completion timeout
2023-03-06 10:33 [PATCH v2 0/3] usb: ucsi: 3 bug fixes Hans de Goede
2023-03-06 10:33 ` [PATCH v2 1/3] usb: ucsi: Fix NULL pointer deref in ucsi_connector_change() Hans de Goede
2023-03-06 10:33 ` [PATCH v2 2/3] usb: ucsi: Fix ucsi->connector race Hans de Goede
@ 2023-03-06 10:33 ` Hans de Goede
2023-03-07 7:47 ` Heikki Krogerus
2 siblings, 1 reply; 9+ messages in thread
From: Hans de Goede @ 2023-03-06 10:33 UTC (permalink / raw
To: Greg Kroah-Hartman, Heikki Krogerus; +Cc: Hans de Goede, linux-usb, stable
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/3] usb: ucsi: Fix NULL pointer deref in ucsi_connector_change()
2023-03-06 10:33 ` [PATCH v2 1/3] usb: ucsi: Fix NULL pointer deref in ucsi_connector_change() Hans de Goede
@ 2023-03-07 7:32 ` Heikki Krogerus
2023-03-07 9:17 ` Heikki Krogerus
1 sibling, 0 replies; 9+ messages in thread
From: Heikki Krogerus @ 2023-03-07 7:32 UTC (permalink / raw
To: Hans de Goede; +Cc: Greg Kroah-Hartman, linux-usb, stable
On Mon, Mar 06, 2023 at 11:33:57AM +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 not setting ucsi->ntfy inside ucsi_init() until ucsi_init()
> has succeeded, so that ucsi_connector_change() ignores the events
> because UCSI_ENABLE_NTFY_CONNECTOR_CHANGE is not set in the ntfy mask.
>
> 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>
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
> Changes in v2:
> -Delay setting ucsi->ntfy in ucsi_init() instead of adding a NULL pointer
> check to ucsi_connector_change()
> ---
> drivers/usb/typec/ucsi/ucsi.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 1cf8947c6d66..8cbbb002fefe 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -1205,7 +1205,7 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
> static int ucsi_init(struct ucsi *ucsi)
> {
> struct ucsi_connector *con;
> - u64 command;
> + u64 command, ntfy;
> int ret;
> int i;
>
> @@ -1217,8 +1217,8 @@ static int ucsi_init(struct ucsi *ucsi)
> }
>
> /* Enable basic notifications */
> - ucsi->ntfy = UCSI_ENABLE_NTFY_CMD_COMPLETE | UCSI_ENABLE_NTFY_ERROR;
> - command = UCSI_SET_NOTIFICATION_ENABLE | ucsi->ntfy;
> + ntfy = UCSI_ENABLE_NTFY_CMD_COMPLETE | UCSI_ENABLE_NTFY_ERROR;
> + command = UCSI_SET_NOTIFICATION_ENABLE | ntfy;
> ret = ucsi_send_command(ucsi, command, NULL, 0);
> if (ret < 0)
> goto err_reset;
> @@ -1250,12 +1250,13 @@ static int ucsi_init(struct ucsi *ucsi)
> }
>
> /* Enable all notifications */
> - ucsi->ntfy = UCSI_ENABLE_NTFY_ALL;
> - command = UCSI_SET_NOTIFICATION_ENABLE | ucsi->ntfy;
> + ntfy = UCSI_ENABLE_NTFY_ALL;
> + command = UCSI_SET_NOTIFICATION_ENABLE | ntfy;
> ret = ucsi_send_command(ucsi, command, NULL, 0);
> if (ret < 0)
> goto err_unregister;
>
> + ucsi->ntfy = ntfy;
> return 0;
>
> err_unregister:
> --
> 2.39.1
--
heikki
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/3] usb: ucsi: Fix ucsi->connector race
2023-03-06 10:33 ` [PATCH v2 2/3] usb: ucsi: Fix ucsi->connector race Hans de Goede
@ 2023-03-07 7:46 ` Heikki Krogerus
0 siblings, 0 replies; 9+ messages in thread
From: Heikki Krogerus @ 2023-03-07 7:46 UTC (permalink / raw
To: Hans de Goede; +Cc: Greg Kroah-Hartman, linux-usb, stable
Hi Hans,
On Mon, Mar 06, 2023 at 11:33:58AM +0100, Hans de Goede wrote:
> ucsi_init() which runs from a workqueue sets ucsi->connector and
> on an error will clear it again.
>
> ucsi->connector gets dereferenced by ucsi_resume(), this checks 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 ucsi_resume() may pass,
> only to have ucsi->connector free-ed underneath it 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>
This does not apply anymore on top of Greg's usb-next. I think you
need to rebase. While at it, I have one nit below...
> ---
> 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 8cbbb002fefe..15a2c91581a8 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -1039,9 +1039,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)
If con->num was set before this function is called, you don't need
"index" at all:
static int ucsi_register_port(struct ucsi *ucsi, 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;
> @@ -1204,7 +1203,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, ntfy;
> int ret;
> int i;
> @@ -1235,16 +1234,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);
Assign it here:
connector[i].num = i + 1;
> + ret = ucsi_register_port(ucsi, i, &connector[i]);
> if (ret)
> goto err_unregister;
> }
> @@ -1256,11 +1254,12 @@ static int ucsi_init(struct ucsi *ucsi)
> if (ret < 0)
> goto err_unregister;
>
> + ucsi->connector = connector;
> ucsi->ntfy = ntfy;
> 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);
> @@ -1269,10 +1268,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);
thanks,
--
heikki
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 3/3] usb: ucsi_acpi: Increase the command completion timeout
2023-03-06 10:33 ` [PATCH v2 3/3] usb: ucsi_acpi: Increase the command completion timeout Hans de Goede
@ 2023-03-07 7:47 ` Heikki Krogerus
0 siblings, 0 replies; 9+ messages in thread
From: Heikki Krogerus @ 2023-03-07 7:47 UTC (permalink / raw
To: Hans de Goede; +Cc: Greg Kroah-Hartman, linux-usb, stable
On Mon, Mar 06, 2023 at 11:33:59AM +0100, Hans de Goede wrote:
> 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>
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.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
--
heikki
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/3] usb: ucsi: Fix NULL pointer deref in ucsi_connector_change()
2023-03-06 10:33 ` [PATCH v2 1/3] usb: ucsi: Fix NULL pointer deref in ucsi_connector_change() Hans de Goede
2023-03-07 7:32 ` Heikki Krogerus
@ 2023-03-07 9:17 ` Heikki Krogerus
2023-03-07 9:28 ` Greg Kroah-Hartman
1 sibling, 1 reply; 9+ messages in thread
From: Heikki Krogerus @ 2023-03-07 9:17 UTC (permalink / raw
To: Hans de Goede; +Cc: Greg Kroah-Hartman, linux-usb, stable
Hi Hans,
On Mon, Mar 06, 2023 at 11:33:57AM +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 not setting ucsi->ntfy inside ucsi_init() until ucsi_init()
> has succeeded, so that ucsi_connector_change() ignores the events
> because UCSI_ENABLE_NTFY_CONNECTOR_CHANGE is not set in the ntfy mask.
>
> 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>
There is now a bug report for this in the kernel.org bugzilla. Can you
add a Link tag pointing to it so the it gets updated automagically:
Link: https://bugzilla.kernel.org/show_bug.cgi?id=217106
Thanks,
> ---
> Changes in v2:
> -Delay setting ucsi->ntfy in ucsi_init() instead of adding a NULL pointer
> check to ucsi_connector_change()
> ---
> drivers/usb/typec/ucsi/ucsi.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 1cf8947c6d66..8cbbb002fefe 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -1205,7 +1205,7 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
> static int ucsi_init(struct ucsi *ucsi)
> {
> struct ucsi_connector *con;
> - u64 command;
> + u64 command, ntfy;
> int ret;
> int i;
>
> @@ -1217,8 +1217,8 @@ static int ucsi_init(struct ucsi *ucsi)
> }
>
> /* Enable basic notifications */
> - ucsi->ntfy = UCSI_ENABLE_NTFY_CMD_COMPLETE | UCSI_ENABLE_NTFY_ERROR;
> - command = UCSI_SET_NOTIFICATION_ENABLE | ucsi->ntfy;
> + ntfy = UCSI_ENABLE_NTFY_CMD_COMPLETE | UCSI_ENABLE_NTFY_ERROR;
> + command = UCSI_SET_NOTIFICATION_ENABLE | ntfy;
> ret = ucsi_send_command(ucsi, command, NULL, 0);
> if (ret < 0)
> goto err_reset;
> @@ -1250,12 +1250,13 @@ static int ucsi_init(struct ucsi *ucsi)
> }
>
> /* Enable all notifications */
> - ucsi->ntfy = UCSI_ENABLE_NTFY_ALL;
> - command = UCSI_SET_NOTIFICATION_ENABLE | ucsi->ntfy;
> + ntfy = UCSI_ENABLE_NTFY_ALL;
> + command = UCSI_SET_NOTIFICATION_ENABLE | ntfy;
> ret = ucsi_send_command(ucsi, command, NULL, 0);
> if (ret < 0)
> goto err_unregister;
>
> + ucsi->ntfy = ntfy;
> return 0;
>
> err_unregister:
> --
> 2.39.1
--
heikki
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/3] usb: ucsi: Fix NULL pointer deref in ucsi_connector_change()
2023-03-07 9:17 ` Heikki Krogerus
@ 2023-03-07 9:28 ` Greg Kroah-Hartman
0 siblings, 0 replies; 9+ messages in thread
From: Greg Kroah-Hartman @ 2023-03-07 9:28 UTC (permalink / raw
To: Heikki Krogerus; +Cc: Hans de Goede, linux-usb, stable
On Tue, Mar 07, 2023 at 11:17:05AM +0200, Heikki Krogerus wrote:
> Hi Hans,
>
> On Mon, Mar 06, 2023 at 11:33:57AM +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 not setting ucsi->ntfy inside ucsi_init() until ucsi_init()
> > has succeeded, so that ucsi_connector_change() ignores the events
> > because UCSI_ENABLE_NTFY_CONNECTOR_CHANGE is not set in the ntfy mask.
> >
> > 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>
>
> There is now a bug report for this in the kernel.org bugzilla. Can you
> add a Link tag pointing to it so the it gets updated automagically:
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217106
My tools should pick this up, thanks.
greg k-h
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-03-07 9:29 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-06 10:33 [PATCH v2 0/3] usb: ucsi: 3 bug fixes Hans de Goede
2023-03-06 10:33 ` [PATCH v2 1/3] usb: ucsi: Fix NULL pointer deref in ucsi_connector_change() Hans de Goede
2023-03-07 7:32 ` Heikki Krogerus
2023-03-07 9:17 ` Heikki Krogerus
2023-03-07 9:28 ` Greg Kroah-Hartman
2023-03-06 10:33 ` [PATCH v2 2/3] usb: ucsi: Fix ucsi->connector race Hans de Goede
2023-03-07 7:46 ` Heikki Krogerus
2023-03-06 10:33 ` [PATCH v2 3/3] usb: ucsi_acpi: Increase the command completion timeout Hans de Goede
2023-03-07 7:47 ` Heikki Krogerus
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.