Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
Cc:     harshit.m.mogalapalli@oracle.com, error27@gmail.com,
        dan.carpenter@linaro.org, kernel-janitors@vger.kernel.org,
        Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>,
        Arnd Bergmann <arnd@arndb.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: [PATCH] misc: microchip: pci1xxxx: Fix error handling in gp_aux_bus_probe()
Date:   Thu, 18 May 2023 09:33:33 -0700
Message-Id: <20230518163333.1355445-1-harshit.m.mogalapalli@oracle.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
To:     unlisted-recipients:; (no To-header on input)
Precedence: bulk

Smatch warns:
	drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c:73
	gp_aux_bus_probe() warn: missing unwind goto?

Apart from above warning that smatch warns, we have other issues with
this function.

1. The call to auxiliary_device_add() needs a matching call to
   auxiliary_device_delete(). When memory allocation for
   "aux_bus->aux_device_wrapper[1]" fails we should also delete
   auxiliary device for "aux_device_wrapper[0]".
2. In the error path when auxiliary_device_uninit() is called, it
   does trigger the release function --> gp_auxiliary_device_release(),
   this release function has the following:

	ida_free(&gp_client_ida, aux_device_wrapper->aux_dev.id);
	kfree(aux_device_wrapper);

   so few error paths have double frees. Eg: The goto label
   "err_aux_dev_add_0" first calls auxiliary_device_uninit() which also
   does an ida_free(), so when the control reaches "err_aux_dev_init_0"
   it will be a double free there.

Re-write the error handling code. Clean up manually before the
auxiliary_device_init() calls succeed and use gotos to clean up after
they succeed. Also change the goto label names to follow freeing the
last thing to make it more readable.

Fixes: 393fc2f5948f ("misc: microchip: pci1xxxx: load auxiliary bus driver for the PIO function in the multi-function endpoint of pci1xxxx device.")
Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
---
Only compile tested, from static analysis.
---
 drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c | 71 ++++++++++---------
 1 file changed, 38 insertions(+), 33 deletions(-)

diff --git a/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c
index 32af2b14ff34..f76ef6fd7bfc 100644
--- a/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c
+++ b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c
@@ -48,8 +48,10 @@ static int gp_aux_bus_probe(struct pci_dev *pdev, const struct pci_device_id *id
 		return -ENOMEM;
 
 	retval = ida_alloc(&gp_client_ida, GFP_KERNEL);
-	if (retval < 0)
-		goto err_ida_alloc_0;
+	if (retval < 0) {
+		kfree(aux_bus->aux_device_wrapper[0]);
+		return retval;
+	}
 
 	aux_bus->aux_device_wrapper[0]->aux_dev.name = aux_dev_otp_e2p_name;
 	aux_bus->aux_device_wrapper[0]->aux_dev.dev.parent = &pdev->dev;
@@ -60,21 +62,28 @@ static int gp_aux_bus_probe(struct pci_dev *pdev, const struct pci_device_id *id
 	aux_bus->aux_device_wrapper[0]->gp_aux_data.region_length = pci_resource_end(pdev, 0);
 
 	retval = auxiliary_device_init(&aux_bus->aux_device_wrapper[0]->aux_dev);
-	if (retval < 0)
-		goto err_aux_dev_init_0;
+	if (retval < 0) {
+		ida_free(&gp_client_ida, aux_bus->aux_device_wrapper[0]->aux_dev.id);
+		kfree(aux_bus->aux_device_wrapper[0]);
+		return retval;
+	}
 
 	retval = auxiliary_device_add(&aux_bus->aux_device_wrapper[0]->aux_dev);
 	if (retval)
-		goto err_aux_dev_add_0;
+		goto uninit_device_wrapper_0;
 
 	aux_bus->aux_device_wrapper[1] = kzalloc(sizeof(*aux_bus->aux_device_wrapper[1]),
 						 GFP_KERNEL);
-	if (!aux_bus->aux_device_wrapper[1])
-		return -ENOMEM;
+	if (!aux_bus->aux_device_wrapper[1]) {
+		retval = -ENOMEM;
+		goto delete_device_wrapper_0;
+	}
 
 	retval = ida_alloc(&gp_client_ida, GFP_KERNEL);
-	if (retval < 0)
-		goto err_ida_alloc_1;
+	if (retval < 0) {
+		kfree(aux_bus->aux_device_wrapper[1]);
+		goto delete_device_wrapper_0;
+	}
 
 	aux_bus->aux_device_wrapper[1]->aux_dev.name = aux_dev_gpio_name;
 	aux_bus->aux_device_wrapper[1]->aux_dev.dev.parent = &pdev->dev;
@@ -85,48 +94,44 @@ static int gp_aux_bus_probe(struct pci_dev *pdev, const struct pci_device_id *id
 	aux_bus->aux_device_wrapper[1]->gp_aux_data.region_length = pci_resource_end(pdev, 0);
 
 	retval = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
-
-	if (retval < 0)
-		goto err_aux_dev_init_1;
+	if (retval < 0) {
+		ida_free(&gp_client_ida, aux_bus->aux_device_wrapper[1]->aux_dev.id);
+		kfree(aux_bus->aux_device_wrapper[1]);
+		goto delete_device_wrapper_0;
+	}
 
 	retval = pci_irq_vector(pdev, 0);
-	if (retval < 0)
-		goto err_aux_dev_init_1;
+	if (retval < 0) {
+		ida_free(&gp_client_ida, aux_bus->aux_device_wrapper[1]->aux_dev.id);
+		kfree(aux_bus->aux_device_wrapper[1]);
+		goto delete_device_wrapper_0;
+	}
 
 	pdev->irq = retval;
 	aux_bus->aux_device_wrapper[1]->gp_aux_data.irq_num = pdev->irq;
 
 	retval = auxiliary_device_init(&aux_bus->aux_device_wrapper[1]->aux_dev);
-	if (retval < 0)
-		goto err_aux_dev_init_1;
+	if (retval < 0) {
+		ida_free(&gp_client_ida, aux_bus->aux_device_wrapper[1]->aux_dev.id);
+		kfree(aux_bus->aux_device_wrapper[1]);
+		goto delete_device_wrapper_0;
+	}
 
 	retval = auxiliary_device_add(&aux_bus->aux_device_wrapper[1]->aux_dev);
 	if (retval)
-		goto err_aux_dev_add_1;
+		goto uninit_device_wrapper_1;
 
 	pci_set_drvdata(pdev, aux_bus);
 	pci_set_master(pdev);
 
 	return 0;
 
-err_aux_dev_add_1:
+uninit_device_wrapper_1:
 	auxiliary_device_uninit(&aux_bus->aux_device_wrapper[1]->aux_dev);
-
-err_aux_dev_init_1:
-	ida_free(&gp_client_ida, aux_bus->aux_device_wrapper[1]->aux_dev.id);
-
-err_ida_alloc_1:
-	kfree(aux_bus->aux_device_wrapper[1]);
-
-err_aux_dev_add_0:
+delete_device_wrapper_0:
+	auxiliary_device_delete(&aux_bus->aux_device_wrapper[0]->aux_dev);
+uninit_device_wrapper_0:
 	auxiliary_device_uninit(&aux_bus->aux_device_wrapper[0]->aux_dev);
-
-err_aux_dev_init_0:
-	ida_free(&gp_client_ida, aux_bus->aux_device_wrapper[0]->aux_dev.id);
-
-err_ida_alloc_0:
-	kfree(aux_bus->aux_device_wrapper[0]);
-
 	return retval;
 }
 
-- 
2.31.1