[1/2] batman-adv: fix skb leak in batadv_dat_snoop_incoming_arp_reply()

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

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,
  

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>
  

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,
  

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

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

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>
  

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,
  

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
  

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
  

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,
  

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
  

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
  

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,
  

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

Matthias
  

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