Re: [PATCH v4 2/2] usb: typec: ucsi: Add ITE IT8851/IT8853 Type-C PD controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Use-After-Free vulnerability in teardown sequence due to incorrect free_irq() placement.
- [High] Unhandled hardware events or I2C errors in the interrupt handler skip acknowledging the interrupt, causing an interrupt storm.
--

--- Patch [2]: [PATCH v4 2/2] usb: typec: ucsi: Add ITE IT8851/IT8853 Type-C PD controller driver ---
commit 17d94bce1f3a61576b21089ce840ef1e6562fa4c
Author: Amber Kao <amber.kao@ite.com.tw>

usb: typec: ucsi: Add ITE IT8851/IT8853 Type-C PD controller driver

This commit adds a UCSI driver for the ITE IT8851 and IT8853 I2C-based
USB Type-C Power Delivery controllers. It implements the I2C interface to
read and write UCSI commands and handles shared interrupt lines to forward
events to the UCSI core.

> diff --git a/drivers/usb/typec/ucsi/ucsi_itepd.c b/drivers/usb/typec/ucsi/ucsi_itepd.c
> new file mode 100644
> index 0000000000000..ceaadb0b3fef6
> --- /dev/null
> +++ b/drivers/usb/typec/ucsi/ucsi_itepd.c

[ ... ]

> +static int itepd_process_event(struct itepd *itepd, u32 *cci)
> +{
> +	u8 msg_in[ITEPD_MSG_IN_MAX_LEN] = {};
> +	__le32 le_cci;
> +	u8 event, ack;
> +	u8 len = 0;
> +	int ret;
> +
> +	mutex_lock(&itepd->event_lock);
> +
> +	ret = itepd_read_reg(itepd, ITEPD_VENDOR_INT, &event, sizeof(event));
> +	if (ret)
> +		goto out_unlock;
> +
> +	event &= ITEPD_ALERT_VDM_EVENT | ITEPD_ALERT_UCSI_EVENT;
> +	if (!event) {
> +		mutex_lock(&itepd->received_lock);
> +		*cci = itepd->cci;
> +		mutex_unlock(&itepd->received_lock);
> +		ret = ITEPD_EVENT_NONE;
> +		goto out_unlock;
> +	}

[Severity: High]
If an unrecognized event occurs (neither VDM nor UCSI) and this path takes
the goto out_unlock, it skips writing to ITEPD_VENDOR_WC_INT to clear the
interrupt. Will this leave the interrupt line asserted and cause an
interrupt storm once the IRQ is unmasked?

> +
> +	if (event & ITEPD_ALERT_UCSI_EVENT) {
> +		ret = itepd_read_reg(itepd, ITEPD_UCSI_CCI_REG, &le_cci,
> +				     sizeof(le_cci));
> +		if (ret)
> +			goto out_unlock;

[Severity: High]
Similarly, if this I2C read fails (for example, if the kzalloc inside
itepd_read_reg fails), this goto out_unlock also skips acknowledging the
interrupt. Can this lead to an interrupt storm?

[ ... ]

> +static void itepd_remove(struct i2c_client *client)
> +{
> +	struct itepd *itepd = i2c_get_clientdata(client);
> +
> +	ucsi_unregister(itepd->ucsi);
> +	free_irq(client->irq, itepd);

[Severity: High]
Could this sequence result in a use-after-free? ucsi_unregister() disables
notifications and frees ucsi->connector. If an interrupt fires or is
already pending before free_irq() is called, the threaded interrupt handler
itepd_irq_thread_fn() can run, process the event, and call
ucsi_notify_common().

In ucsi_notify_common(), this could lead to dereferencing the freed
ucsi->connector array via ucsi_connector_change(). Should the IRQ be
freed or disabled prior to unregistering UCSI?

> +	ucsi_destroy(itepd->ucsi);
> +}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717-ucsi-itepd-v4-0-f3ba5addd0df@ite.com.tw?part=2