Re: [PATCH 2/4] usb: typec: ucpd: Add ST UCPD driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [Critical] Stack buffer overflow in `ucpd_pd_read_message` due to unbounded reading of incoming PD messages.
- [High] Uninitialized variable `ret` is returned from `tcpm_set_vconn` on early bail-out paths.
- [High] Resource leaks for `fwnode_handle` and `typec_switch` in `ucpd_probe` and `ucpd_remove`.
- [High] Invalid use of `IS_ERR()` to check the return value of `device_get_named_child_node()`.
- [High] The threaded IRQ handler is requested before the TCPM port is fully registered, causing a NULL pointer dereference on early interrupts.
- [High] The IRQ handler remains active after the TCPM port's workqueue is destroyed, leading to a Use-After-Free during teardown.
- [High] The function `tcpm_decode_cc` maps CC voltage states incorrectly when operating as a Sink.
- [High] Manual software generation of a GoodCRC for invalid or truncated PD messages inside a threaded IRQ violates USB PD requirements.
- [High] Ignoring the timeout when waiting for the previous transmission to complete can corrupt the hardware state.
- [Medium] Data race on port roles (`pwr_role` and `data_role`) as they are updated without a lock but read under a lock.
--

--- Patch [2]: [PATCH 2/4] usb: typec: ucpd: Add ST UCPD driver ---
commit 2b9c556a7989ba29cde859ea3a479a2ad6876950
Author: Marek Vasut <marex@nabladev.com>

usb: typec: ucpd: Add ST UCPD driver

Add ST UCPD Type-C block driver. The UCPD integrates the physical
layer of the Power Delivery (PD), with CC signaling method, for
operation with Type-C cables.

> diff --git a/drivers/usb/typec/tcpm/ucpd.c b/drivers/usb/typec/tcpm/ucpd.c
> new file mode 100644
> index 0000000000000..fa7c0ffbfd577
> --- /dev/null
> +++ b/drivers/usb/typec/tcpm/ucpd.c

[ ... ]

> +static enum typec_cc_status tcpm_decode_cc(const int cr, const int cc)
> +{
> +	bool sink = !!(cr & UCPD_CR_ANAMODE);
> +
> +	switch (cc) {
> +	case UCPD_SR_TYPEC_VSTATE_CC_LOWEST:
> +		return TYPEC_CC_RA;
> +	case UCPD_SR_TYPEC_VSTATE_CC_LOW:
> +		return TYPEC_CC_RD;

[Severity: High]
When operating as a Sink, the CC line is pulled up by the partner's Rp.
A LOWEST state (0V) means unattached (TYPEC_CC_OPEN), and a LOW state
corresponds to TYPEC_CC_RP_DEF.

Does returning TYPEC_CC_RA and TYPEC_CC_RD unconditionally here cause the
TCPM state machine to fail to detect chargers correctly?

> +	case UCPD_SR_TYPEC_VSTATE_CC_HIGH:
> +		return sink ? TYPEC_CC_RP_1_5 : TYPEC_CC_OPEN;
> +	default:
> +		return sink ? TYPEC_CC_RP_3_0 : TYPEC_CC_OPEN;
> +	}
> +}

[ ... ]

> +static int tcpm_set_vconn(struct tcpc_dev *dev, bool on)
> +{
> +	struct ucpd *ucpd = tcpm_to_ucpd(dev);
> +	DECLARE_BITMAP(bitmap, 2) = { };
> +	enum typec_cc_status cc1, cc2;
> +	int cr, sr;
> +	int ret;

[Severity: High]
Is it possible for this code to return an uninitialized ret?

If we take the early goto done path below due to ucpd->vconn_on == on
or an unknown CC polarity, ret will be returned without ever being
initialized.

> +
> +	mutex_lock(&ucpd->lock);
> +	if (ucpd->vconn_on == on) {
> +		dev_err(ucpd->dev, "vconn is already %d\n", on);
> +		goto done;
> +	}

[ ... ]

> +	if (cc1 == TYPEC_CC_OPEN || cc1 == TYPEC_CC_RA) {
> +		ucpd->cc_polarity = TYPEC_POLARITY_CC2;
> +	} else if (cc2 == TYPEC_CC_OPEN || cc2 == TYPEC_CC_RA) {
> +		ucpd->cc_polarity = TYPEC_POLARITY_CC1;
> +	} else {
> +		dev_err(ucpd->dev, "unknown CC polarity %d %d\n", cc1, cc2);
> +		goto done;
> +	}

[ ... ]

> +done:
> +	mutex_unlock(&ucpd->lock);
> +
> +	return ret;
> +}

[ ... ]

> +static int tcpm_set_roles(struct tcpc_dev *dev, bool attached,
> +			  enum typec_role pwr, enum typec_data_role data)
> +{
> +	struct ucpd *ucpd = tcpm_to_ucpd(dev);
> +
> +	ucpd->pwr_role = pwr;
> +	ucpd->data_role = data;

[Severity: Medium]
Should this state update be protected by ucpd->lock?

These roles are read concurrently under the lock in ucpd_pd_read_message(),
which could lead to a race condition and a GoodCRC message being transmitted
with torn role bits.

> +
> +	return 0;
> +}

[ ... ]

> +static int ucpd_pd_send_message(struct ucpd *ucpd, enum tcpm_transmit_type type,
> +				const struct pd_message *msg)
> +{

[ ... ]

> +	if (ucpd->tx_pending) {
> +		regmap_read_poll_timeout(ucpd->regmap, UCPD_SR,
> +					       val, val & UCPD_SR_TXMSGSENT,
> +					       0, 100000);

[Severity: High]
Could ignoring the return value of regmap_read_poll_timeout() lead to
hardware state corruption?

If the hardware is stuck or still busy transmitting and the timeout occurs,
the code proceeds to unconditionally overwrite TX registers, potentially
corrupting the ongoing transfer.

> +		regmap_write(ucpd->regmap, UCPD_ICR, UCPD_ICR_TXMSGSENTCF);
> +		ucpd->tx_pending = false;
> +	}

[ ... ]

> +static void ucpd_pd_read_message(struct ucpd *ucpd)
> +{

[ ... ]

> +		if (val & UCPD_SR_RXNE) {
> +			regmap_read(ucpd->regmap, UCPD_RXDR, &rxdata);
> +
> +			/* First two bytes are header, rest is message */
> +			if (count < 2)
> +				mh[count] = rxdata & 0xff;
> +			else
> +				mp[count - 2] = rxdata & 0xff;
> +			count++;
> +		}

[Severity: Critical]
Can this unbounded loop overflow the pd_msg stack buffer?

An external device sending an oversized or malformed packet could cause
count to grow past the 30-byte payload limit before the length check is
ever reached, overwriting the kernel stack.

> +
> +		if (val & UCPD_SR_RXMSGEND) {
> +			regmap_write(ucpd->regmap, UCPD_ICR, UCPD_ICR_RXMSGENDCF);
> +			break;
> +		}
> +	}

[ ... ]

> +	if (!len && (pd_header_type_le(pd_msg.header) == PD_CTRL_GOOD_CRC)) {
> +		tcpm_pd_transmit_complete(ucpd->tcpm_port, TCPC_TX_SUCCESS);
> +	} else {
> +		struct pd_message msg = {
> +			.header = PD_HEADER_LE(PD_CTRL_GOOD_CRC,
> +					       ucpd->pwr_role,
> +					       ucpd->data_role,
> +					       pd_header_rev_le(pd_msg.header),
> +					       pd_header_msgid_le(pd_msg.header),
> +					       0),
> +		};
> +		ucpd_pd_send_message(ucpd, TCPC_TX_SOP, &msg);

[Severity: High]
Does generating a GoodCRC in software inside a threaded IRQ violate the
195us tTransmit USB PD requirement?

It seems like this might lead to connection drops. Also, for truncated
messages, it constructs a GoodCRC with a zeroed header acknowledging
corrupted data.

> +		tcpm_pd_receive(ucpd->tcpm_port, &pd_msg, TCPC_TX_SOP);
> +	}
> +}

[ ... ]

> +static int ucpd_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	ucpd->tcpc_dev.fwnode = device_get_named_child_node(dev, "connector");
> +	if (IS_ERR(ucpd->tcpc_dev.fwnode))
> +		return PTR_ERR(ucpd->tcpc_dev.fwnode);

[Severity: High]
Should this check be if (!ucpd->tcpc_dev.fwnode) instead?

device_get_named_child_node() returns NULL on failure, not an error pointer,
which would cause this check to falsely pass and proceed with a NULL fwnode.

> +
> +	platform_set_drvdata(pdev, ucpd);

[ ... ]

> +	ucpd->orient_sw = fwnode_typec_switch_get(ucpd->tcpc_dev.fwnode);
> +	if (IS_ERR(ucpd->orient_sw)) {
> +		return dev_err_probe(dev, PTR_ERR(ucpd->orient_sw),
> +				     "Failed to get orientation switch\n");
> +	}

[Severity: High]
Does this early return bypass the tcpm_put_fwnode error path, leaking the
fwnode handle?

Additionally, it looks like typec_switch_put() is missing in both the
probe error path and in driver removal.

> +
> +	ret = devm_request_threaded_irq(dev, irq, NULL, ucpd_irq,
> +					IRQF_ONESHOT, "ucpd", ucpd);
> +	if (ret < 0)
> +		goto tcpm_put_fwnode;
> +
> +	ucpd->tcpm_port = tcpm_register_port(dev, &ucpd->tcpc_dev);

[Severity: High]
Could an interrupt fire right after it is requested but before
ucpd->tcpm_port is fully registered?

tcpm_init() is called synchronously during port registration and unmasks
hardware interrupts, which might cause ucpd_irq() to dereference a
NULL ucpd->tcpm_port.

> +	if (IS_ERR(ucpd->tcpm_port)) {
> +		ret = PTR_ERR(ucpd->tcpm_port);
> +		goto tcpm_put_fwnode;
> +	}
> +
> +	return ret;
> +
> +tcpm_put_fwnode:
> +	fwnode_handle_put(ucpd->tcpc_dev.fwnode);
> +
> +	return ret;
> +}
> +
> +static void ucpd_remove(struct platform_device *pdev)
> +{
> +	struct ucpd *ucpd = platform_get_drvdata(pdev);
> +
> +	tcpm_unregister_port(ucpd->tcpm_port);

[Severity: High]
Could this trigger a use-after-free if an interrupt fires during removal?

tcpm_unregister_port() destroys the internal workqueue synchronously, but
hardware interrupts are not masked and the devm-managed IRQ is still active
until after remove completes. If an interrupt fires during this window,
ucpd_irq() will queue work on the destroyed TCPM workqueue.

> +	fwnode_handle_put(ucpd->tcpc_dev.fwnode);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260819163410.119755-1-marex@nabladev.com?part=2