| Message ID | 20230306103359.6591-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: ucsi: 3 bug fixes | expand |
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
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
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
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:
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(-)