Re: [PATCH v6 3/3] usb: typec: tipd: Add sn201202x support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Delayed works (`update_work` and `wq_poll`) are not cancelled on the probe error path in tipd_init, leading to a Use-After-Free.
--

commit 547c3b2aceecd19f6dae9a6a5703b8abccb6b9fc
Author: Sasha Finkelstein <k@chaosmail.tech>

usb: typec: tipd: Add sn201202x support
    
Add support for sn201202x (aka ACE3), a tipd variant that uses a very
similar register map, that is exposed over a "logical register"
interface on the SPMI bus.

> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index fb7bb68731bc5..64cde3a290fe6 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
[ ... ]
> @@ -1950,11 +1950,32 @@ static const struct tipd_data tipd_tps25750_data = {
>  	.connect = tps6598x_connect,
>  };
>  
> +static const struct tipd_data tipd_sn201202x_data = {
> +	.irq_handler = cd321x_interrupt,
> +	.irq_mask1 = APPLE_CD_REG_INT_POWER_STATUS_UPDATE |
> +		     APPLE_CD_REG_INT_DATA_STATUS_UPDATE |
> +		     APPLE_CD_REG_INT_PLUG_EVENT,
> +	.tps_struct_size = sizeof(struct sn201202x),
> +	.remove = cd321x_remove,
> +	.register_port = cd321x_register_port,
> +	.unregister_port = cd321x_unregister_port,

[Severity: High]
This is a pre-existing issue, but does cd321x_unregister_port() completely
miss cancelling the update_work?

When tipd_init() calls the err_disconnect cleanup path on probe failure, it
calls this unregister_port callback:

drivers/usb/typec/tipd/core.c:cd321x_unregister_port() {
	...
	typec_mux_put(cd321x->mux);
	cd321x->mux = NULL;
}

Since the delayed work isn't cancelled here or in err_disconnect, the
update_work or wq_poll can run 500ms later and try to access the freed
cd321x or tps6598x device structure, causing a use-after-free.

> +	.trace_data_status = trace_cd321x_data_status,
[ ... ]
> diff --git a/drivers/usb/typec/tipd/spmi.c b/drivers/usb/typec/tipd/spmi.c
> new file mode 100644
> index 0000000000000..a165ec39a8ed8
> --- /dev/null
> +++ b/drivers/usb/typec/tipd/spmi.c
[ ... ]
> +static int sn201202x_probe(struct spmi_device *device)
> +{
[ ... ]
> +	ret = spmi_command_wakeup(device);
> +	if (ret)
> +		return ret;
> +	if (!wait_for_completion_timeout(&sn->wake_completion, msecs_to_jiffies(100)))
> +		return -ETIMEDOUT;
> +
> +	ret = tipd_init(tps);
> +	if (ret)
> +		spmi_command_sleep(device);
> +	return ret;
> +}

[Severity: High]
This isn't a bug introduced by this patch, but could this error path
trigger the use-after-free mentioned above?

If tipd_init() fails after the delayed works are scheduled (for instance,
if devm_request_threaded_irq returns an error, or if polling mode is used
and connect() fails), tipd_init() jumps to err_disconnect:

drivers/usb/typec/tipd/core.c:tipd_init() {
	...
	if (ret)
		goto err_disconnect;
	...
}

This returns the error code, causing the driver core devres framework to
free the memory allocated for the device structure, leaving the delayed
work active and accessing freed memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826-tipd-ace3-v6-0-e919f39a93a2@chaosmail.tech?part=3