pull request net: batman-adv 2013-05-09

pull request net: batman-adv 2013-05-09

[Antonio Quartulli]

here you have four fixes intended for net.

1/4 fixes the parsing of a string sent from userspace in order to avoid random
memory access in case of string length of 0.

2/4 adds a check for the return value of pskb_trim_rcsum() in order to stop
processing the skb in case of failure.

3/4 prevents DAT (the Distributed ARP Table) to send cached ARP replies when
both the source and the destination of the snooped ARP request are local clients
(meaning: directly connected to the node). This can confuse a bridge where
batman-adv is enslaved.

4/4 fix a race condition in the main clean up procedure by reordering
sub-components freeing function invocations.


Please pull or let me know if there is any problem.

Thanks a lot,
	Antonio


The following changes since commit 4f924b2aa4d3cb30f07e57d6b608838edcbc0d88:

  if_cablemodem.h: Add parenthesis around ioctl macros (2013-05-08 13:13:30 -0700)

are available in the git repository at:

  git://git.open-mesh.org/linux-merge.git tags/batman-adv-fix-for-davem

for you to fetch changes up to a4361860351e87876aebd9595906d928ce8572c6:

  batman-adv: reorder clean up routine in order to avoid race conditions (2013-05-09 12:39:45 +0200)

----------------------------------------------------------------
Included changes:
- fix parsing of user typed protocol string to avoid random memory access in
  some cases
- check pskb_trim_rcsum() return value
- prevent DAT from sending ARP replies when not needed
- reorder the main clean up routine to prevent race conditions

----------------------------------------------------------------
Antonio Quartulli (2):
      batman-adv: make DAT drop ARP requests targeting local clients
      batman-adv: reorder clean up routine in order to avoid race conditions

Marek Lindner (2):
      batman-adv: check proto length before accessing proto string buffer
      batman-adv: check return value of pskb_trim_rcsum()

 net/batman-adv/distributed-arp-table.c | 13 +++++++++++++
 net/batman-adv/main.c                  | 18 +++++++++++++-----
 net/batman-adv/network-coding.c        |  8 ++++++--
 3 files changed, 32 insertions(+), 7 deletions(-)

[PATCH 1/4] batman-adv: check proto length before accessing proto string buffer

[Antonio Quartulli]

From: Marek Lindner <lindner_marek@yahoo.de>

batadv_param_set_ra() strips the trailing '\n' from the supplied
string buffer without checking the length of the buffer first. This
patches avoids random memory access and associated potential
crashes.

Reported-by: Sasha Levin <sasha.levin@oracle.com>
Signed-off-by: Marek Lindner <lindner_marek@yahoo.de>
Signed-off-by: Antonio Quartulli <ordex@autistici.org>
---
 net/batman-adv/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/batman-adv/main.c b/net/batman-adv/main.c
index 3e30a0f..9c620cd 100644
--- a/net/batman-adv/main.c
+++ b/net/batman-adv/main.c
@@ -475,7 +475,7 @@ static int batadv_param_set_ra(const char *val, const struct kernel_param *kp)
 	char *algo_name = (char *)val;
 	size_t name_len = strlen(algo_name);
 
-	if (algo_name[name_len - 1] == '\n')
+	if (name_len > 0 && algo_name[name_len - 1] == '\n')
 		algo_name[name_len - 1] = '\0';
 
 	bat_algo_ops = batadv_algo_get(algo_name);
-- 
1.8.1.5

[PATCH 2/4] batman-adv: check return value of pskb_trim_rcsum()

[Antonio Quartulli]

From: Marek Lindner <lindner_marek@yahoo.de>

Reported-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Marek Lindner <lindner_marek@yahoo.de>
Acked-by: Martin Hundebøll <martin@hundeboll.net>
Signed-off-by: Antonio Quartulli <ordex@autistici.org>
---
 net/batman-adv/network-coding.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/batman-adv/network-coding.c b/net/batman-adv/network-coding.c
index f7c5430..e84629e 100644
--- a/net/batman-adv/network-coding.c
+++ b/net/batman-adv/network-coding.c
@@ -1514,6 +1514,7 @@ batadv_nc_skb_decode_packet(struct batadv_priv *bat_priv, struct sk_buff *skb,
 	struct ethhdr *ethhdr, ethhdr_tmp;
 	uint8_t *orig_dest, ttl, ttvn;
 	unsigned int coding_len;
+	int err;
 
 	/* Save headers temporarily */
 	memcpy(&coded_packet_tmp, skb->data, sizeof(coded_packet_tmp));
@@ -1568,8 +1569,11 @@ batadv_nc_skb_decode_packet(struct batadv_priv *bat_priv, struct sk_buff *skb,
 			 coding_len);
 
 	/* Resize decoded skb if decoded with larger packet */
-	if (nc_packet->skb->len > coding_len + h_size)
-		pskb_trim_rcsum(skb, coding_len + h_size);
+	if (nc_packet->skb->len > coding_len + h_size) {
+		err = pskb_trim_rcsum(skb, coding_len + h_size);
+		if (err)
+			return NULL;
+	}
 
 	/* Create decoded unicast packet */
 	unicast_packet = (struct batadv_unicast_packet *)skb->data;
-- 
1.8.1.5

[PATCH 3/4] batman-adv: make DAT drop ARP requests targeting local clients

[Antonio Quartulli]

In the outgoing ARP request snooping routine in DAT, ARP
Request sent by local clients which are supposed to be
replied by other local clients can be silently dropped.

The destination host will reply by itself through the LAN
and therefore there is no need to involve DAT.

Reported-by: Carlos Quijano <carlos@crqgestion.es>
Signed-off-by: Antonio Quartulli <ordex@autistici.org>
Tested-by: Carlos Quijano <carlos@crqgestion.es>
Signed-off-by: Marek Lindner <lindner_marek@yahoo.de>
---
 net/batman-adv/distributed-arp-table.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/net/batman-adv/distributed-arp-table.c b/net/batman-adv/distributed-arp-table.c
index 8e15d96..2399920 100644
--- a/net/batman-adv/distributed-arp-table.c
+++ b/net/batman-adv/distributed-arp-table.c
@@ -837,6 +837,19 @@ bool batadv_dat_snoop_outgoing_arp_request(struct batadv_priv *bat_priv,
 
 	dat_entry = batadv_dat_entry_hash_find(bat_priv, ip_dst);
 	if (dat_entry) {
+		/* If the ARP request is destined for a local client the local
+		 * client will answer itself. DAT would only generate a
+		 * duplicate packet.
+		 *
+		 * Moreover, if the soft-interface is enslaved into a bridge, an
+		 * additional DAT answer may trigger kernel warnings about
+		 * a packet coming from the wrong port.
+		 */
+		if (batadv_is_my_client(bat_priv, dat_entry->mac_addr)) {
+			ret = true;
+			goto out;
+		}
+
 		skb_new = arp_create(ARPOP_REPLY, ETH_P_ARP, ip_src,
 				     bat_priv->soft_iface, ip_dst, hw_src,
 				     dat_entry->mac_addr, hw_src);
-- 
1.8.1.5

[PATCH 4/4] batman-adv: reorder clean up routine in order to avoid race conditions

[Antonio Quartulli]

nc_worker accesses the originator table during its periodic
work, but since the originator table is freed before
stopping the worker this leads to a global protection fault.

Fix this by killing the worker (in nc_free) before freeing
the originator table.

Moreover tidy up the entire clean up routine by running all
the subcomponents freeing procedures first and then killing
the TT and the originator tables at the end.

Signed-off-by: Antonio Quartulli <ordex@autistici.org>
Signed-off-by: Marek Lindner <lindner_marek@yahoo.de>
---
 net/batman-adv/main.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/net/batman-adv/main.c b/net/batman-adv/main.c
index 9c620cd..1240f07 100644
--- a/net/batman-adv/main.c
+++ b/net/batman-adv/main.c
@@ -163,14 +163,22 @@ void batadv_mesh_free(struct net_device *soft_iface)
 	batadv_vis_quit(bat_priv);
 
 	batadv_gw_node_purge(bat_priv);
-	batadv_originator_free(bat_priv);
 	batadv_nc_free(bat_priv);
-
-	batadv_tt_free(bat_priv);
-
-	batadv_bla_free(bat_priv);
-
 	batadv_dat_free(bat_priv);
+	batadv_bla_free(bat_priv);
+
+	/* Free the TT and the originator tables only after having terminated
+	 * all the other depending components which may use these structures for
+	 * their purposes.
+	 */
+	batadv_tt_free(bat_priv);
+
+	/* Since the originator table clean up routine is accessing the TT
+	 * tables as well, it has to be invoked after the TT tables have been
+	 * freed and marked as empty. This ensures that no cleanup RCU callbacks
+	 * accessing the TT data are scheduled for later execution.
+	 */
+	batadv_originator_free(bat_priv);
 
 	free_percpu(bat_priv->bat_counters);
 
-- 
1.8.1.5

Re: [B.A.T.M.A.N.] pull request net: batman-adv 2013-05-09

[Antonio Quartulli]

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

On Thu, May 09, 2013 at 12:56:43PM +0200, Antonio Quartulli wrote:
> here you have four fixes intended for net.

Ops, "Hello David," has been trimmed.
Sorry for that :)

Cheers,

-- 
Antonio Quartulli

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

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

Re: pull request net: batman-adv 2013-05-09

[David Miller]

From: Antonio Quartulli <ordex@autistici.org>
Date: Thu,  9 May 2013 12:56:43 +0200

> here you have four fixes intended for net.
> 
> 1/4 fixes the parsing of a string sent from userspace in order to avoid random
> memory access in case of string length of 0.
> 
> 2/4 adds a check for the return value of pskb_trim_rcsum() in order to stop
> processing the skb in case of failure.
> 
> 3/4 prevents DAT (the Distributed ARP Table) to send cached ARP replies when
> both the source and the destination of the snooped ARP request are local clients
> (meaning: directly connected to the node). This can confuse a bridge where
> batman-adv is enslaved.
> 
> 4/4 fix a race condition in the main clean up procedure by reordering
> sub-components freeing function invocations.
> 
> Please pull or let me know if there is any problem.

Pulled, thanks Antonio.