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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the probe error path fails to cancel delayed work queues (`wq_poll` and `update_work`) when returning an error, leading to a use-after-free.
- [High] This problem wasn't introduced by this patch, but the suspend and resume functions blindly call disable_irq() and enable_irq_wake() on IRQ 0 if wakeup is enabled but no IRQ is present.
--

--- Patch [2]: [PATCH v4 2/3] usb: typec: tipd: Factor out i2c specifics ---
commit 5612453a5a25a0abc22f1c5e791d782754b002b9
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..a3b946f5aaf0a 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c

[ ... ]

> @@ -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;

[Severity: High]
This isn't a bug introduced by this patch, but if tipd_init() encounters an
error after scheduling the polling delayed work, will this jump to
err_disconnect without cancelling the work?

If a device lacks an IRQ and falls back to polling mode, but
tps->data->connect() fails, the wq_poll delayed work is queued but ret
remains non-zero. The function then jumps to the err_disconnect path, which
lacks a call to cancel_delayed_work_sync(&tps->wq_poll) or
cancel_delayed_work_sync(&cd321x->update_work).

Can this lead to a potential use-after-free when the work executes later on
freed memory?

[ ... ]

> @@ -1910,29 +1782,26 @@ static void tps6598x_remove(struct i2c_client *client)
>  	if (tps->reset)
>  		gpiod_set_value_cansleep(tps->reset, 1);
>  }
> +EXPORT_SYMBOL_GPL(tipd_remove);
>  
> -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 is a pre-existing issue, but does this check safely handle misconfigured
devices?

If a device is configured via Device Tree with the wakeup-source property but
lacks an interrupts property, tps->irq will be 0, but tps->wakeup will
evaluate to true. 

During tipd_suspend(), the code checks tps->wakeup without validating
tps->irq, blindly invoking disable_irq(0) and enable_irq_wake(0). Could
disabling IRQ 0 inadvertently disable essential system interrupts like the
system timer, leading to system freezes or panics during suspend?

>  	} else if (tps->reset) {
>  		gpiod_set_value_cansleep(tps->reset, 1);
>  	}
>  
> -	if (!client->irq)
> +	if (!tps->irq)
>  		cancel_delayed_work_sync(&tps->wq_poll);
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL_GPL(tipd_suspend);

[ ... ]

> @@ -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: High]
This is a pre-existing issue, but similar to tipd_suspend() above, could this
blindly invoke disable_irq_wake(0) and enable_irq(0) if tps->irq is missing
but wakeup is enabled?

>  	} else if (tps->reset) {
>  		gpiod_set_value_cansleep(tps->reset, 0);
>  		msleep(TPS_SETUP_MS);
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806-tipd-ace3-v4-0-79855b150914@chaosmail.tech?part=2