Re: [PATCH v3 2/3] usb: typec: tipd: Factor out i2c specifics

[sashiko-bot@kernel.org]

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

New issues:
- [High] The newly extracted `core.c` retains I2C-specific casts in `tps25750_write_firmware()`, risking memory corruption for non-I2C buses.

Pre-existing issues:
- [Critical] tipd_suspend() and tipd_resume() unconditionally pass an invalid IRQ 0 to disable_irq() and enable_irq() if configured as a wakeup source but lacking an interrupt.
- [High] tipd_resume() attempts to communicate with the device over I2C before de-asserting its reset GPIO, causing a guaranteed communication failure.
- [High] Delayed works (`wq_poll` and `cd321x->update_work`) are queued but not canceled on probe error paths, leading to a Use-After-Free.
- [High] Early returns in tipd_resume() skip enable_irq(), permanently disabling a shared interrupt line.
--

--- Patch [2]: [PATCH v3 2/3] usb: typec: tipd: Factor out i2c specifics ---
commit 8d801170f1c1aaccc36ed5f0477a7ac708265867
Author: Alyssa Milburn <amilburn@zall.org>

usb: typec: tipd: Factor out i2c specifics

Make the core driver more bus-agnostic to prepare for SPMI variants of
the tipd chip.

> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index d5ee0af9058b9..31098bdce1096 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c

[Severity: High]
Since this commit prepares core.c for non-I2C buses, will the hardcoded
I2C casts remaining in tps25750_write_firmware() cause memory corruption
if an SPMI variant reaches this path?

For example:

drivers/usb/typec/tipd/core.c:tps25750_write_firmware() {
    struct i2c_client *client = to_i2c_client(tps->dev);
    ...
    client->adapter->timeout = msecs_to_jiffies(5000);
    client->addr = bpms_addr;
}

Could this overwrite memory at the client->adapter->timeout and
client->addr offsets for non-I2C devices?

[ ... ]

> @@ -1842,14 +1716,14 @@ static int tps6598x_probe(struct i2c_client *client)
>  			goto err_unregister_port;
>  		ret = tps->data->connect(tps, status);
>  		if (ret)
> -			dev_err(&client->dev, "failed to register partner\n");
> +			dev_err(tps->dev, "failed to register partner\n");
>  	}
>  
> -	if (client->irq) {
> -		ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> +	if (tps->irq) {
> +		ret = devm_request_threaded_irq(tps->dev, tps->irq, NULL,
>  						tps->data->irq_handler,
>  						IRQF_SHARED | IRQF_ONESHOT,
> -						dev_name(&client->dev), tps);
> +						dev_name(tps->dev), tps);
>  	} else {
>  		dev_warn(tps->dev, "Unable to find the interrupt, switching to polling\n");
>  		INIT_DELAYED_WORK(&tps->wq_poll, tps6598x_poll_work);
> @@ -1860,13 +1734,12 @@ static int tps6598x_probe(struct i2c_client *client)
>  	if (ret)
>  		goto err_disconnect;
>  
> -	i2c_set_clientdata(client, tps);
>  	fwnode_handle_put(fwnode);
>  
>  	tps->wakeup = device_property_read_bool(tps->dev, "wakeup-source");
> -	if (tps->wakeup && client->irq) {
> -		devm_device_init_wakeup(&client->dev);
> -		enable_irq_wake(client->irq);
> +	if (tps->wakeup && tps->irq) {
> +		devm_device_init_wakeup(tps->dev);
> +		enable_irq_wake(tps->irq);
>  	}
>  
>  	return 0;
> @@ -1887,15 +1760,14 @@ static int tps6598x_probe(struct i2c_client *client)
>  
>  	return ret;
>  }
> +EXPORT_SYMBOL_GPL(tipd_init);

[Severity: High]
This is a pre-existing issue, but does the error path in tipd_init()
leak the delayed works?

If tps->data->connect() succeeds and queues cd321x->update_work, or if
the polling mode queues tps->wq_poll, and a subsequent step fails, we
take the err_disconnect path.

Does this path exit and drop the device structure without calling
cancel_delayed_work_sync(&tps->wq_poll) or tps->data->remove(tps) to
cancel pending works, potentially causing a use-after-free when the tps
struct is freed?

[ ... ]

> -static int __maybe_unused tps6598x_suspend(struct device *dev)
> +int tipd_suspend(struct tps6598x *tps)
>  {
> -	struct i2c_client *client = to_i2c_client(dev);
> -	struct tps6598x *tps = i2c_get_clientdata(client);
> -
>  	if (tps->wakeup) {
> -		disable_irq(client->irq);
> -		enable_irq_wake(client->irq);
> +		disable_irq(tps->irq);
> +		enable_irq_wake(tps->irq);

[Severity: Critical]
This is a pre-existing issue, but can this pass an invalid IRQ 0 to
disable_irq() and enable_irq_wake()?

If the device specifies a wakeup-source in the device tree but has no
interrupt configured, tps->wakeup will be true while tps->irq is 0.

Could calling disable_irq(0) inadvertently disable critical system
interrupts, such as the primary system timer?

[ ... ]

> -static int __maybe_unused tps6598x_resume(struct device *dev)
> +int tipd_resume(struct tps6598x *tps)
>  {
> -	struct i2c_client *client = to_i2c_client(dev);
> -	struct tps6598x *tps = i2c_get_clientdata(client);
>  	int ret;
>  
>  	ret = tps6598x_check_mode(tps);

[Severity: High]
This is a pre-existing issue, but does tipd_resume() attempt to
communicate with the device before it is taken out of reset?

When the device lacks a wakeup source but has a reset GPIO,
tipd_suspend() asserts the reset GPIO.

During tipd_resume(), we call tps6598x_check_mode() which tries to read
from the device. However, the reset GPIO is not de-asserted until later:

drivers/usb/typec/tipd/core.c:tipd_resume() {
    ...
    } else if (tps->reset) {
        gpiod_set_value_cansleep(tps->reset, 0);
        ...
    }
}

Will this lead to a guaranteed communication failure during resume?

[Severity: High]
This is also a pre-existing issue, but do the early returns from
tps6598x_check_mode() and tps->data->init() skip re-enabling the
interrupt?

If tipd_resume() encounters an error here, it returns early and skips:

drivers/usb/typec/tipd/core.c:tipd_resume() {
    ...
    if (tps->wakeup) {
        disable_irq_wake(tps->irq);
        enable_irq(tps->irq);
    }
    ...
}

Could this permanently disable a shared interrupt line?

> @@ -1946,25 +1815,22 @@ static int __maybe_unused tps6598x_resume(struct device *dev)
>  	}
>  
>  	if (tps->wakeup) {
> -		disable_irq_wake(client->irq);
> -		enable_irq(client->irq);
> +		disable_irq_wake(tps->irq);
> +		enable_irq(tps->irq);

[Severity: Critical]
This isn't a bug introduced by this patch, but can this pass an invalid
IRQ 0 to disable_irq_wake() and enable_irq()?

As in tipd_suspend(), if the device specifies a wakeup-source in the
device tree but has no interrupt configured, tps->wakeup will be true
while tps->irq is 0.

Could calling enable_irq(0) corrupt the IRQ state for critical system
interrupts?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260801-tipd-ace3-v3-0-8332f2eca4fc@chaosmail.tech?part=2