[B.A.T.M.A.N.] [PATCH 1/2] batman-adv: fix skb leak in batadv_dat_snoop_incoming_arp_reply()

[B.A.T.M.A.N.] [PATCH 1/2] batman-adv: fix skb leak in batadv_dat_snoop_incoming_arp_reply()

[Matthias Schiffer]

The callers of batadv_dat_snoop_incoming_arp_reply() assume the skb has been
freed when it returns true; fix this by calling kfree_skb before returning as
it is done in batadv_dat_snoop_incoming_arp_request().

Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
---
 distributed-arp-table.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/distributed-arp-table.c b/distributed-arp-table.c
index 7485a78..9f4cff3 100644
--- a/distributed-arp-table.c
+++ b/distributed-arp-table.c
@@ -1012,6 +1012,8 @@ bool batadv_dat_snoop_incoming_arp_reply(struct batadv_priv *bat_priv,
 	 */
 	ret = !batadv_is_my_client(bat_priv, hw_dst);
 out:
+	if (ret)
+		kfree_skb(skb);
 	/* if ret == false -> packet has to be delivered to the interface */
 	return ret;
 }
-- 
1.8.1.1

[B.A.T.M.A.N.] [PATCH 2/2] batman-adv: filter out invalid DAT entries

[Matthias Schiffer]

Due to duplicate address detection and other strange ARP packets, sometimes
entries with broadcast MAC addresses or unspecified IP addresses would get into
the Distributed ARP Table. This patch prevents these and some other kinds of
invalid entries from getting into the DAT.

Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
---
 distributed-arp-table.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/distributed-arp-table.c b/distributed-arp-table.c
index 9f4cff3..e28be57 100644
--- a/distributed-arp-table.c
+++ b/distributed-arp-table.c
@@ -274,6 +274,18 @@ static void batadv_dat_entry_add(struct batadv_priv *bat_priv, __be32 ip,
 	struct batadv_dat_entry *dat_entry;
 	int hash_added;
 
+	/* filter invalid MAC addresses that are sometimes used as
+	 * destinations of ARP replies
+	 */
+	if (is_zero_ether_addr(mac_addr) || is_multicast_ether_addr(mac_addr))
+		return;
+
+	/* ARP requests with unspecified source address are used for
+	 * duplicate address detection, we don't want those in the DAT either
+	 */
+	if (!ip)
+		return;
+
 	dat_entry = batadv_dat_entry_hash_find(bat_priv, ip);
 	/* if this entry is already known, just update it */
 	if (dat_entry) {
-- 
1.8.1.1

Re: [B.A.T.M.A.N.] [PATCH 2/2] batman-adv: filter out invalid DAT entries

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 1466 bytes --]

On Wed, Jan 23, 2013 at 06:11:54 +0100, Matthias Schiffer wrote:
> Due to duplicate address detection and other strange ARP packets, sometimes
> entries with broadcast MAC addresses or unspecified IP addresses would get into
> the Distributed ARP Table. This patch prevents these and some other kinds of
> invalid entries from getting into the DAT.
> 
> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
> ---
>  distributed-arp-table.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/distributed-arp-table.c b/distributed-arp-table.c
> index 9f4cff3..e28be57 100644
> --- a/distributed-arp-table.c
> +++ b/distributed-arp-table.c
> @@ -274,6 +274,18 @@ static void batadv_dat_entry_add(struct batadv_priv *bat_priv, __be32 ip,
>  	struct batadv_dat_entry *dat_entry;
>  	int hash_added;
>  
> +	/* filter invalid MAC addresses that are sometimes used as
> +	 * destinations of ARP replies
> +	 */
> +	if (is_zero_ether_addr(mac_addr) || is_multicast_ether_addr(mac_addr))
> +		return;
> +
> +	/* ARP requests with unspecified source address are used for
> +	 * duplicate address detection, we don't want those in the DAT either
> +	 */
> +	if (!ip)

Hi Matthias,
what about using ipv4_is_zeronet() ? Even if this is a base case, I would rather
prefer to use an already implemented function.

Cheers,

-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [B.A.T.M.A.N.] [PATCH 1/2] batman-adv: fix skb leak in batadv_dat_snoop_incoming_arp_reply()

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 539 bytes --]

Hi Matthias,

very good catch!

On Wed, Jan 23, 2013 at 06:11:53PM +0100, Matthias Schiffer wrote:
> The callers of batadv_dat_snoop_incoming_arp_reply() assume the skb has been
> freed when it returns true; fix this by calling kfree_skb before returning as
> it is done in batadv_dat_snoop_incoming_arp_request().
> 
> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>

Acked-by: Antonio Quartulli <ordex@autistici.org>

-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [B.A.T.M.A.N.] [PATCH 1/2] batman-adv: fix skb leak in batadv_dat_snoop_incoming_arp_reply()

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 738 bytes --]

On Thu, Jan 24, 2013 at 05:11:37AM +0800, Antonio Quartulli wrote:
> Hi Matthias,
> 
> very good catch!
> 
> On Wed, Jan 23, 2013 at 06:11:53PM +0100, Matthias Schiffer wrote:
> > The callers of batadv_dat_snoop_incoming_arp_reply() assume the skb has been
> > freed when it returns true; fix this by calling kfree_skb before returning as
> > it is done in batadv_dat_snoop_incoming_arp_request().
> > 
> > Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
> 
> Acked-by: Antonio Quartulli <ordex@autistici.org>

I forgot to say that this fix should be merged into maint, so that we can send
it to net.

Cheers,


-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [B.A.T.M.A.N.] [PATCH 2/2] batman-adv: filter out invalid DAT entries

[Matthias Schiffer]

[-- Attachment #1: Type: text/plain, Size: 1961 bytes --]

ipv4_is_zeronet() checks if the first byte of the address is zero, to my
knowledge there is no special funtion for checking for the unspecified
address, as the case is trivial and independent of byte ordering.

It might make sense though to check for different types of addresses
that are invalid for ARP (zeronet, loopback, multicast, etc.), but I
wanted to keep the patch as simple as possible. If you think these
should be filtered as well, I'll prepare a v2.

Matthias


On 01/23/2013 10:07 PM, Antonio Quartulli wrote:
> On Wed, Jan 23, 2013 at 06:11:54 +0100, Matthias Schiffer wrote:
>> Due to duplicate address detection and other strange ARP packets, sometimes
>> entries with broadcast MAC addresses or unspecified IP addresses would get into
>> the Distributed ARP Table. This patch prevents these and some other kinds of
>> invalid entries from getting into the DAT.
>>
>> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
>> ---
>>  distributed-arp-table.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/distributed-arp-table.c b/distributed-arp-table.c
>> index 9f4cff3..e28be57 100644
>> --- a/distributed-arp-table.c
>> +++ b/distributed-arp-table.c
>> @@ -274,6 +274,18 @@ static void batadv_dat_entry_add(struct batadv_priv *bat_priv, __be32 ip,
>>  	struct batadv_dat_entry *dat_entry;
>>  	int hash_added;
>>  
>> +	/* filter invalid MAC addresses that are sometimes used as
>> +	 * destinations of ARP replies
>> +	 */
>> +	if (is_zero_ether_addr(mac_addr) || is_multicast_ether_addr(mac_addr))
>> +		return;
>> +
>> +	/* ARP requests with unspecified source address are used for
>> +	 * duplicate address detection, we don't want those in the DAT either
>> +	 */
>> +	if (!ip)
> 
> Hi Matthias,
> what about using ipv4_is_zeronet() ? Even if this is a base case, I would rather
> prefer to use an already implemented function.
> 
> Cheers,
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [B.A.T.M.A.N.] [PATCH 2/2] batman-adv: filter out invalid DAT entries

[Matthias Schiffer]

[-- Attachment #1: Type: text/plain, Size: 2446 bytes --]

On 01/23/2013 10:39 PM, Matthias Schiffer wrote:
> ipv4_is_zeronet() checks if the first byte of the address is zero, to my
> knowledge there is no special funtion for checking for the unspecified
> address, as the case is trivial and independent of byte ordering.
> 
> It might make sense though to check for different types of addresses
> that are invalid for ARP (zeronet, loopback, multicast, etc.), but I
> wanted to keep the patch as simple as possible. If you think these
> should be filtered as well, I'll prepare a v2.
> 
> Matthias

Oh, I shouldn't top post. Well, continuing here now...

I just noticed that batadv_arp_get_type() already checks for loopback
and multicast addresses, so adding ipv4_is_zeronet() should be enough.
I'd keep that in batadv_dat_entry_add() though as the source of ARP
replies with 0.0.0.0 destination is still valid and can be should to the
DAT.

Matthias


> 
> 
> On 01/23/2013 10:07 PM, Antonio Quartulli wrote:
>> On Wed, Jan 23, 2013 at 06:11:54 +0100, Matthias Schiffer wrote:
>>> Due to duplicate address detection and other strange ARP packets, sometimes
>>> entries with broadcast MAC addresses or unspecified IP addresses would get into
>>> the Distributed ARP Table. This patch prevents these and some other kinds of
>>> invalid entries from getting into the DAT.
>>>
>>> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
>>> ---
>>>  distributed-arp-table.c | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/distributed-arp-table.c b/distributed-arp-table.c
>>> index 9f4cff3..e28be57 100644
>>> --- a/distributed-arp-table.c
>>> +++ b/distributed-arp-table.c
>>> @@ -274,6 +274,18 @@ static void batadv_dat_entry_add(struct batadv_priv *bat_priv, __be32 ip,
>>>  	struct batadv_dat_entry *dat_entry;
>>>  	int hash_added;
>>>  
>>> +	/* filter invalid MAC addresses that are sometimes used as
>>> +	 * destinations of ARP replies
>>> +	 */
>>> +	if (is_zero_ether_addr(mac_addr) || is_multicast_ether_addr(mac_addr))
>>> +		return;
>>> +
>>> +	/* ARP requests with unspecified source address are used for
>>> +	 * duplicate address detection, we don't want those in the DAT either
>>> +	 */
>>> +	if (!ip)
>>
>> Hi Matthias,
>> what about using ipv4_is_zeronet() ? Even if this is a base case, I would rather
>> prefer to use an already implemented function.
>>
>> Cheers,
>>
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [B.A.T.M.A.N.] [PATCH 2/2] batman-adv: filter out invalid DAT entries

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 887 bytes --]

On Wed, Jan 23, 2013 at 10:39:16PM +0100, Matthias Schiffer wrote:
> ipv4_is_zeronet() checks if the first byte of the address is zero, to my
> knowledge there is no special funtion for checking for the unspecified
> address, as the case is trivial and independent of byte ordering.
> 

correct. Then the change you proposed is fine with me.

> It might make sense though to check for different types of addresses
> that are invalid for ARP (zeronet, loopback, multicast, etc.), but I
> wanted to keep the patch as simple as possible. If you think these
> should be filtered as well, I'll prepare a v2.

in distributed-arp-table.c:784 you can already find these checks ;)

I think at this point the patch is ok. Thanks a lot!

Acked-by: Antonio Quartulli <ordex@autistici.org>


-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [B.A.T.M.A.N.] [PATCH 2/2] batman-adv: filter out invalid DAT entries

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 1231 bytes --]

On Wed, Jan 23, 2013 at 10:52:09PM +0100, Matthias Schiffer wrote:
> On 01/23/2013 10:39 PM, Matthias Schiffer wrote:
> > ipv4_is_zeronet() checks if the first byte of the address is zero, to my
> > knowledge there is no special funtion for checking for the unspecified
> > address, as the case is trivial and independent of byte ordering.
> > 
> > It might make sense though to check for different types of addresses
> > that are invalid for ARP (zeronet, loopback, multicast, etc.), but I
> > wanted to keep the patch as simple as possible. If you think these
> > should be filtered as well, I'll prepare a v2.
> > 
> > Matthias
> 
> Oh, I shouldn't top post. Well, continuing here now...
> 
> I just noticed that batadv_arp_get_type() already checks for loopback
> and multicast addresses, so adding ipv4_is_zeronet() should be enough.
> I'd keep that in batadv_dat_entry_add() though as the source of ARP
> replies with 0.0.0.0 destination is still valid and can be should to the
> DAT.

mh I would not split these checks if they are all logically connected.
Better to leave the patch as it is, imho.

Cheers,


-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [B.A.T.M.A.N.] [PATCH 1/2] batman-adv: fix skb leak in batadv_dat_snoop_incoming_arp_reply()

[Marek Lindner]

On Thursday, January 24, 2013 05:14:03 Antonio Quartulli wrote:
> On Thu, Jan 24, 2013 at 05:11:37AM +0800, Antonio Quartulli wrote:
> > Hi Matthias,
> > 
> > very good catch!
> > 
> > On Wed, Jan 23, 2013 at 06:11:53PM +0100, Matthias Schiffer wrote:
> > > The callers of batadv_dat_snoop_incoming_arp_reply() assume the skb has
> > > been freed when it returns true; fix this by calling kfree_skb before
> > > returning as it is done in batadv_dat_snoop_incoming_arp_request().
> > > 
> > > Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
> > 
> > Acked-by: Antonio Quartulli <ordex@autistici.org>
> 
> I forgot to say that this fix should be merged into maint, so that we can
> send it to net.

Applied in revision 977d8c6.

Thanks,
Marek

Re: [B.A.T.M.A.N.] [PATCH 2/2] batman-adv: filter out invalid DAT entries

[Marek Lindner]

On Thursday, January 24, 2013 05:53:52 Antonio Quartulli wrote:
> > It might make sense though to check for different types of addresses
> > that are invalid for ARP (zeronet, loopback, multicast, etc.), but I
> > wanted to keep the patch as simple as possible. If you think these
> > should be filtered as well, I'll prepare a v2.
> 
> in distributed-arp-table.c:784 you can already find these checks ;)

If most of the checking is done in batadv_arp_get_type() why not moving these 
checks there too ? That would allow to have all checks in the same place ?

Cheers,
Marek

Re: [B.A.T.M.A.N.] [PATCH 2/2] batman-adv: filter out invalid DAT entries

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 955 bytes --]

On Thu, Jan 24, 2013 at 09:36:11PM +0800, Marek Lindner wrote:
> On Thursday, January 24, 2013 05:53:52 Antonio Quartulli wrote:
> > > It might make sense though to check for different types of addresses
> > > that are invalid for ARP (zeronet, loopback, multicast, etc.), but I
> > > wanted to keep the patch as simple as possible. If you think these
> > > should be filtered as well, I'll prepare a v2.
> > 
> > in distributed-arp-table.c:784 you can already find these checks ;)
> 
> If most of the checking is done in batadv_arp_get_type() why not moving these 
> checks there too ? That would allow to have all checks in the same place ?

I thought the same, but in batadv_arp_get_type() we have a general check that
discards wrong/bogus ARP request.

Here instead we are filtering "correct" ARP requests that DAT should not handle.

Cheers,

-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [B.A.T.M.A.N.] [PATCH 2/2] batman-adv: filter out invalid DAT entries

[Marek Lindner]

On Thursday, January 24, 2013 21:38:58 Antonio Quartulli wrote:
> On Thu, Jan 24, 2013 at 09:36:11PM +0800, Marek Lindner wrote:
> > On Thursday, January 24, 2013 05:53:52 Antonio Quartulli wrote:
> > > > It might make sense though to check for different types of addresses
> > > > that are invalid for ARP (zeronet, loopback, multicast, etc.), but I
> > > > wanted to keep the patch as simple as possible. If you think these
> > > > should be filtered as well, I'll prepare a v2.
> > > 
> > > in distributed-arp-table.c:784 you can already find these checks ;)
> > 
> > If most of the checking is done in batadv_arp_get_type() why not moving
> > these checks there too ? That would allow to have all checks in the same
> > place ?
> 
> I thought the same, but in batadv_arp_get_type() we have a general check
> that discards wrong/bogus ARP request.
> 
> Here instead we are filtering "correct" ARP requests that DAT should not
> handle.

What is the difference except for the naming ? In both cases we don't want 
these packets to be handled by DAT. 

Feel free to move these extra validation checks into a separate function that 
gets called from batadv_arp_get_type() if you wish to emphasize the difference 
between the types of checks. Having all checks in the same place will help to 
avoid overlooking things later (as already happened).

Cheers,
Marek

Re: [B.A.T.M.A.N.] [PATCH 2/2] batman-adv: filter out invalid DAT entries

[Matthias Schiffer]

[-- Attachment #1: Type: text/plain, Size: 1353 bytes --]

On 01/24/2013 02:47 PM, Marek Lindner wrote:
> On Thursday, January 24, 2013 21:38:58 Antonio Quartulli wrote:
>>
>> I thought the same, but in batadv_arp_get_type() we have a general check
>> that discards wrong/bogus ARP request.
>>
>> Here instead we are filtering "correct" ARP requests that DAT should not
>> handle.
> 
> What is the difference except for the naming ? In both cases we don't want 
> these packets to be handled by DAT. 
> 
> Feel free to move these extra validation checks into a separate function that 
> gets called from batadv_arp_get_type() if you wish to emphasize the difference 
> between the types of checks. Having all checks in the same place will help to 
> avoid overlooking things later (as already happened).
> 
> Cheers,
> Marek
> 

In my opinion, the DAT should handle the sane one of source and
destination if one of them is sane and the other is bogus. So I would
maybe rather move all the checks to batadv_dat_entry_add()? There it
will only catch the add case though, not the lookup case...

At least a check for ff:ff:ff:ff:ff:ff should be added to maint as soon
as possible, as such entries were actually overwriting correct DAT
entries on my test node (and maybe even preventing ARP resolution as the
DAT node answered with these instead of the actual addresses).

Matthias


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [B.A.T.M.A.N.] [PATCH 2/2] batman-adv: filter out invalid DAT entries

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 1874 bytes --]

On Thu, Jan 24, 2013 at 03:44:35PM +0100, Matthias Schiffer wrote:
> On 01/24/2013 02:47 PM, Marek Lindner wrote:
> > On Thursday, January 24, 2013 21:38:58 Antonio Quartulli wrote:
> >>
> >> I thought the same, but in batadv_arp_get_type() we have a general check
> >> that discards wrong/bogus ARP request.
> >>
> >> Here instead we are filtering "correct" ARP requests that DAT should not
> >> handle.
> > 
> > What is the difference except for the naming ? In both cases we don't want 
> > these packets to be handled by DAT. 
> > 
> > Feel free to move these extra validation checks into a separate function that 
> > gets called from batadv_arp_get_type() if you wish to emphasize the difference 
> > between the types of checks. Having all checks in the same place will help to 
> > avoid overlooking things later (as already happened).
> > 
> > Cheers,
> > Marek
> > 
> 
> In my opinion, the DAT should handle the sane one of source and
> destination if one of them is sane and the other is bogus. So I would
> maybe rather move all the checks to batadv_dat_entry_add()? There it
> will only catch the add case though, not the lookup case...

I agree with Marek: adding these new checks in a separate function is probably
better. At that point batadv_arp_get_type() will directly refuse to parse
whatever ARP packet that DAT does not like.

> 
> At least a check for ff:ff:ff:ff:ff:ff should be added to maint as soon
> as possible, as such entries were actually overwriting correct DAT
> entries on my test node (and maybe even preventing ARP resolution as the
> DAT node answered with these instead of the actual addresses).
> 

Yes, I agree.
What about modifying your patch following the comments above and resend it?


Cheers,


-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [B.A.T.M.A.N.] [PATCH 2/2] batman-adv: filter out invalid DAT entries

[Matthias Schiffer]

Okay, here is the new version, in two parts. I like the first version better,
but maybe this one is more consistent...

Matthias

[B.A.T.M.A.N.] [PATCH v2 1/2] batman-adv: check for more types of invalid IP addresses in DAT

[Matthias Schiffer]

There are more types of IP addresses that may appear in ARP packets that we
don't want to process. While some of these should never appear in sane ARP
packets, a 0.0.0.0 source is used for duplicate address detection and thus seen
quite often.

Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
---
 distributed-arp-table.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/distributed-arp-table.c b/distributed-arp-table.c
index 9f4cff3..a35466a 100644
--- a/distributed-arp-table.c
+++ b/distributed-arp-table.c
@@ -777,7 +777,9 @@ static uint16_t batadv_arp_get_type(struct batadv_priv *bat_priv,
 	ip_src = batadv_arp_ip_src(skb, hdr_size);
 	ip_dst = batadv_arp_ip_dst(skb, hdr_size);
 	if (ipv4_is_loopback(ip_src) || ipv4_is_multicast(ip_src) ||
-	    ipv4_is_loopback(ip_dst) || ipv4_is_multicast(ip_dst))
+	    ipv4_is_zeronet(ip_src) || ipv4_is_lbcast(ip_src) ||
+	    ipv4_is_loopback(ip_dst) || ipv4_is_multicast(ip_dst) ||
+	    ipv4_is_zeronet(ip_dst) || ipv4_is_lbcast(ip_dst))
 		goto out;
 
 	type = ntohs(arphdr->ar_op);
-- 
1.8.1.1

[B.A.T.M.A.N.] [PATCH v2 2/2] batman-adv: filter ARP packets with invalid MAC addresses in DAT

[Matthias Schiffer]

We never want multicast MAC addresses in the Distributed ARP Table, so it's
best to completely ignore ARP packets containing them where we expect unicast
addresses.

Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
---
 distributed-arp-table.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/distributed-arp-table.c b/distributed-arp-table.c
index a35466a..c89a01e 100644
--- a/distributed-arp-table.c
+++ b/distributed-arp-table.c
@@ -738,6 +738,7 @@ static uint16_t batadv_arp_get_type(struct batadv_priv *bat_priv,
 	struct arphdr *arphdr;
 	struct ethhdr *ethhdr;
 	__be32 ip_src, ip_dst;
+	uint8_t *hw_src, *hw_dst;
 	uint16_t type = 0;
 
 	/* pull the ethernet header */
@@ -782,6 +783,18 @@ static uint16_t batadv_arp_get_type(struct batadv_priv *bat_priv,
 	    ipv4_is_zeronet(ip_dst) || ipv4_is_lbcast(ip_dst))
 		goto out;
 
+	hw_src = batadv_arp_hw_src(skb, hdr_size);
+	if (is_zero_ether_addr(hw_src) || is_multicast_ether_addr(hw_src))
+		goto out;
+
+	/* we don't care for the destination MAC address in ARP requests */
+	if (arphdr->ar_op != htons(ARPOP_REQUEST)) {
+		hw_dst = batadv_arp_hw_dst(skb, hdr_size);
+		if (is_zero_ether_addr(hw_dst) ||
+		    is_multicast_ether_addr(hw_dst))
+			goto out;
+	}
+
 	type = ntohs(arphdr->ar_op);
 out:
 	return type;
-- 
1.8.1.1

Re: [B.A.T.M.A.N.] [PATCH v2 2/2] batman-adv: filter ARP packets with invalid MAC addresses in DAT

[Matthias Schiffer]

[-- Attachment #1: Type: text/plain, Size: 1658 bytes --]

On 01/24/2013 06:18 PM, Matthias Schiffer wrote:
> We never want multicast MAC addresses in the Distributed ARP Table, so it's
> best to completely ignore ARP packets containing them where we expect unicast
> addresses.
> 
> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
> ---
>  distributed-arp-table.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/distributed-arp-table.c b/distributed-arp-table.c
> index a35466a..c89a01e 100644
> --- a/distributed-arp-table.c
> +++ b/distributed-arp-table.c
> @@ -738,6 +738,7 @@ static uint16_t batadv_arp_get_type(struct batadv_priv *bat_priv,
>  	struct arphdr *arphdr;
>  	struct ethhdr *ethhdr;
>  	__be32 ip_src, ip_dst;
> +	uint8_t *hw_src, *hw_dst;
>  	uint16_t type = 0;
>  
>  	/* pull the ethernet header */
> @@ -782,6 +783,18 @@ static uint16_t batadv_arp_get_type(struct batadv_priv *bat_priv,
>  	    ipv4_is_zeronet(ip_dst) || ipv4_is_lbcast(ip_dst))
>  		goto out;
>  
> +	hw_src = batadv_arp_hw_src(skb, hdr_size);
> +	if (is_zero_ether_addr(hw_src) || is_multicast_ether_addr(hw_src))
> +		goto out;
> +
> +	/* we don't care for the destination MAC address in ARP requests */
Oops, this comment should be "care about" ... if the patch is okay apart
from this, should I make a v3, or can you just fix it when applying the
patch?

> +	if (arphdr->ar_op != htons(ARPOP_REQUEST)) {
> +		hw_dst = batadv_arp_hw_dst(skb, hdr_size);
> +		if (is_zero_ether_addr(hw_dst) ||
> +		    is_multicast_ether_addr(hw_dst))
> +			goto out;
> +	}
> +
>  	type = ntohs(arphdr->ar_op);
>  out:
>  	return type;
> 

Matthias


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [B.A.T.M.A.N.] [PATCH v2 1/2] batman-adv: check for more types of invalid IP addresses in DAT

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 581 bytes --]

On Thu, Jan 24, 2013 at 06:18:26PM +0100, Matthias Schiffer wrote:
> There are more types of IP addresses that may appear in ARP packets that we
> don't want to process. While some of these should never appear in sane ARP
> packets, a 0.0.0.0 source is used for duplicate address detection and thus seen
> quite often.
> 
> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>

Acked-by: Antonio Quartulli <ordex@autistici.org>


Please merge into maint.

Cheers,

-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [B.A.T.M.A.N.] [PATCH v2 2/2] batman-adv: filter ARP packets with invalid MAC addresses in DAT

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 497 bytes --]

On Thu, Jan 24, 2013 at 06:18:27PM +0100, Matthias Schiffer wrote:
> We never want multicast MAC addresses in the Distributed ARP Table, so it's
> best to completely ignore ARP packets containing them where we expect unicast
> addresses.
> 
> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>

Acked-by: Antonio Quartulli <ordex@autistici.org>

Please merge into maint.

Cheers,

-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [B.A.T.M.A.N.] [PATCH v2 1/2] batman-adv: check for more types of invalid IP addresses in DAT

[Marek Lindner]

On Friday, January 25, 2013 21:27:07 Antonio Quartulli wrote:
>   On Thu, Jan 24, 2013 at 06:18:26PM +0100, Matthias Schiffer wrote:
> > There are more types of IP addresses that may appear in ARP packets that
> > we don't want to process. While some of these should never appear in
> > sane ARP packets, a 0.0.0.0 source is used for duplicate address
> > detection and thus seen quite often.
> >
> > Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
> 
> Acked-by: Antonio Quartulli <ordex@autistici.org>

Applied in revision 3b24193.

Thanks,
Marek

Re: [B.A.T.M.A.N.] [PATCH v2 2/2] batman-adv: filter ARP packets with invalid MAC addresses in DAT

[Marek Lindner]

On Friday, January 25, 2013 21:28:49 Antonio Quartulli wrote:
> On Thu, Jan 24, 2013 at 06:18:27PM +0100, Matthias Schiffer wrote:
> > We never want multicast MAC addresses in the Distributed ARP Table, so
> > it's best to completely ignore ARP packets containing them where we
> > expect unicast addresses.
> >
> > Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
> 
> Acked-by: Antonio Quartulli <ordex@autistici.org>

Applied in revision ab361a9.

Thanks,
Marek