* [B.A.T.M.A.N.] [PATCH-maint] batman-adv: fix potential orig_node reference leak
@ 2014-02-08 15:45 Simon Wunderlich
2014-02-11 12:04 ` Marek Lindner
2014-02-11 17:26 ` Antonio Quartulli
0 siblings, 2 replies; 8+ messages in thread
From: Simon Wunderlich @ 2014-02-08 15:45 UTC (permalink / raw)
To: b.a.t.m.a.n
Since batadv_orig_node_new() sets the refcount to two, assuming that
the calling function will use a reference for putting the orig_node into
a hash or similar, both references must be freed if initialization of
the orig_node fails. Otherwise that object may be leaked in that error
case.
Reported-by: Antonio Quartulli <antonio@meshcoding.com>
Signed-off-by: Simon Wunderlich <sw@simonwunderlich.de>
---
bat_iv_ogm.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/bat_iv_ogm.c b/bat_iv_ogm.c
index 6f4fcdc..6000337 100644
--- a/bat_iv_ogm.c
+++ b/bat_iv_ogm.c
@@ -256,6 +256,8 @@ batadv_iv_ogm_orig_get(struct batadv_priv *bat_priv, const uint8_t *addr)
free_bcast_own:
kfree(orig_node->bat_iv.bcast_own);
free_orig_node:
+ /* free twice, as batadv_orig_node_new set refcount to 2 */
+ batadv_orig_node_free_ref(orig_node);
batadv_orig_node_free_ref(orig_node);
return NULL;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH-maint] batman-adv: fix potential orig_node reference leak
2014-02-08 15:45 [B.A.T.M.A.N.] [PATCH-maint] batman-adv: fix potential orig_node reference leak Simon Wunderlich
@ 2014-02-11 12:04 ` Marek Lindner
2014-02-11 17:26 ` Antonio Quartulli
1 sibling, 0 replies; 8+ messages in thread
From: Marek Lindner @ 2014-02-11 12:04 UTC (permalink / raw)
To: b.a.t.m.a.n
[-- Attachment #1: Type: text/plain, Size: 592 bytes --]
On Saturday 08 February 2014 16:45:06 Simon Wunderlich wrote:
> Since batadv_orig_node_new() sets the refcount to two, assuming that
> the calling function will use a reference for putting the orig_node into
> a hash or similar, both references must be freed if initialization of
> the orig_node fails. Otherwise that object may be leaked in that error
> case.
>
> Reported-by: Antonio Quartulli <antonio@meshcoding.com>
> Signed-off-by: Simon Wunderlich <sw@simonwunderlich.de>
> ---
> bat_iv_ogm.c | 2 ++
> 1 file changed, 2 insertions(+)
Applied in revision cb4d66e.
Thanks,
Marek
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH-maint] batman-adv: fix potential orig_node reference leak
2014-02-08 15:45 [B.A.T.M.A.N.] [PATCH-maint] batman-adv: fix potential orig_node reference leak Simon Wunderlich
2014-02-11 12:04 ` Marek Lindner
@ 2014-02-11 17:26 ` Antonio Quartulli
2014-02-11 17:29 ` Martin Hundebøll
2014-02-11 17:35 ` Simon Wunderlich
1 sibling, 2 replies; 8+ messages in thread
From: Antonio Quartulli @ 2014-02-11 17:26 UTC (permalink / raw)
To: b.a.t.m.a.n, Simon Wunderlich
[-- Attachment #1: Type: text/plain, Size: 1336 bytes --]
On 08/02/14 16:45, Simon Wunderlich wrote:
> Since batadv_orig_node_new() sets the refcount to two, assuming that
> the calling function will use a reference for putting the orig_node into
> a hash or similar, both references must be freed if initialization of
> the orig_node fails. Otherwise that object may be leaked in that error
> case.
>
> Reported-by: Antonio Quartulli <antonio@meshcoding.com>
> Signed-off-by: Simon Wunderlich <sw@simonwunderlich.de>
> ---
> bat_iv_ogm.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/bat_iv_ogm.c b/bat_iv_ogm.c
> index 6f4fcdc..6000337 100644
> --- a/bat_iv_ogm.c
> +++ b/bat_iv_ogm.c
> @@ -256,6 +256,8 @@ batadv_iv_ogm_orig_get(struct batadv_priv *bat_priv, const uint8_t *addr)
> free_bcast_own:
> kfree(orig_node->bat_iv.bcast_own);
> free_orig_node:
> + /* free twice, as batadv_orig_node_new set refcount to 2 */
> + batadv_orig_node_free_ref(orig_node);
> batadv_orig_node_free_ref(orig_node);
Coudln't we just invoke kfree(orig_node) here ? I think that if we hit
this point it is because the node has not added to the hash and
therefore it i snot used in any other context. This way we avoid the
double free_ref() and we don't trgger the whole RCU mechanism.
Or am I missing something?
Cheers,
--
Antonio Quartulli
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH-maint] batman-adv: fix potential orig_node reference leak
2014-02-11 17:26 ` Antonio Quartulli
@ 2014-02-11 17:29 ` Martin Hundebøll
2014-02-11 17:34 ` Antonio Quartulli
2014-02-11 17:35 ` Simon Wunderlich
1 sibling, 1 reply; 8+ messages in thread
From: Martin Hundebøll @ 2014-02-11 17:29 UTC (permalink / raw)
To: The list for a Better Approach To Mobile Ad-hoc Networking,
Simon Wunderlich
[-- Attachment #1: Type: text/plain, Size: 1605 bytes --]
On 2014-02-11 18:26, Antonio Quartulli wrote:
> On 08/02/14 16:45, Simon Wunderlich wrote:
>> Since batadv_orig_node_new() sets the refcount to two, assuming that
>> the calling function will use a reference for putting the orig_node into
>> a hash or similar, both references must be freed if initialization of
>> the orig_node fails. Otherwise that object may be leaked in that error
>> case.
>>
>> Reported-by: Antonio Quartulli <antonio@meshcoding.com>
>> Signed-off-by: Simon Wunderlich <sw@simonwunderlich.de>
>> ---
>> bat_iv_ogm.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/bat_iv_ogm.c b/bat_iv_ogm.c
>> index 6f4fcdc..6000337 100644
>> --- a/bat_iv_ogm.c
>> +++ b/bat_iv_ogm.c
>> @@ -256,6 +256,8 @@ batadv_iv_ogm_orig_get(struct batadv_priv *bat_priv, const uint8_t *addr)
>> free_bcast_own:
>> kfree(orig_node->bat_iv.bcast_own);
>> free_orig_node:
>> + /* free twice, as batadv_orig_node_new set refcount to 2 */
>> + batadv_orig_node_free_ref(orig_node);
>> batadv_orig_node_free_ref(orig_node);
>
> Coudln't we just invoke kfree(orig_node) here ? I think that if we hit
> this point it is because the node has not added to the hash and
> therefore it i snot used in any other context. This way we avoid the
> double free_ref() and we don't trgger the whole RCU mechanism.
>
> Or am I missing something?
batadv_<type>_free_ref() might have side effect that are not handled by
kfree alone...
--
Kind Regards
Martin Hundebøll
Frederiks Allé 99, 1.th
8000 Aarhus C
Denmark
+45 61 65 54 61
martin@hundeboll.net
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH-maint] batman-adv: fix potential orig_node reference leak
2014-02-11 17:29 ` Martin Hundebøll
@ 2014-02-11 17:34 ` Antonio Quartulli
2014-02-11 17:40 ` Marek Lindner
0 siblings, 1 reply; 8+ messages in thread
From: Antonio Quartulli @ 2014-02-11 17:34 UTC (permalink / raw)
To: b.a.t.m.a.n
[-- Attachment #1: Type: text/plain, Size: 1880 bytes --]
On 11/02/14 18:29, Martin Hundebøll wrote:
> On 2014-02-11 18:26, Antonio Quartulli wrote:
>> On 08/02/14 16:45, Simon Wunderlich wrote:
>>> Since batadv_orig_node_new() sets the refcount to two, assuming that
>>> the calling function will use a reference for putting the orig_node into
>>> a hash or similar, both references must be freed if initialization of
>>> the orig_node fails. Otherwise that object may be leaked in that error
>>> case.
>>>
>>> Reported-by: Antonio Quartulli <antonio@meshcoding.com>
>>> Signed-off-by: Simon Wunderlich <sw@simonwunderlich.de>
>>> ---
>>> bat_iv_ogm.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/bat_iv_ogm.c b/bat_iv_ogm.c
>>> index 6f4fcdc..6000337 100644
>>> --- a/bat_iv_ogm.c
>>> +++ b/bat_iv_ogm.c
>>> @@ -256,6 +256,8 @@ batadv_iv_ogm_orig_get(struct batadv_priv *bat_priv, const uint8_t *addr)
>>> free_bcast_own:
>>> kfree(orig_node->bat_iv.bcast_own);
>>> free_orig_node:
>>> + /* free twice, as batadv_orig_node_new set refcount to 2 */
>>> + batadv_orig_node_free_ref(orig_node);
>>> batadv_orig_node_free_ref(orig_node);
>>
>> Coudln't we just invoke kfree(orig_node) here ? I think that if we hit
>> this point it is because the node has not added to the hash and
>> therefore it i snot used in any other context. This way we avoid the
>> double free_ref() and we don't trgger the whole RCU mechanism.
>>
>> Or am I missing something?
>
> batadv_<type>_free_ref() might have side effect that are not handled by
> kfree alone...
true, but since the orig_node has not been returned yet there is no
other component in batman-adv which is using it.
Otherwise, we may want to define and invoke a free_now() version of this
function (like we have done for other objects). But I think kfree() is
safe here.
Cheers,
--
Antonio Quartulli
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH-maint] batman-adv: fix potential orig_node reference leak
2014-02-11 17:26 ` Antonio Quartulli
2014-02-11 17:29 ` Martin Hundebøll
@ 2014-02-11 17:35 ` Simon Wunderlich
1 sibling, 0 replies; 8+ messages in thread
From: Simon Wunderlich @ 2014-02-11 17:35 UTC (permalink / raw)
To: Antonio Quartulli; +Cc: b.a.t.m.a.n
> On 08/02/14 16:45, Simon Wunderlich wrote:
> > Since batadv_orig_node_new() sets the refcount to two, assuming that
> > the calling function will use a reference for putting the orig_node into
> > a hash or similar, both references must be freed if initialization of
> > the orig_node fails. Otherwise that object may be leaked in that error
> > case.
> >
> > Reported-by: Antonio Quartulli <antonio@meshcoding.com>
> > Signed-off-by: Simon Wunderlich <sw@simonwunderlich.de>
> > ---
> >
> > bat_iv_ogm.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/bat_iv_ogm.c b/bat_iv_ogm.c
> > index 6f4fcdc..6000337 100644
> > --- a/bat_iv_ogm.c
> > +++ b/bat_iv_ogm.c
> > @@ -256,6 +256,8 @@ batadv_iv_ogm_orig_get(struct batadv_priv *bat_priv,
> > const uint8_t *addr)
> >
> > free_bcast_own:
> > kfree(orig_node->bat_iv.bcast_own);
> >
> > free_orig_node:
> > + /* free twice, as batadv_orig_node_new set refcount to 2 */
> > + batadv_orig_node_free_ref(orig_node);
> >
> > batadv_orig_node_free_ref(orig_node);
>
> Coudln't we just invoke kfree(orig_node) here ? I think that if we hit
> this point it is because the node has not added to the hash and
> therefore it i snot used in any other context. This way we avoid the
> double free_ref() and we don't trgger the whole RCU mechanism.
>
> Or am I missing something?
What about already allocated substructures like bat_iv.bcast_own,
bat_iv.bacst_own_sum etc? Of course we could find out which is already
allocated and free that too, but that orig node free function does already
that.
Cheers,
Simon
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH-maint] batman-adv: fix potential orig_node reference leak
2014-02-11 17:34 ` Antonio Quartulli
@ 2014-02-11 17:40 ` Marek Lindner
2014-02-11 17:45 ` Antonio Quartulli
0 siblings, 1 reply; 8+ messages in thread
From: Marek Lindner @ 2014-02-11 17:40 UTC (permalink / raw)
To: The list for a Better Approach To Mobile Ad-hoc Networking
[-- Attachment #1: Type: text/plain, Size: 505 bytes --]
On Tuesday 11 February 2014 18:34:10 Antonio Quartulli wrote:
> true, but since the orig_node has not been returned yet there is no
> other component in batman-adv which is using it.
>
> Otherwise, we may want to define and invoke a free_now() version of this
> function (like we have done for other objects). But I think kfree() is
> safe here.
It may be safe now but will surely be forgotten later. A guaranteed source for
trouble. That is why we have cleanup routines for everything.
Cheers,
Marek
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH-maint] batman-adv: fix potential orig_node reference leak
2014-02-11 17:40 ` Marek Lindner
@ 2014-02-11 17:45 ` Antonio Quartulli
0 siblings, 0 replies; 8+ messages in thread
From: Antonio Quartulli @ 2014-02-11 17:45 UTC (permalink / raw)
To: b.a.t.m.a.n
[-- Attachment #1: Type: text/plain, Size: 1206 bytes --]
On 11/02/14 18:40, Marek Lindner wrote:
> On Tuesday 11 February 2014 18:34:10 Antonio Quartulli wrote:
>> true, but since the orig_node has not been returned yet there is no
>> other component in batman-adv which is using it.
>>
>> Otherwise, we may want to define and invoke a free_now() version of this
>> function (like we have done for other objects). But I think kfree() is
>> safe here.
>
> It may be safe now but will surely be forgotten later. A guaranteed source for
> trouble. That is why we have cleanup routines for everything.
>
True, in particular because we (as bat_iv_ogm.c) do not know what
batadv_orig_node_new() has allocated - a kfree() would be fine if the
object was allocated with a plain kmalloc(), but this is not the case.
By the way we have a bug here:
if we jump to
256 free_bcast_own:
257 kfree(orig_node->bat_iv.bcast_own);
bcast_own gets free'd but not assigned to NULL. Later
batadv_orig_node_free_rcu() (scheduled by batadv_orig_node_free_ref())
will call batadv_iv_ogm_orig_free() that will try to kfree() bcast_own
again (line 98 in bat_iv_ogm.c), thus leading to a double free. no?
Cheers,
--
Antonio Quartulli
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-02-11 17:45 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-02-08 15:45 [B.A.T.M.A.N.] [PATCH-maint] batman-adv: fix potential orig_node reference leak Simon Wunderlich
2014-02-11 12:04 ` Marek Lindner
2014-02-11 17:26 ` Antonio Quartulli
2014-02-11 17:29 ` Martin Hundebøll
2014-02-11 17:34 ` Antonio Quartulli
2014-02-11 17:40 ` Marek Lindner
2014-02-11 17:45 ` Antonio Quartulli
2014-02-11 17:35 ` Simon Wunderlich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).