PCI/pwrctrl: Cancel outstanding rescan work when unregistering

[PATCH] PCI/pwrctrl: Cancel outstanding rescan work when unregistering

Posted by Brian Norris 10 months ago

From: Brian Norris <briannorris@google.com>

It's possible to trigger use-after-free here by:
(a) forcing rescan_work_func() to take a long time and
(b) utilizing a pwrctrl driver that may be unloaded for some reason.

I'm unlucky to trigger both of these in development. It's likely much
more difficult to hit this in practice.

Anyway, we should ensure our work is finished before we allow our data
structures to be cleaned up.

Fixes: 8f62819aaace ("PCI/pwrctl: Rescan bus on a separate thread")
Cc: Konrad Dybcio <konradybcio@kernel.org>
Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Signed-off-by: Brian Norris <briannorris@google.com>
Signed-off-by: Brian Norris <briannorris@chromium.org>
---

 drivers/pci/pwrctrl/core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/pwrctrl/core.c b/drivers/pci/pwrctrl/core.c
index 9cc7e2b7f2b5..6bdbfed584d6 100644
--- a/drivers/pci/pwrctrl/core.c
+++ b/drivers/pci/pwrctrl/core.c
@@ -101,6 +101,8 @@ EXPORT_SYMBOL_GPL(pci_pwrctrl_device_set_ready);
  */
 void pci_pwrctrl_device_unset_ready(struct pci_pwrctrl *pwrctrl)
 {
+	cancel_work_sync(&pwrctrl->work);
+
 	/*
 	 * We don't have to delete the link here. Typically, this function
 	 * is only called when the power control device is being detached. If
-- 
2.49.0.604.gff1f9ca942-goog

Re: [PATCH] PCI/pwrctrl: Cancel outstanding rescan work when unregistering

Posted by Bjorn Helgaas 9 months, 3 weeks ago

On Wed, Apr 09, 2025 at 11:53:13AM -0700, Brian Norris wrote:
> From: Brian Norris <briannorris@google.com>
> 
> It's possible to trigger use-after-free here by:
> (a) forcing rescan_work_func() to take a long time and
> (b) utilizing a pwrctrl driver that may be unloaded for some reason.
> 
> I'm unlucky to trigger both of these in development. It's likely much
> more difficult to hit this in practice.
> 
> Anyway, we should ensure our work is finished before we allow our data
> structures to be cleaned up.
> 
> Fixes: 8f62819aaace ("PCI/pwrctl: Rescan bus on a separate thread")
> Cc: Konrad Dybcio <konradybcio@kernel.org>
> Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> Signed-off-by: Brian Norris <briannorris@google.com>
> Signed-off-by: Brian Norris <briannorris@chromium.org>

Applied to pci/pwrctrl for v6.16, thanks!

> ---
> 
>  drivers/pci/pwrctrl/core.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/pci/pwrctrl/core.c b/drivers/pci/pwrctrl/core.c
> index 9cc7e2b7f2b5..6bdbfed584d6 100644
> --- a/drivers/pci/pwrctrl/core.c
> +++ b/drivers/pci/pwrctrl/core.c
> @@ -101,6 +101,8 @@ EXPORT_SYMBOL_GPL(pci_pwrctrl_device_set_ready);
>   */
>  void pci_pwrctrl_device_unset_ready(struct pci_pwrctrl *pwrctrl)
>  {
> +	cancel_work_sync(&pwrctrl->work);
> +
>  	/*
>  	 * We don't have to delete the link here. Typically, this function
>  	 * is only called when the power control device is being detached. If
> -- 
> 2.49.0.604.gff1f9ca942-goog
>

Re: [PATCH] PCI/pwrctrl: Cancel outstanding rescan work when unregistering

Posted by Bartosz Golaszewski 9 months, 3 weeks ago

On Wed, Apr 9, 2025 at 8:54 PM Brian Norris <briannorris@chromium.org> wrote:
>
> From: Brian Norris <briannorris@google.com>
>
> It's possible to trigger use-after-free here by:
> (a) forcing rescan_work_func() to take a long time and
> (b) utilizing a pwrctrl driver that may be unloaded for some reason.
>
> I'm unlucky to trigger both of these in development. It's likely much
> more difficult to hit this in practice.
>
> Anyway, we should ensure our work is finished before we allow our data
> structures to be cleaned up.
>
> Fixes: 8f62819aaace ("PCI/pwrctl: Rescan bus on a separate thread")
> Cc: Konrad Dybcio <konradybcio@kernel.org>
> Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> Signed-off-by: Brian Norris <briannorris@google.com>
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
>

Sorry, it fell off my radar.

Acked-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Re: [PATCH] PCI/pwrctrl: Cancel outstanding rescan work when unregistering

Posted by Manivannan Sadhasivam 9 months, 3 weeks ago

On Wed, Apr 09, 2025 at 11:53:13AM -0700, Brian Norris wrote:
> From: Brian Norris <briannorris@google.com>
> 
> It's possible to trigger use-after-free here by:
> (a) forcing rescan_work_func() to take a long time and
> (b) utilizing a pwrctrl driver that may be unloaded for some reason.
> 
> I'm unlucky to trigger both of these in development. It's likely much
> more difficult to hit this in practice.
> 

I never envisioned this situation :) But yeah, better be safe than sorry.

> Anyway, we should ensure our work is finished before we allow our data
> structures to be cleaned up.
> 
> Fixes: 8f62819aaace ("PCI/pwrctl: Rescan bus on a separate thread")
> Cc: Konrad Dybcio <konradybcio@kernel.org>
> Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> Signed-off-by: Brian Norris <briannorris@google.com>
> Signed-off-by: Brian Norris <briannorris@chromium.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
> 
>  drivers/pci/pwrctrl/core.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/pci/pwrctrl/core.c b/drivers/pci/pwrctrl/core.c
> index 9cc7e2b7f2b5..6bdbfed584d6 100644
> --- a/drivers/pci/pwrctrl/core.c
> +++ b/drivers/pci/pwrctrl/core.c
> @@ -101,6 +101,8 @@ EXPORT_SYMBOL_GPL(pci_pwrctrl_device_set_ready);
>   */
>  void pci_pwrctrl_device_unset_ready(struct pci_pwrctrl *pwrctrl)
>  {
> +	cancel_work_sync(&pwrctrl->work);
> +
>  	/*
>  	 * We don't have to delete the link here. Typically, this function
>  	 * is only called when the power control device is being detached. If
> -- 
> 2.49.0.604.gff1f9ca942-goog
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] PCI/pwrctrl: Cancel outstanding rescan work when unregistering

Posted by Bjorn Helgaas 9 months, 3 weeks ago

On Wed, Apr 09, 2025 at 11:53:13AM -0700, Brian Norris wrote:
> From: Brian Norris <briannorris@google.com>
> 
> It's possible to trigger use-after-free here by:
> (a) forcing rescan_work_func() to take a long time and
> (b) utilizing a pwrctrl driver that may be unloaded for some reason.
> 
> I'm unlucky to trigger both of these in development. It's likely much
> more difficult to hit this in practice.
> 
> Anyway, we should ensure our work is finished before we allow our data
> structures to be cleaned up.
> 
> Fixes: 8f62819aaace ("PCI/pwrctl: Rescan bus on a separate thread")
> Cc: Konrad Dybcio <konradybcio@kernel.org>
> Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> Signed-off-by: Brian Norris <briannorris@google.com>
> Signed-off-by: Brian Norris <briannorris@chromium.org>

Looking for ack/reviewed-by from Bartosz before doing anything here.

> ---
> 
>  drivers/pci/pwrctrl/core.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/pci/pwrctrl/core.c b/drivers/pci/pwrctrl/core.c
> index 9cc7e2b7f2b5..6bdbfed584d6 100644
> --- a/drivers/pci/pwrctrl/core.c
> +++ b/drivers/pci/pwrctrl/core.c
> @@ -101,6 +101,8 @@ EXPORT_SYMBOL_GPL(pci_pwrctrl_device_set_ready);
>   */
>  void pci_pwrctrl_device_unset_ready(struct pci_pwrctrl *pwrctrl)
>  {
> +	cancel_work_sync(&pwrctrl->work);
> +
>  	/*
>  	 * We don't have to delete the link here. Typically, this function
>  	 * is only called when the power control device is being detached. If
> -- 
> 2.49.0.604.gff1f9ca942-goog
>