Re: [PATCH 2/2] PCI/pwrctrl: tc9563: use devm-managed I2C dummy device allocation

[Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>]

[-- Attachment #1: Type: text/plain, Size: 3035 bytes --]

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Use-After-Free due to teardown order inversion between i2c_put_adapter() and devm-managed dummy client release.
> --
> 
> --- Patch [2]: [PATCH 2/2] PCI/pwrctrl: tc9563: use devm-managed I2C dummy device allocation ---
> commit f54ecfd3f2485cf33e559cd8f58fa24de337138b
> Author: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
> 
> PCI/pwrctrl: tc9563: use devm-managed I2C dummy device allocation
> 
> This patch converts the driver to use devm_i2c_new_dummy_device to manage
> the I2C dummy client. It removes the manual i2c_unregister_device calls
> from the probe error path and the remove function.
> 
> > diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> > index 8bccf3abd1892..5b05e76f6c80d 100644
> > --- a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> > +++ b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> [ ... ]
> > @@ -607,7 +606,6 @@ static int tc9563_pwrctrl_probe(struct platform_device *pdev)
> >  power_off:
> >  	tc9563_pwrctrl_power_off(&tc9563->pwrctrl);
> >  remove_i2c:
> > -	i2c_unregister_device(tc9563->client);
> >  	i2c_put_adapter(tc9563->adapter);
> >  	return ret;
> >  }
> 
> [Severity: High]
> Will this cause a use-after-free by changing the teardown order?
> 
> By switching to devm_i2c_new_dummy_device, the dummy client is now
> unregistered by devres cleanup, which executes after tc9563_pwrctrl_probe
> returns. 
> 
> Because i2c_put_adapter is still called manually before returning, the
> adapter reference is dropped before devres unregisters the dummy client. If
> dropping this reference allows the adapter to be freed concurrently, it would
> forcibly unregister all children. When devres later attempts to unregister
> the client, it would operate on an already-freed pointer.
> 
> Could devm_add_action_or_reset be used to manage the i2c_put_adapter call
> so the teardown sequence happens in the correct reverse order?

IIUC i2c_put_adapter() does not unregister any i2c children device but just
decrements the adapter reference counting. I guess even if
devm_i2c_release_dummy() runs after i2c_put_adapter(), it will be safe. Agree?

> 
> [ ... ]
> > @@ -619,7 +617,6 @@ static void tc9563_pwrctrl_remove(struct platform_device *pdev)
> >  					struct tc9563_pwrctrl, pwrctrl);
> >  
> >  	tc9563_pwrctrl_power_off(&tc9563->pwrctrl);
> > -	i2c_unregister_device(tc9563->client);
> >  	i2c_put_adapter(tc9563->adapter);
> >  }
> 
> [Severity: High]
> Does this also expose driver unbind to the same teardown order inversion?
> 
> Since tc9563_pwrctrl_remove executes before devres cleanup, dropping the
> adapter reference here deterministically happens before the dummy client is
> unregistered.

ditto.

Regards,
Lorenzo

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260729-pci-pwrctrl-tc956x-regmap-v1-0-20ce61a3ce12@oss.qualcomm.com?part=2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]