Re: [PATCH 03/19] usb: typec: tipd: Hook up Thunderbolt switch for CD321x

[sashiko-bot@kernel.org]

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 <sven@kernel.org>

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.

> 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

[ ... ]

> @@ -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?

> +		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);

[ ... ]

> @@ -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?

[ ... ]

> @@ -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