netdev - Re: Issue with gratuitous arps when new addr is different from cached addr

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]

Date:	Wed, 20 Nov 2013 22:06:34 -0800
From:	Salam Noureddine <noureddine@...stanetworks.com>
To:	Salam Noureddine <noureddine@...stanetworks.com>,
	"David S. Miller" <davem@...emloft.net>,
	Daniel Borkmann <dborkman@...hat.com>,
	Willem de Bruijn <willemb@...gle.com>,
	Phil Sutter <phil@....cc>, Eric Dumazet <edumazet@...gle.com>,
	netdev@...r.kernel.org
Subject: Re: Issue with gratuitous arps when new addr is different from cached addr

Isn't locktime useful in that case for limiting the rate of garp
changes to the arp cache?

Thanks,

Salam

On Wed, Nov 20, 2013 at 8:40 PM, Hannes Frederic Sowa
<hannes@...essinduktion.org> wrote:
> On Wed, Nov 20, 2013 at 04:40:52PM -0800, Salam Noureddine wrote:
>> Hi,
>>
>> It seems to me that neigh_update is not handling correctly the case
>> when the new address is different from the cached one and
>> NEIGH_UPDATE_F_OVERRIDE is not set. When we receive a gratuitous arp
>> request we check jiffies against the neigh->updated + locktime in
>> arp_process. If we're passed that time then the flag is set.
>>
>> In neigh_update, we set neigh->updated before checking for the case
>> where we have a new address and the override flag is not set. This
>> means, that we "extend the life of the old address". By setting
>> locktime to 2 sec and sending an arp with a new address every 1 sec, I
>> was able to perpetuate the old entry for as long as I wanted.
>>
>> To fix this, we can just move setting neigh->updated to after the
>> check for new address and override flag not present,
>>
>> --- linux-3.4.orig/net/core/neighbour.c
>> +++ linux-3.4/net/core/neighbour.c
>> @@ -1206,10 +1206,6 @@ int neigh_update(struct neighbour *neigh
>>                 lladdr = neigh->ha;
>>         }
>>
>> -       if (new & NUD_CONNECTED)
>> -               neigh->confirmed = jiffies;
>> -       neigh->updated = jiffies;
>> -
>>         /* If entry was valid and address is not changed,
>>            do not change entry state, if new one is STALE.
>>          */
>> @@ -1233,6 +1229,10 @@ int neigh_update(struct neighbour *neigh
>>                 }
>>         }
>>
>> +       if (new & NUD_CONNECTED)
>> +               neigh->confirmed = jiffies;
>> +       neigh->updated = jiffies;
>> +
>>         if (new != old) {
>>                 neigh_del_timer(neigh);
>>                 if (new & NUD_IN_TIMER)
>>
>> If that seems like a an acceptable solution, I would post a patch shortly.
>
> Yes, that would help. But wouldn't it be better if we detect garp and
> overwrite the lladdr with F_OVERWRITE? It would be nice if these could also be
> rate-limited.
>
> Greetings,
>
>   Hannes
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html