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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider:
- [High] DMA safety violation passing stack-allocated variables directly to `i2c_transfer()`.
- [High] Incorrect `devm` unwind ordering causes Use-After-Free of Type-C resources during workqueue flush.
- [High] Race condition during driver removal causing Use-After-Free by destroying the UCSI instance and workqueue before unregistering the interrupt callback.
- [High] Out-of-bounds array access due to unvalidated port index extracted from UCSI commands and hardware responses.
- [High] Race condition during probe where premature interrupt callback registration causes a NULL pointer dereference of the `ucsi` instance.
- [High] Uninitialized variables `ret` and `data` in `ucsi_itepd_connector_change_work` lead to undefined behavior and stack data leaks.
- [High] Missing lock protection when reading `itepd->altmode_cb` leads to data races and potential TOCTOU NULL pointer dereference.
- [High] Use of uninitialized stack memory during UCSI response parsing (`msg_in`).
- [High] Multi-byte native types (u64, u32, u16) are copied directly to/from I2C byte buffers, breaking on Big Endian architectures.
- [High] A pending initial hardware interrupt is explicitly acknowledged during probe but the event is permanently dropped.
- [Medium] `free_irq()` is called unconditionally during cleanup, even if the IRQ was never successfully requested.
- [Medium] Data races on `dp_en` and `con_change_processed` flags due to missing synchronization between workqueues.
--

commit 06a7873d268be15ee101449d82fa1a3ba9e341f7
Author: Amber Kao <amber.kao@ite.com.tw>

usb: typec: ucsi: Add ITE IT885x Type-C PD controller driver

Add core, UCSI, and Alternate Mode support for the ITE IT885x
Type-C Power Delivery controller over I2C. The driver uses the
auxiliary bus to spawn UCSI and Alternate Mode child devices from
the main I2C core driver.

> diff --git a/drivers/usb/typec/ucsi/itepd.c b/drivers/usb/typec/ucsi/itepd.c
> new file mode 100644
> index 0000000000000..39ac1c1361572
> --- /dev/null
> +++ b/drivers/usb/typec/ucsi/itepd.c
[ ... ]
> +static int itepd_read_reg(struct itepd *itepd, u8 reg, void *data, u32 len)
> +{
> +	struct i2c_client *client = itepd->client;
> +	struct i2c_msg msg[] = {
> +		{
> +			.addr	= client->addr,
> +			.flags	= 0x0,
> +			.len	= 1,
> +			.buf	= &reg,
> +		},
> +		{
> +			.addr	= client->addr,
> +			.flags	= I2C_M_RD,
> +			.len	= len,
> +			.buf	= (u8 *)data,
> +		}
> +	};

[Severity: High]
Could this cause DMA mapping errors? The stack-allocated msg array passes
the addresses of the local variables reg and data. The I2C subsystem
requires all buffers passed to i2c_transfer to be DMA-safe. If the I2C
adapter utilizes DMA, mapping stack memory can cause cache coherency issues
or trigger a panic if CONFIG_DMA_API_DEBUG is enabled.

[ ... ]
> +static int itepd_write_reg(struct itepd *itepd, u8 reg, const void *data, u32 len)
> +{
[ ... ]
> +	buf[0] = reg;
> +	memcpy(buf + sizeof(reg), (u8 *)data, len);

[Severity: High]
Might this break on big-endian architectures? When a multi-byte native
type like u64 or u32 is passed into data and copied directly into the I2C
buffer via memcpy, the most significant bytes are placed first on a big
endian system. Should this use explicit byte-order conversions like
cpu_to_le64 or le32_to_cpu?

[ ... ]
> +int itepd_mode(struct device *dev, u8 port, u8 mux, u32 config, u32 status)
> +{
[ ... ]
> +		itepd->altmode_data[port].dp_status = status;
> +		if (itepd->altmode_cb)
> +			itepd->altmode_cb->notify(itepd->altmode_cb->priv,
> +						  &itepd->altmode_data[port]);

[Severity: High]
Is there a potential TOCTOU race condition here? The altmode callback pointer
is checked and invoked without holding the cb_lock mutex. If
itepd_altmode_remove unregisters the callback concurrently, altmode_cb
could become NULL right after the check, leading to a dereference panic.
Does itepd_hpd share this same unprotected pattern?

[ ... ]
> +static int itepd_probe(struct i2c_client *client)
> +{
[ ... ]
> +	desc = devm_gpiod_get(dev, NULL, GPIOD_IN);
> +
> +	if (IS_ERR(desc)) {
> +		dev_info(dev, "get gpios from DTS failed\n");
> +	} else {
> +		if (gpiod_get_value(desc))
> +			itepd_irq_process(itepd);
> +	}

[Severity: High]
Could this manually trigger an interrupt drop? At this point in the probe
sequence, the auxiliary devices and callbacks are not yet added. Calling
itepd_irq_process clears the pending hardware events by writing to
ITEPD_VENDOR_WC_INT, but since the callbacks are NULL, the event data is
discarded without notifying the UCSI or altmode subsystems.

[ ... ]
> +out_release_altmode_aux:
> +	if (itepd->client_mask & BIT(ITEPD_CLIENT_ALTMODE))
> +		itepd_del_aux_device(itepd->altmode_aux);
> +out_free_irq:
> +	free_irq(itepd->irq, itepd);
> +	return ret;
> +}
> +
> +static void itepd_remove(struct i2c_client *client)
> +{
> +	struct itepd *itepd = i2c_get_clientdata(client);
> +
> +	if (itepd->client_mask & BIT(ITEPD_CLIENT_UCSI))
> +		itepd_del_aux_device(itepd->ucsi_aux);
> +	if (itepd->client_mask & BIT(ITEPD_CLIENT_ALTMODE))
> +		itepd_del_aux_device(itepd->altmode_aux);
> +	free_irq(itepd->irq, itepd);
> +}

[Severity: Medium]
Is it safe to unconditionally call free_irq here? The probe function correctly
checks if itepd->irq > 0 before calling request_threaded_irq, but the error
handling and remove paths free the IRQ regardless. If the device was probed
without a valid IRQ, wouldn't this attempt to free an unassigned IRQ line
and trigger a warning?

> diff --git a/drivers/usb/typec/ucsi/itepd_altmode.c b/drivers/usb/typec/ucsi/itepd_altmode.c
> new file mode 100644
> index 0000000000000..a88ae2fb6e798
> --- /dev/null
> +++ b/drivers/usb/typec/ucsi/itepd_altmode.c
[ ... ]
> +static int itepd_altmode_probe(struct auxiliary_device *adev,
> +			       const struct auxiliary_device_id *id)
> +{
[ ... ]
> +		alt_port->ordered_wq = alloc_ordered_workqueue("itepd_altmode_%u", 0, port);
> +		if (!alt_port->ordered_wq) {
> +			fwnode_handle_put(fwnode);
> +			return -ENOMEM;
> +		}
> +
> +		ret = devm_add_action_or_reset(dev, itepd_altmode_destroy_wq,
> +					       alt_port->ordered_wq);
> +		if (ret) {
> +			fwnode_handle_put(fwnode);
> +			return ret;
> +		}
[ ... ]
> +		alt_port->typec_mux = fwnode_typec_mux_get(fwnode);
> +		if (IS_ERR(alt_port->typec_mux)) {

[Severity: High]
Does this devm initialization order create a use-after-free risk during
device removal? The action to destroy the workqueue is added before the
typec_mux, typec_retimer, and typec_switch are acquired and their put actions
registered. Since devm unwinds in LIFO order, the workqueue flush will
execute after the Type-C resources have been freed. If a worker is pending,
it might dereference the already freed typec resource pointers.

> diff --git a/drivers/usb/typec/ucsi/ucsi_itepd.c b/drivers/usb/typec/ucsi/ucsi_itepd.c
> new file mode 100644
> index 0000000000000..ea55ff7b866ee
> --- /dev/null
> +++ b/drivers/usb/typec/ucsi/ucsi_itepd.c
[ ... ]
> +static void ucsi_itepd_connector_change_work(struct work_struct *work)
> +{
> +	struct ucsi_itepd_work *worker = container_of(work, struct ucsi_itepd_work, work);
> +	struct ucsi_itepd *ucsi_itepd = worker->ucsi_itepd;
> +	u8 data[11];
> +	u8 num_vdos;
> +	u32 status;
> +	u64 cmd;
> +	int ret;
> +
> +	if (ucsi_itepd->con_change_processed[worker->port])
> +		goto out;
> +
> +	ucsi_itepd->con_change_processed[worker->port] = true;

[Severity: Medium]
Is there a data race on these array flags? The flags dp_en and
con_change_processed are modified in ucsi_itepd_connector_status, which runs
on the core system_long_wq, but they are read and modified here on the local
ordered_wq. Without mutual exclusion, could this result in skipped
evaluations or out-of-order state transitions?

> +	if (ucsi_itepd->dp_en[worker->port]) {
> +		/* UCSI_GET_ATTENTION_VDO (0x16) */
> +		cmd = UCSI_COMMAND(UCSI_GET_ATTENTION_VDO) |
> +		      UCSI_CONNECTOR_NUMBER(worker->port + 1);
> +		ret = ucsi_send_command(ucsi_itepd->ucsi, cmd, data, 11);
> +	}
> +	if (ret < 0)
> +		goto out;

[Severity: High]
Can this lead to uninitialized stack variable usage? If dp_en is false, the
block that calls ucsi_send_command is skipped, leaving ret and data
uninitialized. Depending on the garbage value of ret, it might skip the
connector change notification or proceed to read random stack values from
data to pass into itepd_hpd.

[ ... ]
> +static void ucsi_itepd_response_hook(struct ucsi_itepd *ucsi_itepd,
> +				     u32 *cci, u8 *msg_in)
> +{
> +	u8 recipient;
> +	u8 offset;
> +	struct ucsi_itepd_work *worker;
> +
> +	if (((*cci & UCSI_CCI_COMMAND_COMPLETE) == 0) &&
> +	    UCSI_CCI_CONNECTOR(*cci)) {
> +		worker = kmalloc_obj(*worker, GFP_KERNEL);
> +		if (!worker) {
> +			dev_err(ucsi_itepd->dev,
> +				"out of memory, skip attention check\n");
> +			ucsi_connector_change(ucsi_itepd->ucsi,
> +					      UCSI_CCI_CONNECTOR(*cci));
> +		} else {
> +			worker->port = UCSI_CCI_CONNECTOR(*cci) - 1;
> +			worker->ucsi_itepd = ucsi_itepd;
> +			worker->cci = *cci;

[Severity: High]
Is the worker->port index sufficiently validated? The port index is derived
directly from the hardware CCI register and can be up to 126. It is then
passed to the workqueue where it indexes bounded arrays like
con_change_processed and dp_en which are sized by ITEPD_MAX_PORTS. Would
a faulty or unexpected hardware response cause out-of-bounds memory
corruption here?

[ ... ]
> +		switch (UCSI_COMMAND(ucsi_itepd->cmd)) {
> +		case UCSI_GET_CONNECTOR_STATUS:
> +			ucsi_itepd->connected[ucsi_itepd->cmd_port] =
> +				!!(msg_in[2] & BIT(3));
> +			ucsi_itepd->orientation[ucsi_itepd->cmd_port] =
> +				FIELD_GET(BIT(6), msg_in[10]);
> +			break;

[Severity: High]
Could this read past the end of the populated response data? The driver copies
the I2C payload into a local msg_in buffer based on the length provided in
the CCI register. If the hardware responds with a shorter message, reading
msg_in[10] unconditionally evaluates uninitialized stack memory.

[ ... ]
> +static int ucsi_itepd_probe(struct auxiliary_device *adev,
> +			    const struct auxiliary_device_id *id)
> +{
[ ... ]
> +	ret = itepd_register_cb(dev, ITEPD_CLIENT_UCSI, ucsi_itepd_cb);
> +	if (ret)
> +		goto out_destroy_wq;
> +
> +	ucsi_itepd->ucsi = ucsi_create(dev, &ucsi_itepd_ops);

[Severity: High]
Does registering the callback early create a race condition? The interrupt on
the parent I2C device is already active. If an interrupt fires immediately
after itepd_register_cb, it will invoke ucsi_itepd_notify which ultimately
dereferences ucsi_itepd->ucsi. Since ucsi_create has not been called yet,
could this result in a NULL pointer dereference?

[ ... ]
> +static void ucsi_itepd_remove(struct auxiliary_device *adev)
> +{
> +	struct ucsi_itepd *ucsi_itepd = dev_get_drvdata(&adev->dev);
> +
> +	if (ucsi_itepd->ordered_wq) {
> +		flush_workqueue(ucsi_itepd->ordered_wq);
> +		destroy_workqueue(ucsi_itepd->ordered_wq);
> +	}
> +
> +	ucsi_unregister(ucsi_itepd->ucsi);
> +	ucsi_destroy(ucsi_itepd->ucsi);
> +	usleep_range(2000, 2500);
> +	itepd_register_cb(&adev->dev, ITEPD_CLIENT_UCSI, NULL);
> +}

[Severity: High]
Might this teardown sequence result in a use-after-free? The workqueue is
destroyed and the ucsi instance is freed before the callback is unregistered.
If an interrupt occurs concurrently, the handler could invoke the callback,
attempting to queue a task on the destroyed workqueue or dereference the
freed ucsi structure. Should the callback be cleared before resources are
freed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615-ucsi-itepd-feature-v1-0-a826cfd0df6a@ite.com.tw?part=2