[B.A.T.M.A.N.] [PATCH 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(+)
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; }
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) {
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,
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: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 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 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
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);
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,
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
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;
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
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,
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
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,
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
b.a.t.m.a.n@lists.open-mesh.org
-
Antonio Quartulli -
Marek Lindner -
Matthias Schiffer