[PATCH v2] PCI/ACPI: Fix double free bug in pci_acpi_scan_root() function

[PATCH v2] PCI/ACPI: Fix double free bug in pci_acpi_scan_root() function

[Zhe Qiao]

The patch "PCI/ACPI: Fix allocated memory release on error in
pci_acpi_scan_root()" introduces a dual release issue. When
acpi_pci_root_creat() fails, the pci_cpi_can_root() function
will release 'ri ->cfg' and 'root_ops' in the error handling
path.However, acpi_pci_root_creat() will also call
__acpi_pci_root_release_info(), which in turn will call the
release_info hook, causing the same block of memory to be
released again.

Fixes: 631b2af2f357 ("PCI/ACPI: Fix allocated memory release on error in pci_acpi_scan_root()")
Signed-off-by: Zhe Qiao <qiaozhe@iscas.ac.cn>
---
v1 -> v2:
 - Restore all changes from the first version.
 - Remove unnecessary release info hooks.
 - Add a NULL check before calling info->ops->release_info().
 - Delete the currently unused pci_api_geneic_delease_info () function.
---
 drivers/acpi/pci_root.c |  3 ++-
 drivers/pci/pci-acpi.c  | 12 ------------
 2 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 74ade4160314..83628adbc56b 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -974,7 +974,8 @@ static void __acpi_pci_root_release_info(struct acpi_pci_root_info *info)
 		resource_list_destroy_entry(entry);
 	}
 
-	info->ops->release_info(info);
+	if (info->ops && info->ops->release_info)
+		info->ops->release_info(info);
 }
 
 static void acpi_pci_root_release_info(struct pci_host_bridge *bridge)
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index b78e0e417324..6e85816ee1c3 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -1652,17 +1652,6 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
 	return cfg;
 }
 
-/* release_info: free resources allocated by init_info */
-static void pci_acpi_generic_release_info(struct acpi_pci_root_info *ci)
-{
-	struct acpi_pci_generic_root_info *ri;
-
-	ri = container_of(ci, struct acpi_pci_generic_root_info, common);
-	pci_ecam_free(ri->cfg);
-	kfree(ci->ops);
-	kfree(ri);
-}
-
 /* Interface called from ACPI code to setup PCI host controller */
 struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
 {
@@ -1683,7 +1672,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
 	if (!ri->cfg)
 		goto free_root_ops;
 
-	root_ops->release_info = pci_acpi_generic_release_info;
 	root_ops->prepare_resources = pci_acpi_root_prepare_resources;
 	root_ops->pci_ops = (struct pci_ops *)&ri->cfg->ops->pci_ops;
 	bus = acpi_pci_root_create(root, root_ops, &ri->common, ri->cfg);
-- 
2.43.0

Re: [PATCH v2] PCI/ACPI: Fix double free bug in pci_acpi_scan_root() function

[Bjorn Helgaas]

[+cc Dan]

On Tue, Jun 17, 2025 at 10:37:38AM +0800, Zhe Qiao wrote:
> The patch "PCI/ACPI: Fix allocated memory release on error in
> pci_acpi_scan_root()" introduces a dual release issue. When
> acpi_pci_root_creat() fails, the pci_cpi_can_root() function
> will release 'ri ->cfg' and 'root_ops' in the error handling
> path.However, acpi_pci_root_creat() will also call
> __acpi_pci_root_release_info(), which in turn will call the
> release_info hook, causing the same block of memory to be
> released again.

These are all nits, but would have to be fixed before applying:

  - 'The patch "PCI/ACPI: Fix ..."' is not the usual way to identify a
    commit.  Use the same style as in the Fixes: tag below.

  - Typo in "acpi_pci_root_creat" (twice)

  - Typo in "pci_cpi_can_root"

  - Add space after the period in "path.However, ..."

  - Add "Reported-by: Dan Carpenter <dan.carpenter@linaro.org>" and
    "Closes: https://lore.kernel.org/all/aEmdnuw715btq7Q5@stanley.mountain/"
    and cc: Dan.

  - 631b2af2f357 appeared in v6.16-rc1, so we should try to get the
    fix into v6.16.  A hint after the "---" would be helpful to make
    sure that happens.

Wait a few days before reposting in case other folks have comments.

> Fixes: 631b2af2f357 ("PCI/ACPI: Fix allocated memory release on error in pci_acpi_scan_root()")
> Signed-off-by: Zhe Qiao <qiaozhe@iscas.ac.cn>
> ---
> v1 -> v2:
>  - Restore all changes from the first version.
>  - Remove unnecessary release info hooks.
>  - Add a NULL check before calling info->ops->release_info().
>  - Delete the currently unused pci_api_geneic_delease_info () function.
> ---
>  drivers/acpi/pci_root.c |  3 ++-
>  drivers/pci/pci-acpi.c  | 12 ------------
>  2 files changed, 2 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 74ade4160314..83628adbc56b 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -974,7 +974,8 @@ static void __acpi_pci_root_release_info(struct acpi_pci_root_info *info)
>  		resource_list_destroy_entry(entry);
>  	}
>  
> -	info->ops->release_info(info);
> +	if (info->ops && info->ops->release_info)
> +		info->ops->release_info(info);
>  }
>  
>  static void acpi_pci_root_release_info(struct pci_host_bridge *bridge)
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index b78e0e417324..6e85816ee1c3 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -1652,17 +1652,6 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
>  	return cfg;
>  }
>  
> -/* release_info: free resources allocated by init_info */
> -static void pci_acpi_generic_release_info(struct acpi_pci_root_info *ci)
> -{
> -	struct acpi_pci_generic_root_info *ri;
> -
> -	ri = container_of(ci, struct acpi_pci_generic_root_info, common);
> -	pci_ecam_free(ri->cfg);
> -	kfree(ci->ops);
> -	kfree(ri);
> -}
> -
>  /* Interface called from ACPI code to setup PCI host controller */
>  struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
>  {
> @@ -1683,7 +1672,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
>  	if (!ri->cfg)
>  		goto free_root_ops;
>  
> -	root_ops->release_info = pci_acpi_generic_release_info;
>  	root_ops->prepare_resources = pci_acpi_root_prepare_resources;
>  	root_ops->pci_ops = (struct pci_ops *)&ri->cfg->ops->pci_ops;
>  	bus = acpi_pci_root_create(root, root_ops, &ri->common, ri->cfg);
> -- 
> 2.43.0
> 

Re: [PATCH v2] PCI/ACPI: Fix double free bug in pci_acpi_scan_root() function

[Zhe Qiao]

Hi Bjorn

On Tue, Jun 17, 2025 at 10:40 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc Dan]
>
> On Tue, Jun 17, 2025 at 10:37:38AM +0800, Zhe Qiao wrote:
> > The patch "PCI/ACPI: Fix allocated memory release on error in
> > pci_acpi_scan_root()" introduces a dual release issue. When
> > acpi_pci_root_creat() fails, the pci_cpi_can_root() function
> > will release 'ri ->cfg' and 'root_ops' in the error handling
> > path.However, acpi_pci_root_creat() will also call
> > __acpi_pci_root_release_info(), which in turn will call the
> > release_info hook, causing the same block of memory to be
> > released again.
>
> These are all nits, but would have to be fixed before applying:
>
>   - 'The patch "PCI/ACPI: Fix ..."' is not the usual way to identify a
>     commit.  Use the same style as in the Fixes: tag below.
>
>   - Typo in "acpi_pci_root_creat" (twice)
>
>   - Typo in "pci_cpi_can_root"
>
>   - Add space after the period in "path.However, ..."
>
>   - Add "Reported-by: Dan Carpenter <dan.carpenter@linaro.org>" and
>     "Closes: https://lore.kernel.org/all/aEmdnuw715btq7Q5@stanley.mountain/"
>     and cc: Dan.
>
>   - 631b2af2f357 appeared in v6.16-rc1, so we should try to get the
>     fix into v6.16.  A hint after the "---" would be helpful to make
>     sure that happens.
>
> Wait a few days before reposting in case other folks have comments.
>

Thank you for your detailed review and pointing out these issues.
I will wait a few days before resending.

Thanks again!

> > Fixes: 631b2af2f357 ("PCI/ACPI: Fix allocated memory release on error in pci_acpi_scan_root()")
> > Signed-off-by: Zhe Qiao <qiaozhe@iscas.ac.cn>
> > ---
> > v1 -> v2:
> >  - Restore all changes from the first version.
> >  - Remove unnecessary release info hooks.
> >  - Add a NULL check before calling info->ops->release_info().
> >  - Delete the currently unused pci_api_geneic_delease_info () function.
> > ---
> >  drivers/acpi/pci_root.c |  3 ++-
> >  drivers/pci/pci-acpi.c  | 12 ------------
> >  2 files changed, 2 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> > index 74ade4160314..83628adbc56b 100644
> > --- a/drivers/acpi/pci_root.c
> > +++ b/drivers/acpi/pci_root.c
> > @@ -974,7 +974,8 @@ static void __acpi_pci_root_release_info(struct acpi_pci_root_info *info)
> >               resource_list_destroy_entry(entry);
> >       }
> >
> > -     info->ops->release_info(info);
> > +     if (info->ops && info->ops->release_info)
> > +             info->ops->release_info(info);
> >  }
> >
> >  static void acpi_pci_root_release_info(struct pci_host_bridge *bridge)
> > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> > index b78e0e417324..6e85816ee1c3 100644
> > --- a/drivers/pci/pci-acpi.c
> > +++ b/drivers/pci/pci-acpi.c
> > @@ -1652,17 +1652,6 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
> >       return cfg;
> >  }
> >
> > -/* release_info: free resources allocated by init_info */
> > -static void pci_acpi_generic_release_info(struct acpi_pci_root_info *ci)
> > -{
> > -     struct acpi_pci_generic_root_info *ri;
> > -
> > -     ri = container_of(ci, struct acpi_pci_generic_root_info, common);
> > -     pci_ecam_free(ri->cfg);
> > -     kfree(ci->ops);
> > -     kfree(ri);
> > -}
> > -
> >  /* Interface called from ACPI code to setup PCI host controller */
> >  struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
> >  {
> > @@ -1683,7 +1672,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
> >       if (!ri->cfg)
> >               goto free_root_ops;
> >
> > -     root_ops->release_info = pci_acpi_generic_release_info;
> >       root_ops->prepare_resources = pci_acpi_root_prepare_resources;
> >       root_ops->pci_ops = (struct pci_ops *)&ri->cfg->ops->pci_ops;
> >       bus = acpi_pci_root_create(root, root_ops, &ri->common, ri->cfg);
> > --
> > 2.43.0
> >

regards,
Zhe

Re: [PATCH v2] PCI/ACPI: Fix double free bug in pci_acpi_scan_root() function

[Dan Carpenter]

Wait, what?  We can't just delete pci_acpi_generic_release_info().
What's going to free all those things on the success path?  I
don't understand.

I had suggested you do something like this:

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 74ade4160314..ff5799b696d6 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -1012,20 +1012,20 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
 		 root->segment, busnum);
 
 	if (ops->init_info && ops->init_info(info))
-		goto out_release_info;
+		return NULL;
 	if (ops->prepare_resources)
 		ret = ops->prepare_resources(info);
 	else
 		ret = acpi_pci_probe_root_resources(info);
 	if (ret < 0)
-		goto out_release_info;
+		goto cleanup_init_info;
 
 	pci_acpi_root_add_resources(info);
 	pci_add_resource(&info->resources, &root->secondary);
 	bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
 				  sysdata, &info->resources);
 	if (!bus)
-		goto out_release_info;
+		goto cleanup_acpi_pci_probe_root_resources;
 
 	host_bridge = to_pci_host_bridge(bus->bridge);
 	if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL))
@@ -1053,8 +1053,10 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
 		dev_printk(KERN_DEBUG, &bus->dev, "on NUMA node %d\n", node);
 	return bus;
 
+cleanup_acpi_pci_probe_root_resources:
+	free_something_with_probe();
 out_release_info:
-	__acpi_pci_root_release_info(info);
+	free_something_info();
 	return NULL;
 }
 

But you'd need to replace the dummy code with actual code and
change the callers etc.  And I haven't looked at the actual
code to verify it's a good idea.

regards,
dan carpenter