[PATCH 1/2] PCI: remove stray put_device() in pci_register_host_bridge()

[PATCH 1/2] PCI: remove stray put_device() in pci_register_host_bridge()

[Dan Carpenter]

This put_device() was accidentally left over from when we changed the
code from using device_register() to calling device_add().  Delete it.

Fixes: 9885440b16b8 ("PCI: Fix pci_host_bridge struct device release/free handling")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/pci/probe.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 9ce83a1d6e31..819d23ce3565 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -999,10 +999,9 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
 	/* Temporarily move resources off the list */
 	list_splice_init(&bridge->windows, &resources);
 	err = device_add(&bridge->dev);
-	if (err) {
-		put_device(&bridge->dev);
+	if (err)
 		goto free;
-	}
+
 	bus->bridge = get_device(&bridge->dev);
 	device_enable_async_suspend(bus->bridge);
 	pci_set_bus_of_node(bus);
-- 
2.47.2

[PATCH 2/2] PCI: Fix double free in pci_register_host_bridge()

[Dan Carpenter]

Calling put_device(&bus->dev) will call release_pcibus_dev() which will
free the bus.  It leads to a use after free when we dereference "bus" in
the cleanup code and the kfree(bus) is a double free.

Fixes: b80b4d4972e6 ("PCI: Fix reference leak in pci_register_host_bridge()")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/pci/probe.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 819d23ce3565..c13f2c957002 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -957,6 +957,7 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
 	resource_size_t offset, next_offset;
 	LIST_HEAD(resources);
 	struct resource *res, *next_res;
+	bool bus_registered = false;
 	char addr[64], *fmt;
 	const char *name;
 	int err;
@@ -1020,10 +1021,9 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
 	name = dev_name(&bus->dev);
 
 	err = device_register(&bus->dev);
-	if (err) {
-		put_device(&bus->dev);
+	bus_registered = true;
+	if (err)
 		goto unregister;
-	}
 
 	pcibios_add_bus(bus);
 
@@ -1110,12 +1110,15 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
 unregister:
 	put_device(&bridge->dev);
 	device_del(&bridge->dev);
-
 free:
 #ifdef CONFIG_PCI_DOMAINS_GENERIC
 	pci_bus_release_domain_nr(parent, bus->domain_nr);
 #endif
-	kfree(bus);
+	if (bus_registered)
+		put_device(&bus->dev);
+	else
+		kfree(bus);
+
 	return err;
 }
 
-- 
2.47.2

Re: [PATCH 1/2] PCI: remove stray put_device() in pci_register_host_bridge()

[Bjorn Helgaas]

On Fri, Mar 07, 2025 at 11:46:34AM +0300, Dan Carpenter wrote:
> This put_device() was accidentally left over from when we changed the
> code from using device_register() to calling device_add().  Delete it.
> 
> Fixes: 9885440b16b8 ("PCI: Fix pci_host_bridge struct device release/free handling")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>

Applied to pci/enumeration for v6.15, thanks!

> ---
>  drivers/pci/probe.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 9ce83a1d6e31..819d23ce3565 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -999,10 +999,9 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
>  	/* Temporarily move resources off the list */
>  	list_splice_init(&bridge->windows, &resources);
>  	err = device_add(&bridge->dev);
> -	if (err) {
> -		put_device(&bridge->dev);
> +	if (err)
>  		goto free;
> -	}
> +
>  	bus->bridge = get_device(&bridge->dev);
>  	device_enable_async_suspend(bus->bridge);
>  	pci_set_bus_of_node(bus);
> -- 
> 2.47.2
> 

Re: [PATCH 2/2] PCI: Fix double free in pci_register_host_bridge()

[Bjorn Helgaas]

On Fri, Mar 07, 2025 at 11:46:41AM +0300, Dan Carpenter wrote:
> Calling put_device(&bus->dev) will call release_pcibus_dev() which will
> free the bus.  It leads to a use after free when we dereference "bus" in
> the cleanup code and the kfree(bus) is a double free.
> 
> Fixes: b80b4d4972e6 ("PCI: Fix reference leak in pci_register_host_bridge()")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>

Squashed into the b80b4d4972e6 commit on pci/enumeration for v6.15,
thanks!

> ---
>  drivers/pci/probe.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 819d23ce3565..c13f2c957002 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -957,6 +957,7 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
>  	resource_size_t offset, next_offset;
>  	LIST_HEAD(resources);
>  	struct resource *res, *next_res;
> +	bool bus_registered = false;
>  	char addr[64], *fmt;
>  	const char *name;
>  	int err;
> @@ -1020,10 +1021,9 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
>  	name = dev_name(&bus->dev);
>  
>  	err = device_register(&bus->dev);
> -	if (err) {
> -		put_device(&bus->dev);
> +	bus_registered = true;
> +	if (err)
>  		goto unregister;
> -	}
>  
>  	pcibios_add_bus(bus);
>  
> @@ -1110,12 +1110,15 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
>  unregister:
>  	put_device(&bridge->dev);
>  	device_del(&bridge->dev);
> -
>  free:
>  #ifdef CONFIG_PCI_DOMAINS_GENERIC
>  	pci_bus_release_domain_nr(parent, bus->domain_nr);
>  #endif
> -	kfree(bus);
> +	if (bus_registered)
> +		put_device(&bus->dev);
> +	else
> +		kfree(bus);
> +
>  	return err;
>  }
>  
> -- 
> 2.47.2
>