[bug report] ARM: Orion: fix driver probe error handling with respect to clk

[bug report] ARM: Orion: fix driver probe error handling with respect to clk

[Dan Carpenter]

Hello Simon Baatz,

The patch baffab28b131: "ARM: Orion: fix driver probe error handling
with respect to clk" from Jul 19, 2012, leads to the following static
checker warning:

	drivers/mtd/nand/orion_nand.c:172 orion_nand_probe()
	warn: 'clk' was already freed.

drivers/mtd/nand/orion_nand.c
   150          /* Not all platforms can gate the clock, so it is not
   151             an error if the clock does not exists. */
   152          clk = clk_get(&pdev->dev, NULL);
   153          if (!IS_ERR(clk)) {
   154                  clk_prepare_enable(clk);
   155                  clk_put(clk);

Huh?  Apparently clk_get() and clk_put() are not ref counted
opperations?

You would think they would be from the name.  What it looks like to me
is that clk_put() should be renamed clk_free().  The comments on
clk_put() are not totally clear on this.  I'm just joking.  :P  There
aren't any comments...

   156          }
   157  
   158          ret = nand_scan(mtd, 1);
   159          if (ret)
   160                  goto no_dev;
   161  
   162          mtd->name = "orion_nand";
   163          ret = mtd_device_register(mtd, board->parts, board->nr_parts);
   164          if (ret) {
   165                  nand_release(mtd);
   166                  goto no_dev;
   167          }
   168  
   169          return 0;
   170  
   171  no_dev:
   172          if (!IS_ERR(clk)) {
   173                  clk_disable_unprepare(clk);

Any later reference to "clk" after clk_put() is a use after free.

   174                  clk_put(clk);
   175          }
   176  
   177          return ret;
   178  }

regards,
dan carpenter

Re: [bug report] ARM: Orion: fix driver probe error handling with respect to clk

[Simon Baatz]

Hi Dan

On Mon, Feb 06, 2017 at 06:29:47PM +0300, Dan Carpenter wrote:
> The patch baffab28b131: "ARM: Orion: fix driver probe error handling
> with respect to clk" from Jul 19, 2012, leads to the following static
> checker warning:
> 
> 	drivers/mtd/nand/orion_nand.c:172 orion_nand_probe()
> 	warn: 'clk' was already freed.
> 
> drivers/mtd/nand/orion_nand.c
>    150          /* Not all platforms can gate the clock, so it is not
>    151             an error if the clock does not exists. */
>    152          clk = clk_get(&pdev->dev, NULL);
>    153          if (!IS_ERR(clk)) {
>    154                  clk_prepare_enable(clk);
>    155                  clk_put(clk);
> 
> Huh?  Apparently clk_get() and clk_put() are not ref counted
> opperations?
> 
> You would think they would be from the name.  What it looks like to me
> is that clk_put() should be renamed clk_free().  The comments on
> clk_put() are not totally clear on this.  I'm just joking.  :P  There
> aren't any comments...

This looks fishy indeed (btw.  that's not my code).  Instead of
holding a clock pointer, the driver seems to use clk_get()/clk_put()
every time it "needs" a clock (see also orion_nand_remove()).  This
results in calling clk_put() on an enabled clock here and not holding
a reference to the clock after the probe.

Looks somewhat similar to the situation fixed in ac0696629d73 ('usb:
ehci-orion: fix clock reference leaking') to me.
 
>    156          }
>    157  
>    158          ret = nand_scan(mtd, 1);
>    159          if (ret)
>    160                  goto no_dev;
>    161  
>    162          mtd->name = "orion_nand";
>    163          ret = mtd_device_register(mtd, board->parts, board->nr_parts);
>    164          if (ret) {
>    165                  nand_release(mtd);
>    166                  goto no_dev;
>    167          }
>    168  
>    169          return 0;
>    170  
>    171  no_dev:
>    172          if (!IS_ERR(clk)) {
>    173                  clk_disable_unprepare(clk);
> 
> Any later reference to "clk" after clk_put() is a use after free.


Yes, sure. But the clk_put() above should not be there in the first
place. When I added this code, I probably should have had a closer
look at the clock handling above. (Or, more realistically, I just did
not understand enough at that time...)

> 
>    174                  clk_put(clk);
>    175          }
>    176  
>    177          return ret;
>    178  }


- Simon