Re: [PATCH 03/19] usb: typec: tipd: Hook up Thunderbolt switch for CD321x
flat view
From: sashiko-bot@kernel.org
Date: 2026-08-30 20:36:15
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: New issues: - [Medium] Uninitialized stack padding in `struct enter_usb_data` is copied into `tbt_switch_data` and passed to a generic callback, potentially leaking kernel stack memory. - [Medium] The Thunderbolt switch is not initialized to the OFF state during probe, nor is it explicitly turned off during driver removal, potentially leaving the hardware state desynchronized. Pre-existing issues: - [Critical] Pending delayed work `cd321x->update_work` is not cancelled in `tps6598x_probe()` error path, leading to Use-After-Free. -- commit 7c593740c56d0ad285a4bc2f76d978af8ae887d9 Author: Sven Peter [off-list ref] usb: typec: tipd: Hook up Thunderbolt switch for CD321x Look up the Thunderbolt switch for CD321x ports and forward cable details whenever a Thunderbolt or USB4 connection comes up or goes away.
quoted hunk ↗ jump to hunk
diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c index 522f56742aa9b..5016f9dba9560 100644 --- a/drivers/usb/typec/tipd/core.c +++ b/drivers/usb/typec/tipd/core.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -738,6 +745,13 @@ static void cd321x_typec_update_mode(struct tps6598x *tps, struct cd321x_status cd321x->state.mode = TYPEC_TBT_MODE; cd321x->state.data = &tbt_data; typec_mux_set(cd321x->mux, &cd321x->state); + + tbt_switch_data.state = TYPEC_THUNDERBOLT_SWITCH_TBT; + tbt_switch_data.tbt = tbt_data; + tbt_switch_data.orientation = TPS_STATUS_TO_UPSIDE_DOWN(st->status) ? + TYPEC_ORIENTATION_REVERSE : + TYPEC_ORIENTATION_NORMAL; + typec_thunderbolt_switch_set(cd321x->tbt_switch, &tbt_switch_data); } else if (st->data_status & CD321X_DATA_STATUS_USB4_CONNECTION) { struct enter_usb_data eusb_data;@@ -752,12 +766,20 @@ static void cd321x_typec_update_mode(struct tps6598x *tps, struct cd321x_status cd321x->state.data = &eusb_data; cd321x->state.mode = TYPEC_MODE_USB4; typec_mux_set(cd321x->mux, &cd321x->state); + + tbt_switch_data.state = TYPEC_THUNDERBOLT_SWITCH_USB4; + tbt_switch_data.usb4 = eusb_data;
[Severity: Medium] Does this struct assignment copy uninitialized stack memory into the union? eusb_data is declared earlier as struct enter_usb_data eusb_data; without zero-initialization. Because the struct contains a 4-byte u32 and a 1-byte bitfield, the compiler pads it to an 8-byte boundary, leaving uninitialized stack memory in the padding bytes. Assigning it directly to tbt_switch_data.usb4 copies these uninitialized padding bytes over the previously zeroed tbt_switch_data union payload. Could this be passed to typec_thunderbolt_switch_set() and potentially leak kernel stack memory across driver subsystem boundaries?
quoted hunk ↗ jump to hunk
+ tbt_switch_data.orientation = TPS_STATUS_TO_UPSIDE_DOWN(st->status) ? + TYPEC_ORIENTATION_REVERSE : + TYPEC_ORIENTATION_NORMAL; + typec_thunderbolt_switch_set(cd321x->tbt_switch, &tbt_switch_data);
[ ... ]
quoted hunk ↗ jump to hunk
@@ -1328,6 +1354,12 @@ cd321x_register_port(struct tps6598x *tps, struct fwnode_handle *fwnode) goto err_unregister_altmodes; } + cd321x->tbt_switch = fwnode_typec_thunderbolt_switch_get(fwnode); + if (IS_ERR(cd321x->tbt_switch)) { + ret = PTR_ERR(cd321x->tbt_switch); + goto err_unregister_mux; + } + cd321x->state.alt = NULL; cd321x->state.mode = TYPEC_STATE_SAFE; cd321x->state.data = NULL; typec_set_mode(tps->port, TYPEC_STATE_SAFE);
[Severity: Medium] Does this leave the Thunderbolt switch hardware in an unknown state? The software state is initialized to TYPEC_STATE_SAFE and the main Type-C mux is reset by typec_set_mode(), but the newly added Thunderbolt switch is not explicitly set to TYPEC_THUNDERBOLT_SWITCH_OFF. If the bootloader left the Thunderbolt switch active, the hardware and software states might become desynchronized. Later, if a connection event occurs without a data connection, cd321x_typec_update_mode() returns early, and the switch is never explicitly turned off. Should there be an explicit call to initialize the tbt_switch to the off state here? [ ... ]
quoted hunk ↗ jump to hunk
@@ -1356,6 +1391,8 @@ cd321x_unregister_port(struct tps6598x *tps) { struct cd321x *cd321x = container_of(tps, struct cd321x, tps);
[Severity: Critical] This is a pre-existing issue, but does this teardown function leak the update_work delayed work? cd321x_unregister_port() is missing a call to cancel the update_work delayed work. If a partner is connected during probe, tps6598x_probe() schedules cd321x->update_work. If a subsequent probe step fails (such as devm_request_threaded_irq()), the driver jumps to the err_disconnect path and returns an error. This path fails to cancel the delayed work before devres frees the tps structure, which can lead to a use-after-free crash when the work executes and dereferences the freed memory. Should cancel_delayed_work_sync(&cd321x->update_work) be added to the teardown paths?
+ typec_thunderbolt_switch_put(cd321x->tbt_switch); + cd321x->tbt_switch = NULL;
[Severity: Medium] Is it necessary to turn off the Thunderbolt switch before dropping its reference during driver removal? The tbt_switch reference is dropped here without commanding the hardware to turn off, which might leave the switch active.
typec_mux_put(cd321x->mux); cd321x->mux = NULL; typec_unregister_altmode(cd321x->port_altmode_dp);
-- Sashiko AI review · https://sashiko.dev/#/patchset/20260830-b4-apple-soc-tbt-v1-0-44bc9348683c@kernel.org?part=3