[maint] batman-adv: fix potential orig_node reference leak

  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(+)
  

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
  

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,
  

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...
  

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,
  

> 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
  

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
  

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,