Re: [PATCH v3 2/3] usb: typec: tipd: add read_power_status callback to tipd_data

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Missing cancellation of queued delayed works in the probe error paths leads to Use-After-Free of devres-managed memory.
- [High] In `tps6598x_probe()`, a failure in `tps->data->connect()` causes probe to fail in polling mode but silently succeed in IRQ mode.
- [High] Multi-byte raw I2C data is read into native integers (e.g., u32, u64) without endianness conversions, breaking bitwise operations on big-endian architectures.
--

--- Patch [2]: [PATCH v3 2/3] usb: typec: tipd: add read_power_status callback to tipd_data ---
commit 5bbdcd3daa173c06fcfc36de1b9d3a33bfefea78
Author: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
Subject: usb: typec: tipd: add read_power_status callback to tipd_data

This commit converts direct tps6598x_read_power_status() calls to use an
indirect callback through tipd_data. This allows variants to provide their
own power status reading logic while keeping existing behavior unchanged.

> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index b6335b36d3846..9097b0d40a71d 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c

[ ... ]

> @@ -1026,7 +1027,7 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
>  		goto err_unlock;
>  
>  	if ((event1[0] | event2[0]) & TPS_REG_INT_POWER_STATUS_UPDATE)
> -		if (!tps6598x_read_power_status(tps))
> +		if (!tps->data->read_power_status(tps))
>  			goto err_unlock;
>  
>  	if ((event1[0] | event2[0]) & TPS_REG_INT_DATA_STATUS_UPDATE)

[Severity: High]
This is a pre-existing issue, but does evaluating the unswapped 64-bit I2C
data cause bitwise operations to check the wrong bits on big-endian CPUs?

When multi-byte registers are read in tps6598x_block_read(), raw I2C bytes
are copied directly into native integers using memcpy(). Because bitmasks
like TPS_REG_INT_POWER_STATUS_UPDATE are applied to event1[0] as a native
integer, the driver may evaluate the wrong bytes and malfunction on
big-endian architectures.

[ ... ]

> @@ -1839,7 +1840,7 @@ static int tps6598x_probe(struct i2c_client *client)
>  
>  	if (status & TPS_STATUS_PLUG_PRESENT) {
>  		ret = -EINVAL;
> -		if (!tps6598x_read_power_status(tps))
> +		if (!tps->data->read_power_status(tps))
>  			goto err_unregister_port;
>  		if (!tps->data->read_data_status(tps))
>  			goto err_unregister_port;

[Severity: High]
This is a pre-existing issue, but does probe silently succeed if partner
connection fails in IRQ mode?

Slightly later in tps6598x_probe():

    ret = tps->data->connect(tps, status);
    if (ret)
        dev_err(&client->dev, "failed to register partner\n");
    }

    if (client->irq) {
        ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, ...

If tps->data->connect() returns an error, the ret value is unconditionally
overwritten when client->irq is present. This completely masks the
connection error, leaving the device registered in a degraded state without
a partner. If polling mode is used instead, the same error correctly causes
probe to abort entirely.

[Severity: Critical]
This is another pre-existing issue in tps6598x_probe(), but can the probe
error handling path cause a Use-After-Free?

If probe jumps to err_disconnect after delayed works have been queued
(e.g., tps->wq_poll or cd321x->update_work), the error path executes:

err_disconnect:
    tps6598x_disconnect(tps, 0);
err_unregister_port:
    tps->data->unregister_port(tps);
...

This cleanup completely omits canceling the pending works. The devres
cleanup will subsequently free the tps structure, but the delayed works
remain active in the system workqueue. When they later execute, they will
access the freed memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714061820.537792-1-radhey.shyam.pandey@amd.com?part=2