[PATCH] agp: efficeon: Disable PCI device on probe failure

[PATCH] agp: efficeon: Disable PCI device on probe failure

[Myeonghun Pak]

agp_efficeon_probe() enables the PCI device before checking whether the
GART resource can be assigned and before registering the AGP bridge.  If
either of those later steps fails, the probe path can return without
dropping the enable reference.

Route the resource-assignment failure through a shared unwind path that
puts the bridge before disabling the PCI device.  Also check the return
value from agp_add_bridge() explicitly, disabling the PCI device on
failure while leaving the bridge cleanup to agp_add_bridge()'s failure
handling.

Balance the successful probe path by disabling the PCI device from remove()
after the AGP bridge has been removed and released.

This issue was identified during our ongoing static-analysis research while
reviewing kernel code.

Fixes: 46cfc58a77de ("agp: efficeon-agp: do not use PCI resources before pci_enable_device()")
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>

---
 drivers/char/agp/efficeon-agp.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/char/agp/efficeon-agp.c b/drivers/char/agp/efficeon-agp.c
index 0d25bbdc7e..03804b08ef 100644
--- a/drivers/char/agp/efficeon-agp.c
+++ b/drivers/char/agp/efficeon-agp.c
@@ -346,6 +346,7 @@ static int agp_efficeon_probe(struct pci_dev *pdev,
 	struct agp_bridge_data *bridge;
 	u8 cap_ptr;
 	struct resource *r;
+	int err;
 
 	cap_ptr = pci_find_capability(pdev, PCI_CAP_ID_AGP);
 	if (!cap_ptr)
@@ -388,8 +389,8 @@ static int agp_efficeon_probe(struct pci_dev *pdev,
 	if (!r->start && r->end) {
 		if (pci_assign_resource(pdev, 0)) {
 			printk(KERN_ERR PFX "could not assign resource 0\n");
-			agp_put_bridge(bridge);
-			return -ENODEV;
+			err = -ENODEV;
+			goto err_put_bridge;
 		}
 	}
 
@@ -401,7 +402,17 @@ static int agp_efficeon_probe(struct pci_dev *pdev,
 	}
 
 	pci_set_drvdata(pdev, bridge);
-	return agp_add_bridge(bridge);
+	err = agp_add_bridge(bridge);
+	if (err)
+		goto err_disable_device;
+
+	return 0;
+
+err_put_bridge:
+	agp_put_bridge(bridge);
+err_disable_device:
+	pci_disable_device(pdev);
+	return err;
 }
 
 static void agp_efficeon_remove(struct pci_dev *pdev)
@@ -410,6 +421,7 @@ static void agp_efficeon_remove(struct pci_dev *pdev)
 
 	agp_remove_bridge(bridge);
 	agp_put_bridge(bridge);
+	pci_disable_device(pdev);
 }
 
 static int agp_efficeon_resume(struct device *dev)
-- 
2.47.1

Re: [PATCH] agp: efficeon: Disable PCI device on probe failure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] The global agp_bridges list is accessed and modified locklessly, risking list corruption.
- [High] The global agp_bridge pointer becomes a dangling pointer (Use-After-Free) if the primary AGP bridge is removed while another bridge exists.
- [High] The efficeon AGP probe workaround for unassigned GART resources is dead code due to premature pci_enable_device() failure.
--

commit 36a9a4194e2f3dcd5be646e083175ac41ccbc0a0
Author: Myeonghun Pak <mhun512@gmail.com>
Subject: agp: efficeon: Disable PCI device on probe failure

agp_efficeon_probe() enables the PCI device before checking whether the
GART resource can be assigned and before registering the AGP bridge. If
either of those later steps fails, the probe path can return without
dropping the enable reference. Route the resource-assignment failure
through a shared unwind path that puts the bridge before disabling the
PCI device.

> diff --git a/drivers/char/agp/efficeon-agp.c b/drivers/char/agp/efficeon-agp.c
> index 0d25bbdc7e6af..8c8f0cfcb04d8 100644
> --- a/drivers/char/agp/efficeon-agp.c
> +++ b/drivers/char/agp/efficeon-agp.c
[ ... ]
> @@ -388,8 +389,8 @@ static int agp_efficeon_probe(struct pci_dev *pdev,
>  	if (!r->start && r->end) {
>  		if (pci_assign_resource(pdev, 0)) {
>  			printk(KERN_ERR PFX "could not assign resource 0\n");
> -			agp_put_bridge(bridge);
> -			return -ENODEV;
> +			err = -ENODEV;
> +			goto err_put_bridge;

[Severity: High]
This isn't a bug introduced by this patch, but is the Efficeon AGP probe
workaround for unassigned GART resources completely dead code?

In agp_efficeon_probe(), pci_enable_device() is called before we reach this
pci_assign_resource() fallback. Because pci_enable_device() internally calls
pci_enable_resources(), it will return -EINVAL if any mandatory resource is
unassigned. 

If the BIOS failed to assign the GART resource, pci_enable_device() will fail
and abort the probe prematurely, making this pci_assign_resource() logic
unreachable.

>  		}
>  	}
>  
> @@ -401,7 +402,17 @@ static int agp_efficeon_probe(struct pci_dev *pdev,
>  	}
>  
>  	pci_set_drvdata(pdev, bridge);
> -	return agp_add_bridge(bridge);
> +	err = agp_add_bridge(bridge);

[Severity: High]
This is a pre-existing issue, but does modifying the global agp_bridges list
here risk list corruption?

In drivers/char/agp/backend.c, agp_add_bridge() calls:
    list_add(&bridge->list, &agp_bridges);

And later in agp_remove_bridge():
    list_del(&bridge->list);

Both access the global agp_bridges list locklessly. If asynchronous PCI
probing is enabled or a parallel sysfs unbind occurs while another bridge is
probed or accessed by a client driver, could concurrent modifications corrupt
the linked list pointers?

> +	if (err)
> +		goto err_disable_device;
> +
> +	return 0;
> +
> +err_put_bridge:
> +	agp_put_bridge(bridge);

[Severity: High]
This is a pre-existing issue, but does agp_put_bridge() leave a dangling
pointer in the global agp_bridge variable if the primary AGP bridge is removed
while another bridge exists?

Looking at drivers/char/agp/backend.c:agp_put_bridge():

    kfree(bridge);
    if (list_empty(&agp_bridges))
            agp_bridge = NULL;

If multiple AGP bridges are probed, agp_bridge points to the first one. When
this first bridge is removed (e.g. via sysfs unbind), agp_put_bridge() frees
it using kfree(). However, since the second bridge is still in the list,
list_empty() evaluates to false, and agp_bridge remains unchanged, pointing
to freed memory.

Could subsequent accesses to the global agp_bridge pointer result in a
use-after-free?

> +err_disable_device:
> +	pci_disable_device(pdev);
> +	return err;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623075300.42474-1-mhun512@gmail.com?part=1