netdev - Re: [PATCH] net/garp: avoid infinite loop if attribute already exists

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <4F707917.4020003@ll.mit.edu>
Date:	Mon, 26 Mar 2012 10:11:35 -0400
From:	"Ward, David - 0663 - MITLL" <david.ward@...mit.edu>
To:	"jorge@...2.net" <jorge@...2.net>
CC:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH] net/garp: avoid infinite loop if attribute already
 exists

On 26/03/12 07:23, Jorge Boncompte [DTI2] wrote:
> El 26/03/2012 0:43, David Ward escribió:
>> An infinite loop occurred if garp_attr_create was called with the
>> values of an existing attribute. Return -EEXIST instead.
> 	I should have sent this some months ago but others things keep me from doing it.
> 	Anyway, I think that the right thing to do it's reuse the attribute to not
> disturb the switch/network. Also, returning an error it's pointless because
> nobody checks vlan_gvrp_request_join() return and you'll end up in a state where
> the VLAN device has the GVRP flag but it's not announcing the attribute.

I think what you are saying is that if we try to create an attribute 
that already exists, we should leave the old attribute alone.  I agree 
with that, and the patch I sent does that.  I also think the fact that 
the attribute existed would likely indicate a bug in the GARP 
application, in which we are not withdrawing an existing attribute when 
we should.  Your patch warns on this condition.

Our patches are mostly the same.  One thing I notice with your patch is 
that for new attributes, it now traverses the RB tree twice.  And it 
doesn't free the memory for a new attribute if it wasn't inserted into 
the RB tree.  So, what if instead I modified my patch to add 
"WARN_ON(err)" to either garp_request_join or vlan_gvrp_request_join?  
This would also warn us on -ENOMEM.

David

>
>   	Please take a look at the conversation I had with Patrick in the patch commit log.
>
> 	If you agree, I'm fine with you redoing your patch or David using mine if he
> thinks it's ok.
>
> 	Regards,
> 		Jorge
>
>> Signed-off-by: David Ward<david.ward@...mit.edu>
>> ---
>>   net/802/garp.c |   18 +++++++++++++-----
>>   1 files changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/802/garp.c b/net/802/garp.c
>> index 8e21b6d..bb5015e 100644
>> --- a/net/802/garp.c
>> +++ b/net/802/garp.c
>> @@ -167,7 +167,7 @@ static struct garp_attr *garp_attr_lookup(const struct garp_applicant *app,
>>   	return NULL;
>>   }
>>
>> -static void garp_attr_insert(struct garp_applicant *app, struct garp_attr *new)
>> +static int garp_attr_insert(struct garp_applicant *app, struct garp_attr *new)
>>   {
>>   	struct rb_node *parent = NULL, **p =&app->gid.rb_node;
>>   	struct garp_attr *attr;
>> @@ -181,24 +181,32 @@ static void garp_attr_insert(struct garp_applicant *app, struct garp_attr *new)
>>   			p =&parent->rb_left;
>>   		else if (d>  0)
>>   			p =&parent->rb_right;
>> +		else
>> +			return -EEXIST;
>>   	}
>>   	rb_link_node(&new->node, parent, p);
>>   	rb_insert_color(&new->node,&app->gid);
>> +	return 0;
>>   }
>>
>>   static struct garp_attr *garp_attr_create(struct garp_applicant *app,
>>   					  const void *data, u8 len, u8 type)
>>   {
>>   	struct garp_attr *attr;
>> +	int err;
>>
>>   	attr = kmalloc(sizeof(*attr) + len, GFP_ATOMIC);
>>   	if (!attr)
>> -		return attr;
>> +		return PTR_ERR(-ENOMEM);
>>   	attr->state = GARP_APPLICANT_VO;
>>   	attr->type  = type;
>>   	attr->dlen  = len;
>>   	memcpy(attr->data, data, len);
>> -	garp_attr_insert(app, attr);
>> +	err = garp_attr_insert(app, attr);
>> +	if (err<  0) {
>> +		kfree(attr);
>> +		return PTR_ERR(err);
>> +	}
>>   	return attr;
>>   }
>>
>> @@ -353,9 +361,9 @@ int garp_request_join(const struct net_device *dev,
>>
>>   	spin_lock_bh(&app->lock);
>>   	attr = garp_attr_create(app, data, len, type);
>> -	if (!attr) {
>> +	if (IS_ERR(attr)) {
>>   		spin_unlock_bh(&app->lock);
>> -		return -ENOMEM;
>> +		return ERR_PTR(attr);
>>   	}
>>   	garp_attr_event(app, attr, GARP_EVENT_REQ_JOIN);
>>   	spin_unlock_bh(&app->lock);


Download attachment "smime.p7s" of type "application/pkcs7-signature" (4558 bytes)