[PATCH] PCI: hv: Fix a use-after-free bug in hv_eject_device_work()

[PATCH] PCI: hv: Fix a use-after-free bug in hv_eject_device_work()

[Dexuan Cui]

The commit 05f151a73ec2 itself is correct, but it exposes this
use-after-free bug, which is caught by some memory debug options.

Add the Fixes tag to indicate the dependency.

Fixes: 05f151a73ec2 ("PCI: hv: Fix a memory leak in hv_eject_device_work()")
Signed-off-by: Dexuan Cui <decui@microsoft.com>
Cc: stable@vger.kernel.org
---
Sorry for not spotting the bug when sending 05f151a73ec2. 

Now I have enabled the mm debug options to help catch such mistakes in future.

 drivers/pci/controller/pci-hyperv.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index 808a182830e5..42ace1a690f9 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -1880,6 +1880,7 @@ static void hv_pci_devices_present(struct hv_pcibus_device *hbus,
 static void hv_eject_device_work(struct work_struct *work)
 {
 	struct pci_eject_response *ejct_pkt;
+	struct hv_pcibus_device *hbus;
 	struct hv_pci_dev *hpdev;
 	struct pci_dev *pdev;
 	unsigned long flags;
@@ -1890,6 +1891,7 @@ static void hv_eject_device_work(struct work_struct *work)
 	} ctxt;
 
 	hpdev = container_of(work, struct hv_pci_dev, wrk);
+	hbus = hpdev->hbus;
 
 	WARN_ON(hpdev->state != hv_pcichild_ejecting);
 
@@ -1929,7 +1931,9 @@ static void hv_eject_device_work(struct work_struct *work)
 	/* For the two refs got in new_pcichild_device() */
 	put_pcichild(hpdev);
 	put_pcichild(hpdev);
-	put_hvpcibus(hpdev->hbus);
+	/* hpdev has been freed. Do not use it any more. */
+
+	put_hvpcibus(hbus);
 }
 
 /**
-- 
2.17.1

RE: [PATCH] PCI: hv: Fix a use-after-free bug in hv_eject_device_work()

[Michael Kelley]

From: Dexuan Cui <decui@microsoft.com> Sent: Friday, June 21, 2019 12:02 PM
> 
> The commit 05f151a73ec2 itself is correct, but it exposes this
> use-after-free bug, which is caught by some memory debug options.
> 
> Add the Fixes tag to indicate the dependency.
> 
> Fixes: 05f151a73ec2 ("PCI: hv: Fix a memory leak in hv_eject_device_work()")
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> Cc: stable@vger.kernel.org
> ---
> Sorry for not spotting the bug when sending 05f151a73ec2.
> 
> Now I have enabled the mm debug options to help catch such mistakes in future.
> 
>  drivers/pci/controller/pci-hyperv.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index 808a182830e5..42ace1a690f9 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -1880,6 +1880,7 @@ static void hv_pci_devices_present(struct hv_pcibus_device
> *hbus,
>  static void hv_eject_device_work(struct work_struct *work)
>  {
>  	struct pci_eject_response *ejct_pkt;
> +	struct hv_pcibus_device *hbus;
>  	struct hv_pci_dev *hpdev;
>  	struct pci_dev *pdev;
>  	unsigned long flags;
> @@ -1890,6 +1891,7 @@ static void hv_eject_device_work(struct work_struct *work)
>  	} ctxt;
> 
>  	hpdev = container_of(work, struct hv_pci_dev, wrk);
> +	hbus = hpdev->hbus;

In the lines of code following this new assignment, there are four uses of
hpdev->hbus besides the one at the bottom of the function that causes the
use-after-free error.  With 'hbus' now available as a local variable, it looks
rather strange to have those other places still using hpdev->hbus.  I'm thinking
they should be shortened to just 'hbus' for consistency, even though such
changes aren't directly related to fixing the bug.

Michael

> 
>  	WARN_ON(hpdev->state != hv_pcichild_ejecting);
> 
> @@ -1929,7 +1931,9 @@ static void hv_eject_device_work(struct work_struct *work)
>  	/* For the two refs got in new_pcichild_device() */
>  	put_pcichild(hpdev);
>  	put_pcichild(hpdev);
> -	put_hvpcibus(hpdev->hbus);
> +	/* hpdev has been freed. Do not use it any more. */
> +
> +	put_hvpcibus(hbus);
>  }
> 
>  /**
> --
> 2.17.1

RE: [PATCH] PCI: hv: Fix a use-after-free bug in hv_eject_device_work()

[Dexuan Cui]

> From: Michael Kelley <mikelley@microsoft.com>
> > @@ -1880,6 +1880,7 @@ static void hv_pci_devices_present(struct
> hv_pcibus_device
> > *hbus,
> >  static void hv_eject_device_work(struct work_struct *work)
> >  {
> >  	struct pci_eject_response *ejct_pkt;
> > +	struct hv_pcibus_device *hbus;
> >  	struct hv_pci_dev *hpdev;
> >  	struct pci_dev *pdev;
> >  	unsigned long flags;
> > @@ -1890,6 +1891,7 @@ static void hv_eject_device_work(struct
> work_struct *work)
> >  	} ctxt;
> >
> >  	hpdev = container_of(work, struct hv_pci_dev, wrk);
> > +	hbus = hpdev->hbus;
> 
> In the lines of code following this new assignment, there are four uses of
> hpdev->hbus besides the one at the bottom of the function that causes the
> use-after-free error.  With 'hbus' now available as a local variable, it looks
> rather strange to have those other places still using hpdev->hbus.  I'm
> thinking
> they should be shortened to just 'hbus' for consistency, even though such
> changes aren't directly related to fixing the bug.
> 
> Michael
 
Ok, let me post a v2 for this.

Thanks,
Dexuan

RE: [PATCH] PCI: hv: Fix a use-after-free bug in hv_eject_device_work()

[Dexuan Cui]

> From: Sasha Levin <sashal@kernel.org>
> Sent: Saturday, June 22, 2019 11:14 AM
> To: Sasha Levin <sashal@kernel.org>; Dexuan Cui <decui@microsoft.com>;
> linux-pci@vger.kernel.org
> Cc: Lili Deng (Wicresoft North America Ltd) <v-lide@microsoft.com>;
> stable@vger.kernel.org; stable@vger.kernel.org
> Subject: Re: [PATCH] PCI: hv: Fix a use-after-free bug in hv_eject_device_work()
> 
> Hi,
> 
> [This is an automated email]
> 
> This commit has been processed because it contains a "Fixes:" tag,
> fixing commit: 05f151a73ec2 PCI: hv: Fix a memory leak in
> hv_eject_device_work().
> 
> The bot has tested the following trees: v5.1.12, v4.19.53, v4.14.128, v4.9.182.
> 
> v5.1.12: Build OK!
> v4.19.53: Build OK!
> v4.14.128: Failed to apply! Possible dependencies:
>     8c99e120ffca ("PCI: hv: Remove unused reason for refcount handler")
> 
> v4.9.182: Failed to apply! Possible dependencies:
>     02c3764c776c ("PCI: hv: Temporary own CPU-number-to-vCPU-number
> infra")
>     0de8ce3ee8e3 ("PCI: hv: Allocate physically contiguous hypercall params
> buffer")
>     24196f0c7d4b ("PCI: hv: Convert hv_pci_dev.refs from atomic_t to
> refcount_t")
>     6ab42a66d2cc ("Drivers: hv: vmbus: Move Hypercall invocation code out
> of common code")
>     76d36ab79820 ("hv: switch to cpuhp state machine for synic
> init/cleanup")
>     7dcf90e9e032 ("PCI: hv: Use vPCI protocol version 1.2")
>     8730046c1498 ("Drivers: hv vmbus: Move Hypercall page setup out of
> common code")
>     8c99e120ffca ("PCI: hv: Remove unused reason for refcount handler")
>     b1db7e7e1d70 ("PCI: hv: Add vPCI version protocol negotiation")
>     d058fa7e98ff ("Drivers: hv: vmbus: Move the crash notification function")
> 
> 
> How should we proceed with this patch?
> 
> --
> Thanks,
> Sasha

The patch has not gone into any upstream trees yet. So we can not do anything
at this point. I'll try to backport the patch for the old kernels once it's in.

Thanks,
-- Dexuan