linux-kernel - Re: question about drivers/pinctrl/pinctrl-at91.c

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 11 Dec 2012 09:06:59 +0000
From:	Grant Likely <grant.likely@...retlab.ca>
To:	Julia Lawall <julia.lawall@...6.fr>, plagnioj@...osoft.com,
	linus.walleij@...aro.org, rob.herring@...xeda.com,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	devicetree-discuss@...ts.ozlabs.org
Subject: Re: question about drivers/pinctrl/pinctrl-at91.c

On Sat, 8 Dec 2012 16:52:42 +0100 (CET), Julia Lawall <julia.lawall@...6.fr> wrote:
> The function at91_dt_node_to_map in drivers/pinctrl/pinctrl-at91.c 
> contains the following code:
> 
>         new_map = devm_kzalloc(pctldev->dev, sizeof(*new_map) * map_num, GFP_KERNEL);
>          if (!new_map)
>                  return -ENOMEM;
> 
>          *map = new_map;
>          *num_maps = map_num;
> 
>          /* create mux map */
>          parent = of_get_parent(np);
>          if (!parent) {
>                  kfree(new_map);
>                  return -EINVAL;
>          }
> 
> This is clearly not correct, because the combination of devm_kzalloc and 
> kfree risks creating a double free.  But I am not sure how best to fix it. 
> Is the data structure intended to normally exist until the driver's remove 
> function is called?  If so, perhaps the devm_kzalloc is OK.  If I just 
> remove the kfree, then the structure will persist until the remove 
> function is called, even though there was an error, which is perhaps not 
> good.  So I could change the kfree to devm_kfree?

Hi Julia,

I don't have that file in my tree. Is it in linux-next? (I'm too lazy to
go and look)

Yes, devm_kfree() is the right thing to call, but I'd be tempted to just
drop the free entirely. If it fails there is something wrong and it will
get cleaned up when the probe returns a fail code... but look at the
code; if failing there is a valid way for the driver to operate, then
doing the cleanup is the right thing....

Alternately the of_get_parent() call could simply be moved above the
devm_kzalloc() and then the issue becomes moot.  :-)

g.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/