mbox series

[0/2] Fix invalid pointer access

Message ID 20250321143728.4092417-1-akuchynski@chromium.org
Headers show
Series Fix invalid pointer access | expand

Message

Andrei Kuchynski March 21, 2025, 2:37 p.m. UTC 
These patches fix bugs in the USB Type-C class driver, addressing a NULL
pointer dereference and invalid pointer usage during partner
registration / unregistration.

1. The first patch resolves NULL pointer dereference resulting from a race
condition where the port driver and USB hub driver simultaneously invoke
typec_partner_unlink_device. The crash originated from dev->kdef.name being
NULL during the second unlinking process.

The crash log:
 BUG: kernel NULL pointer dereference, address: 0000000000000000
 CPU: 0 PID: 55688 Comm: kworker/0:2 Tainted: G U 6.6.56-05934
 Hardware name: Google Redrix/Redrix, BIOS Google_Redrix.14505.778.0
 Workqueue: events cros_typec_port_work [cros_ec_typec]
 RIP: 0010:strlen+0xb/0x20
 RSP: 0000:ffffbe400c5b3b90 EFLAGS: 00010202
 RAX: ffffffffffffffff RBX: 0000000000000000 RCX: 0000000000000000
 RDX: 0000000000000001 RSI: 0000000000000000 RDI: 0000000000000000
 RBP: ffffbe400c5b3b90 R08: ffffa2a011187000 R09: ffffffff8d633a4d
 R10: 0000000000000000 R11: ffffffffc092a6b0 R12: ffffa29ff4d02990
 R13: ffffa29ff4d02d98 R14: 0000000000000000 R15: 0000000000000000
 FS:  0000000000000000(0000) GS:ffffa2a6df800000(0000) knlGS:00000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 CR2: 0000000000000000 CR3: 0000000191cb0000 CR4: 0000000000750ef0
 Call Trace:
  <TASK>
  ? __die_body+0x68/0xb0
  ? page_fault_oops+0x379/0x3e0
  ? ep_poll_callback+0x132/0x260
  ? exc_page_fault+0x4f/0xa0
  ? asm_exc_page_fault+0x22/0x30
  ? __pfx_pmc_usb_mux_set+0x10/0x10 [intel_pmc_mux (HASH:1400 30)]
  ? kobject_uevent_env+0x13d/0x3c0
  ? strlen+0xb/0x20
  kernfs_name_hash+0x17/0x90
  kernfs_find_ns+0x3e/0xe0
  kernfs_remove_by_name_ns+0x47/0xb0
  typec_unregister_partner+0x4f/0xd0 [typec (HASH:1400 31)]
  cros_typec_remove_partner+0x19f/0x200 [cros_ec_typec (HASH:1400 32)]
  cros_typec_port_update+0x555/0x1690 [cros_ec_typec (HASH:1400 32)]
  ? snprintf+0xd0/0x640
  ? cros_ec_typec_event+0x2a/0x40 [cros_ec_typec (HASH:1400 32)]
  ? blocking_notifier_call_chain+0x73/0xf0
  cros_typec_port_work+0x34/0x60 [cros_ec_typec (HASH:1400 32)]
  worker_thread+0x395/0x940
  kthread+0xed/0x110
  ? __pfx_worker_thread+0x10/0x10
  ? __pfx_kthread+0x10/0x10
  ret_from_fork+0x38/0x50
  ? __pfx_kthread+0x10/0x10
  ret_from_fork_asm+0x1b/0x30
  </TASK>

2. The USB HUB driver's failure to notify the Type-C class driver about
device detachments resulted in typec_partner_detach() not being invoked.
This omission caused the port driver to retain a reference to a released
device, triggering a kernel WARNING upon unplug. Furthermore, when a new
device was connected, typec_register_partner() attempted to use an obsolete
port->usb3_dev pointer. The crash, occurring in typec_register_partner(),
is resolved in a second patch.

The WARNING log occurred during device unplug:
[534629.006366] kernfs: can not remove 'typec', no directory
 WARNING: CPU: 0 PID: 864020 at fs/kernfs/dir.c:1684
 kernfs_remove_by_name_ns+0x79/0xb0
 CPU: 0 PID: 864020 Comm: kworker/0:2 Tainted: G U 6.6.56-05934
 Hardware name: Google Screebo/Screebo, BIOS Google_Screebo.15709.185.0
 Workqueue: events cros_typec_port_work [cros_ec_typec]
 RIP: 0010:kernfs_remove_by_name_ns+0x79/0xb0
 Call Trace:
  <TASK>
  ? __warn+0xca/0x1c0
  ? kernfs_remove_by_name_ns+0x79/0xb0
  ? report_bug+0x14e/0x1f0
  ? handle_bug+0x41/0x70
  ? exc_invalid_op+0x1b/0x50
  ? asm_exc_invalid_op+0x16/0x20
  ? kernfs_remove_by_name_ns+0x79/0xb0
  typec_unregister_partner+0x5e/0xd0 [typec (HASH:1400 25)]
  cros_typec_remove_partner+0x19f/0x200 [cros_ec_typec (HASH:1400 26)]
  cros_typec_port_update+0x4a0/0x12a0 [cros_ec_typec (HASH:1400 26)]
  ? cros_usbpd_charger_ec_command+0x20/0xd0
  ? __queue_work+0x3f4/0x520
  ? queue_work_on+0x6a/0x80
  ? cros_ec_typec_event+0x2a/0x40 [cros_ec_typec (HASH:1400 26)]
  ? blocking_notifier_call_chain+0xce/0xf0
  cros_typec_port_work+0x34/0x60 [cros_ec_typec (HASH:1400 26)]
  process_scheduled_works+0x299/0x490
  worker_thread+0x2d7/0x3b0

The crash log occurred during the subsequent plug:
[536103.301256] BUG: kernel NULL pointer dereference,
 address: 000000000000050b
 Oops: 0000 [#1] PREEMPT SMP NOPTI
 CPU: 0 PID: 864020 Comm: kworker/0:2 Tainted: G U 6.6.56-05934
 Hardware name: Google Screebo/Screebo, BIOS Google_Screebo.15709.185.0
 Workqueue: events cros_typec_port_work [cros_ec_typec]
 RIP: 0010:kernfs_new_node+0x1b/0x80
 RSP: 0018:ffffbd318c433bd8 EFLAGS: 00010246
 RAX: 0000000000000004 RBX: 0000000000000498 RCX: 000000000000a1ff
 RDX: ffffffffc0a3b435 RSI: ffffffffc0a3b435 RDI: 0000000000000498
 RBP: ffffbd318c433be0 R08: 0000000000000000 R09: 0000000000000000
 R10: 0000000000000000 R11: ffffffffa61156d0 R12: ffff9e573ad2d400
 R13: ffff9e5653c47e10 R14: 0000000000000498 R15: 0000000000000498
 FS:  0000000000000000(0000) GS:ffff9e5d5fe00000(0000) knlGS:00000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 CR2: 000000000000050b CR3: 00000002ea838003 CR4: 0000000000770ef0
 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
 DR3: 0000000000000000 DR6: 00000000ffff07f0 DR7: 0000000000000400
 Call Trace:
  <TASK>
  ? __die_body+0x68/0xb0
  ? page_fault_oops+0x379/0x3e0
  ? exc_page_fault+0x4f/0xa0
  ? asm_exc_page_fault+0x22/0x30
  ? __pfx_klist_class_dev_get+0x10/0x10
  ? kernfs_new_node+0x1b/0x80
  kernfs_create_link+0x3d/0xa0
  sysfs_do_create_link_sd+0x68/0xd0
  typec_register_partner+0x12f/0x220 [typec (HASH:1400 25)]
  cros_typec_port_update+0x5dc/0x12a0 [cros_ec_typec (HASH:1400 26)]
  ? cros_usbpd_charger_ec_command+0x20/0xd0
  ? __queue_work+0x3f4/0x520
  ? cros_ec_typec_event+0x2a/0x40 [cros_ec_typec (HASH:1400 26)]
  ? blocking_notifier_call_chain+0xce/0xf0
  cros_typec_port_work+0x34/0x60 [cros_ec_typec (HASH:1400 26)]
  process_scheduled_works+0x299/0x490
  worker_thread+0x2d7/0x3b0
  kthread+0xed/0x110
  ? __pfx_worker_thread+0x10/0x10
  ? __pfx_kthread+0x10/0x10
  ret_from_fork+0x38/0x50
  ? __pfx_kthread+0x10/0x10
  ret_from_fork_asm+0x1b/0x30
  </TASK>

Andrei Kuchynski (2):
  usb: typec: class: Fix NULL pointer access
  usb: typec: class: Invalidate USB device pointers on partner
    unregistration

 drivers/usb/typec/class.c | 23 +++++++++++++++++++----
 drivers/usb/typec/class.h |  1 +
 2 files changed, 20 insertions(+), 4 deletions(-)

Comments

Benson Leung March 21, 2025, 11:16 p.m. UTC | #1 
Hi Andrei,


On Fri, Mar 21, 2025 at 02:37:26PM +0000, Andrei Kuchynski wrote:
> Concurrent calls to typec_partner_unlink_device can lead to a NULL pointer
> dereference. This patch adds a mutex to protect USB device pointers and
> prevent this issue. The same mutex protects both the device pointers and
> the partner device registration.
> 
> Cc: stable@vger.kernel.org
> Fixes: 59de2a56d127 ("usb: typec: Link enumerated USB devices with Type-C partner")       
> Signed-off-by: Andrei Kuchynski <akuchynski@chromium.org>

Reviewed-by: Benson Leung <bleung@chromium.org>

> ---
>  drivers/usb/typec/class.c | 15 +++++++++++++--
>  drivers/usb/typec/class.h |  1 +
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index 9c76c3d0c6cf..eadb150223f8 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -1052,6 +1052,7 @@ struct typec_partner *typec_register_partner(struct typec_port *port,
>  		partner->usb_mode = USB_MODE_USB3;
>  	}
>  
> +	mutex_lock(&port->partner_link_lock);
>  	ret = device_register(&partner->dev);
>  	if (ret) {
>  		dev_err(&port->dev, "failed to register partner (%d)\n", ret);
> @@ -1063,6 +1064,7 @@ struct typec_partner *typec_register_partner(struct typec_port *port,
>  		typec_partner_link_device(partner, port->usb2_dev);
>  	if (port->usb3_dev)
>  		typec_partner_link_device(partner, port->usb3_dev);
> +	mutex_unlock(&port->partner_link_lock);
>  
>  	return partner;
>  }
> @@ -1083,12 +1085,14 @@ void typec_unregister_partner(struct typec_partner *partner)
>  
>  	port = to_typec_port(partner->dev.parent);
>  
> +	mutex_lock(&port->partner_link_lock);
>  	if (port->usb2_dev)
>  		typec_partner_unlink_device(partner, port->usb2_dev);
>  	if (port->usb3_dev)
>  		typec_partner_unlink_device(partner, port->usb3_dev);
>  
>  	device_unregister(&partner->dev);
> +	mutex_unlock(&port->partner_link_lock);
>  }
>  EXPORT_SYMBOL_GPL(typec_unregister_partner);
>  
> @@ -2041,10 +2045,11 @@ static struct typec_partner *typec_get_partner(struct typec_port *port)
>  static void typec_partner_attach(struct typec_connector *con, struct device *dev)
>  {
>  	struct typec_port *port = container_of(con, struct typec_port, con);
> -	struct typec_partner *partner = typec_get_partner(port);
> +	struct typec_partner *partner;
>  	struct usb_device *udev = to_usb_device(dev);
>  	enum usb_mode usb_mode;
>  
> +	mutex_lock(&port->partner_link_lock);
>  	if (udev->speed < USB_SPEED_SUPER) {
>  		usb_mode = USB_MODE_USB2;
>  		port->usb2_dev = dev;
> @@ -2053,18 +2058,22 @@ static void typec_partner_attach(struct typec_connector *con, struct device *dev
>  		port->usb3_dev = dev;
>  	}
>  
> +	partner = typec_get_partner(port);
>  	if (partner) {
>  		typec_partner_set_usb_mode(partner, usb_mode);
>  		typec_partner_link_device(partner, dev);
>  		put_device(&partner->dev);
>  	}
> +	mutex_unlock(&port->partner_link_lock);
>  }
>  
>  static void typec_partner_deattach(struct typec_connector *con, struct device *dev)
>  {
>  	struct typec_port *port = container_of(con, struct typec_port, con);
> -	struct typec_partner *partner = typec_get_partner(port);
> +	struct typec_partner *partner;
>  
> +	mutex_lock(&port->partner_link_lock);
> +	partner = typec_get_partner(port);
>  	if (partner) {
>  		typec_partner_unlink_device(partner, dev);
>  		put_device(&partner->dev);
> @@ -2074,6 +2083,7 @@ static void typec_partner_deattach(struct typec_connector *con, struct device *d
>  		port->usb2_dev = NULL;
>  	else if (port->usb3_dev == dev)
>  		port->usb3_dev = NULL;
> +	mutex_unlock(&port->partner_link_lock);
>  }
>  
>  /**
> @@ -2614,6 +2624,7 @@ struct typec_port *typec_register_port(struct device *parent,
>  
>  	ida_init(&port->mode_ids);
>  	mutex_init(&port->port_type_lock);
> +	mutex_init(&port->partner_link_lock);
>  
>  	port->id = id;
>  	port->ops = cap->ops;
> diff --git a/drivers/usb/typec/class.h b/drivers/usb/typec/class.h
> index b3076a24ad2e..db2fe96c48ff 100644
> --- a/drivers/usb/typec/class.h
> +++ b/drivers/usb/typec/class.h
> @@ -59,6 +59,7 @@ struct typec_port {
>  	enum typec_port_type		port_type;
>  	enum usb_mode			usb_mode;
>  	struct mutex			port_type_lock;
> +	struct mutex			partner_link_lock;
>  
>  	enum typec_orientation		orientation;
>  	struct typec_switch		*sw;
> -- 
> 2.49.0.395.g12beb8f557-goog
>