Re: [PATCH v2 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] I2C-specific code remains in the bus-agnostic core, causing memory corruption on non-I2C buses.
- [Low] `tps6598x_pm_ops` is missing the `static` modifier.

Pre-existing issues:
- [High] Probe error path fails to cancel queued delayed works, leading to a Use-After-Free.
- [High] Suspend/resume functions blindly operate on IRQ 0 if `wakeup-source` is present but no interrupt is supplied.
- [High] The polling delayed work re-arms itself unconditionally, leading to a Use-After-Free when the driver is unbound.
--

--- Patch [2]: [PATCH v2 2/3] usb: typec: tipd: Factor out i2c specifics ---
commit 882592426a7c1d39d01dd47ae8a3f79d6c0d8efd
Author: Alyssa Milburn <amilburn@zall.org>
Subject: usb: typec: tipd: Factor out i2c specifics

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

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

[Severity: High]
Does the core driver still contain I2C-specific code?

Looking at tps25750_write_firmware() in core.c, it still uses
to_i2c_client(tps->dev):

	struct i2c_client *client = to_i2c_client(tps->dev);
...
	slave_addr = client->addr;
	timeout = client->adapter->timeout;

	client->adapter->timeout = msecs_to_jiffies(5000);
	client->addr = bpms_addr;

If this core code is used with SPMI variants, won't to_i2c_client() perform an
invalid container_of() cast on the device pointer, resulting in memory
corruption when modifying the non-existent i2c_client struct?

> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index d5ee0af9058b9..aadadfdc6ed12 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
[ ... ]
> @@ -1842,14 +1717,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 +1735,12 @@ static int tps6598x_probe(struct i2c_client *client)
>  	if (ret)
>  		goto err_disconnect;
>  
> -	i2c_set_clientdata(client, tps);
>  	fwnode_handle_put(fwnode);

[Severity: High]
This is a pre-existing issue, but if we take the goto err_disconnect path
here, are we leaking queued works?

If tps->data->connect() fails earlier, tps->wq_poll is already queued.
If cd321x_connect() queued cd321x->update_work, that is also queued.

When jumping to err_disconnect, the delayed works are never canceled before
the function returns and the devres-allocated tps object is freed. Could this
cause a use-after-free when the works later execute on freed memory?

[ ... ]
> @@ -1887,15 +1761,14 @@ static int tps6598x_probe(struct i2c_client *client)
>  
>  	return ret;
>  }
> +EXPORT_SYMBOL_GPL(tipd_init);
>  
> -static void tps6598x_remove(struct i2c_client *client)
> +void tipd_remove(struct tps6598x *tps)
>  {
> -	struct tps6598x *tps = i2c_get_clientdata(client);
> -
> -	if (!client->irq)
> +	if (!tps->irq)
>  		cancel_delayed_work_sync(&tps->wq_poll);
>  	else
> -		devm_free_irq(tps->dev, client->irq, tps);
> +		devm_free_irq(tps->dev, tps->irq, tps);

[Severity: High]
This is a pre-existing issue, but doesn't tps6598x_poll_work() unconditionally
re-arm itself?

Looking at tps6598x_poll_work():

	queue_delayed_work(system_power_efficient_wq, &tps->wq_poll,
			   msecs_to_jiffies(POLL_INTERVAL));

Because the re-arming is unconditional, won't cancel_delayed_work_sync() here
allow the work to just queue itself again immediately, resulting in a
use-after-free after the device is unbound?

[ ... ]
> -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: High]
This isn't a bug introduced by this patch, but if the device tree specifies
a wakeup-source but no interrupt is provided, tps->irq is 0 and tps->wakeup
is true.

Won't this call disable_irq(0) and enable_irq_wake(0), mistakenly manipulating
the wrong interrupt (such as the 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;
[ ... ]
>  	if (tps->wakeup) {
> -		disable_irq_wake(client->irq);
> -		enable_irq(client->irq);
> +		disable_irq_wake(tps->irq);
> +		enable_irq(tps->irq);

[Severity: High]
This is a pre-existing issue, but similar to suspend, if tps->irq is 0, won't
this resume path call enable_irq(0) and disable_irq_wake(0)?

> diff --git a/drivers/usb/typec/tipd/i2c.c b/drivers/usb/typec/tipd/i2c.c
> new file mode 100644
> index 0000000000000..1ea6ee59c8226
> --- /dev/null
> +++ b/drivers/usb/typec/tipd/i2c.c
[ ... ]
> +static int __maybe_unused tps6598x_resume(struct device *dev)
> +{
> +	struct tps6598x *tps = dev_get_drvdata(dev);
> +
> +	return tipd_resume(tps);
> +}
> +
> +const struct dev_pm_ops tps6598x_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(tps6598x_suspend, tps6598x_resume)
> +};

[Severity: Low]
Should this dev_pm_ops struct be declared static to avoid namespace pollution
and missing-prototype warnings?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727-tipd-ace3-v2-0-1345957bd635@chaosmail.tech?part=2