[PATCH] atm: he: fix potential ioremap leak of membase in he_dev

[PATCH] atm: he: fix potential ioremap leak of membase in he_dev

[Gencen Gan]

From: Ganliber <u202011061@gmail.com>

In the function he_start() in drivers/atm/he.c, there
is no unmapping of he_dev->membase in the branch that
exits due to an error like reset failure, which may 
cause a memory leak.

Signed-off-by: Ganliber <u202011061@gmail.com>
Reviewed-by: Dongliang Mu <dzm91@hust.edu.cn>
---
 drivers/atm/he.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/atm/he.c b/drivers/atm/he.c
index ad91cc6a34fc..2d12b46aa5bd 100644
--- a/drivers/atm/he.c
+++ b/drivers/atm/he.c
@@ -1058,6 +1058,7 @@ static int he_start(struct atm_dev *dev)
 	status = he_readl(he_dev, RESET_CNTL);
 	if ((status & BOARD_RST_STATUS) == 0) {
 		hprintk("reset failed\n");
+		iounmap(he_dev->membase);
 		return -EINVAL;
 	}
 
@@ -1114,8 +1115,10 @@ static int he_start(struct atm_dev *dev)
 	he_writel(he_dev, lb_swap, LB_SWAP);
 
 	/* 4.10 initialize the interrupt queues */
-	if ((err = he_init_irq(he_dev)) != 0)
+	if ((err = he_init_irq(he_dev)) != 0) {
+		iounmap(he_dev->membase);
 		return err;
+	}
 
 	/* 4.11 enable pci bus controller state machines */
 	host_cntl |= (OUTFF_ENB | CMDFF_ENB |
@@ -1165,6 +1168,7 @@ static int he_start(struct atm_dev *dev)
 
 	if (nvpibits != -1 && nvcibits != -1 && nvpibits+nvcibits != HE_MAXCIDBITS) {
 		hprintk("nvpibits + nvcibits != %d\n", HE_MAXCIDBITS);
+		iounmap(he_dev->membase);
 		return -ENODEV;
 	}
 
@@ -1413,8 +1417,10 @@ static int he_start(struct atm_dev *dev)
 
 	/* 5.1.8 cs block connection memory initialization */
 	
-	if (he_init_cs_block_rcm(he_dev) < 0)
+	if (he_init_cs_block_rcm(he_dev) < 0) {
+		iounmap(he_dev->membase);
 		return -ENOMEM;
+	}
 
 	/* 5.1.10 initialize host structures */
 
@@ -1424,13 +1430,16 @@ static int he_start(struct atm_dev *dev)
 					   sizeof(struct he_tpd), TPD_ALIGNMENT, 0);
 	if (he_dev->tpd_pool == NULL) {
 		hprintk("unable to create tpd dma_pool\n");
+		iounmap(he_dev->membase);
 		return -ENOMEM;         
 	}
 
 	INIT_LIST_HEAD(&he_dev->outstanding_tpds);
 
-	if (he_init_group(he_dev, 0) != 0)
+	if (he_init_group(he_dev, 0) != 0) {
+		iounmap(he_dev->membase);
 		return -ENOMEM;
+	}
 
 	for (group = 1; group < HE_NUM_GROUPS; ++group) {
 		he_writel(he_dev, 0x0, G0_RBPS_S + (group * 32));
@@ -1465,6 +1474,7 @@ static int he_start(struct atm_dev *dev)
 					 &he_dev->hsp_phys, GFP_KERNEL);
 	if (he_dev->hsp == NULL) {
 		hprintk("failed to allocate host status page\n");
+		iounmap(he_dev->membase);
 		return -ENOMEM;
 	}
 	he_writel(he_dev, he_dev->hsp_phys, HSP_BA);
-- 
2.34.1

Re: [PATCH] atm: he: fix potential ioremap leak of membase in he_dev

[Francois Romieu]

Gencen Gan <u202011061@gmail.com> :
> In the function he_start() in drivers/atm/he.c, there
> is no unmapping of he_dev->membase in the branch that
> exits due to an error like reset failure, which may
> cause a memory leak.

Why would he_dev->membase not be unmapped in he_stop() ?

he_stop() is paired with he_start() as soon a he_start() returns
anything different from 0 in he_init_one(). I see no other place
where he_start() is used.

The atm_dev/he_dev pointers also seem correctly set.

-- 
Ueimor

Re: [PATCH] atm: he: fix potential ioremap leak of membase in he_dev

[Dongliang Mu]

On 3/10/23 19:28, Francois Romieu wrote:
> Gencen Gan <u202011061@gmail.com> :
>> In the function he_start() in drivers/atm/he.c, there
>> is no unmapping of he_dev->membase in the branch that
>> exits due to an error like reset failure, which may
>> cause a memory leak.
> 
> Why would he_dev->membase not be unmapped in he_stop() ?
> 
> he_stop() is paired with he_start() as soon a he_start() returns
> anything different from 0 in he_init_one(). I see no other place
> where he_start() is used.

Yes, you're right. We will check more about reports from the static 
checker Smatch.

Smatch should make a false positive here, I think it might be that, 
Smatch has an assumption about do and its paired undo functions. The do 
function should clean up its own allocation operations. And the paired 
undo function can be only called if the do function succeeds.

+cc Dan Carpenter

Maybe @Dan could tell more about this point.

> 
> The atm_dev/he_dev pointers also seem correctly set.
> 

Re: [PATCH] atm: he: fix potential ioremap leak of membase in he_dev

[Dan Carpenter]

On Fri, Mar 10, 2023 at 09:15:43PM +0800, Dongliang Mu wrote:
> 
> 
> On 3/10/23 19:28, Francois Romieu wrote:
> > Gencen Gan <u202011061@gmail.com> :
> > > In the function he_start() in drivers/atm/he.c, there
> > > is no unmapping of he_dev->membase in the branch that
> > > exits due to an error like reset failure, which may
> > > cause a memory leak.
> > 
> > Why would he_dev->membase not be unmapped in he_stop() ?
> > 
> > he_stop() is paired with he_start() as soon a he_start() returns
> > anything different from 0 in he_init_one(). I see no other place
> > where he_start() is used.
> 
> Yes, you're right. We will check more about reports from the static checker
> Smatch.
> 
> Smatch should make a false positive here, I think it might be that, Smatch
> has an assumption about do and its paired undo functions. The do function
> should clean up its own allocation operations. And the paired undo function
> can be only called if the do function succeeds.
> 
> +cc Dan Carpenter
> 
> Maybe @Dan could tell more about this point.
> 

Yes.  Smatch is assuming that every function cleans up after itself.
Generally this is the way most functions do it.

Perhaps the best option here is to create a new warning for the double
free bug if this patch were applied.

regards,
dan carpenter