[B.A.T.M.A.N.] [PATCH maint 1/3] batman-adv: update data pointers after skb_cow()

[B.A.T.M.A.N.] [PATCH maint 1/3] batman-adv: update data pointers after skb_cow()

[Matthias Schiffer]

batadv_check_unicast_ttvn() calls skb_cow(), so pointers into the SKB data
must be (re)set after calling it. The ethhdr variable is dropped
altogether.

Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
---
 net/batman-adv/routing.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/batman-adv/routing.c b/net/batman-adv/routing.c
index 289df027ecdd..0f10c565ac85 100644
--- a/net/batman-adv/routing.c
+++ b/net/batman-adv/routing.c
@@ -968,14 +968,10 @@ int batadv_recv_unicast_packet(struct sk_buff *skb,
 	struct batadv_orig_node *orig_node = NULL, *orig_node_gw = NULL;
 	int check, hdr_size = sizeof(*unicast_packet);
 	enum batadv_subtype subtype;
-	struct ethhdr *ethhdr;
 	int ret = NET_RX_DROP;
 	bool is4addr, is_gw;
 
 	unicast_packet = (struct batadv_unicast_packet *)skb->data;
-	unicast_4addr_packet = (struct batadv_unicast_4addr_packet *)skb->data;
-	ethhdr = eth_hdr(skb);
-
 	is4addr = unicast_packet->packet_type == BATADV_UNICAST_4ADDR;
 	/* the caller function should have already pulled 2 bytes */
 	if (is4addr)
@@ -995,12 +991,14 @@ int batadv_recv_unicast_packet(struct sk_buff *skb,
 	if (!batadv_check_unicast_ttvn(bat_priv, skb, hdr_size))
 		goto free_skb;
 
+	unicast_packet = (struct batadv_unicast_packet *)skb->data;
+
 	/* packet for me */
 	if (batadv_is_my_mac(bat_priv, unicast_packet->dest)) {
 		/* If this is a unicast packet from another backgone gw,
 		 * drop it.
 		 */
-		orig_addr_gw = ethhdr->h_source;
+		orig_addr_gw = eth_hdr(skb)->h_source;
 		orig_node_gw = batadv_orig_hash_find(bat_priv, orig_addr_gw);
 		if (orig_node_gw) {
 			is_gw = batadv_bla_is_backbone_gw(skb, orig_node_gw,
@@ -1015,6 +1013,8 @@ int batadv_recv_unicast_packet(struct sk_buff *skb,
 		}
 
 		if (is4addr) {
+			unicast_4addr_packet =
+				(struct batadv_unicast_4addr_packet *)skb->data;
 			subtype = unicast_4addr_packet->subtype;
 			batadv_dat_inc_counter(bat_priv, subtype);
 
-- 
2.16.2

[B.A.T.M.A.N.] [PATCH maint 2/3] batman-adv: fix header size check in batadv_dbg_arp()

[Matthias Schiffer]

Checking for 0 is insufficient: when an SKB without a batadv header, but
with a VLAN header is received, hdr_size will be 4, making the following
code interpret the Ethernet header as a batadv header.

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

diff --git a/net/batman-adv/distributed-arp-table.c b/net/batman-adv/distributed-arp-table.c
index 19b15de455ab..b7ca4a311b91 100644
--- a/net/batman-adv/distributed-arp-table.c
+++ b/net/batman-adv/distributed-arp-table.c
@@ -393,7 +393,7 @@ static void batadv_dbg_arp(struct batadv_priv *bat_priv, struct sk_buff *skb,
 		   batadv_arp_hw_src(skb, hdr_size), &ip_src,
 		   batadv_arp_hw_dst(skb, hdr_size), &ip_dst);
 
-	if (hdr_size == 0)
+	if (hdr_size < sizeof(struct batadv_unicast_packet))
 		return;
 
 	unicast_4addr_packet = (struct batadv_unicast_4addr_packet *)skb->data;
-- 
2.16.2

Re: [B.A.T.M.A.N.] [PATCH maint 2/3] batman-adv: fix header size check in batadv_dbg_arp()

[Sven Eckelmann]

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

On Freitag, 16. März 2018 11:29:10 CET Matthias Schiffer wrote:
> Checking for 0 is insufficient: when an SKB without a batadv header, but
> with a VLAN header is received, hdr_size will be 4, making the following
> code interpret the Ethernet header as a batadv header.
> 
> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
> ---

Fixes: 0591c25abca9 ("batman-adv: Distributed ARP Table - add ARP parsing functions")

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [B.A.T.M.A.N.] [PATCH maint 2/3] batman-adv: fix header size check in batadv_dbg_arp()

[Matthias Schiffer]

[-- Attachment #1.1: Type: text/plain, Size: 671 bytes --]

On 03/16/2018 12:31 PM, Sven Eckelmann wrote:
> On Freitag, 16. März 2018 11:29:10 CET Matthias Schiffer wrote:
>> Checking for 0 is insufficient: when an SKB without a batadv header, but
>> with a VLAN header is received, hdr_size will be 4, making the following
>> code interpret the Ethernet header as a batadv header.
>>
>> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
>> ---
> 
> Fixes: 0591c25abca9 ("batman-adv: Distributed ARP Table - add ARP parsing functions")

I think this one would be more appropriate:

Fixes: 3e26722bc9f2 ("batman-adv: make the Distributed ARP Table vlan aware")

> 
> Kind regards,
> 	Sven
> 



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

[B.A.T.M.A.N.] [PATCH maint 3/3] batman-adv: do not modify batadv packet header before pulling it

[Matthias Schiffer]

batadv_check_unicast_ttvn() may modify the batadv header, leading to
checksum errors in the following processing of the packet.

Rather than fixing up the checksum, simply pull the batadv header before
modifying it (and push it back in case the packet is rerouted).

Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
---
 net/batman-adv/routing.c        | 38 +++++++++++++++++++++-----------------
 net/batman-adv/soft-interface.c | 10 ++--------
 net/batman-adv/soft-interface.h |  2 +-
 3 files changed, 24 insertions(+), 26 deletions(-)

diff --git a/net/batman-adv/routing.c b/net/batman-adv/routing.c
index 0f10c565ac85..37b87fce685b 100644
--- a/net/batman-adv/routing.c
+++ b/net/batman-adv/routing.c
@@ -824,16 +824,16 @@ static bool batadv_check_unicast_ttvn(struct batadv_priv *bat_priv,
 	int is_old_ttvn;
 
 	/* check if there is enough data before accessing it */
-	if (!pskb_may_pull(skb, hdr_len + ETH_HLEN))
+	if (!pskb_may_pull(skb, ETH_HLEN))
 		return false;
 
 	/* create a copy of the skb (in case of for re-routing) to modify it. */
-	if (skb_cow(skb, sizeof(*unicast_packet)) < 0)
+	if (skb_cow_head(skb, ETH_HLEN + hdr_len) < 0)
 		return false;
 
-	unicast_packet = (struct batadv_unicast_packet *)skb->data;
-	vid = batadv_get_vid(skb, hdr_len);
-	ethhdr = (struct ethhdr *)(skb->data + hdr_len);
+	unicast_packet = (struct batadv_unicast_packet *)(skb->data - hdr_len);
+	vid = batadv_get_vid(skb, 0);
+	ethhdr = (struct ethhdr *)skb->data;
 
 	/* check if the destination client was served by this node and it is now
 	 * roaming. In this case, it means that the node has got a ROAM_ADV
@@ -985,13 +985,16 @@ int batadv_recv_unicast_packet(struct sk_buff *skb,
 	 */
 	if (check == -EREMOTE)
 		batadv_nc_skb_store_sniffed_unicast(bat_priv, skb);
-
 	if (check < 0)
 		goto free_skb;
+
+	/* batadv_check_unicast_packet has checked if we may pull */
+	skb_pull_rcsum(skb, hdr_size);
+
 	if (!batadv_check_unicast_ttvn(bat_priv, skb, hdr_size))
 		goto free_skb;
 
-	unicast_packet = (struct batadv_unicast_packet *)skb->data;
+	unicast_packet = (struct batadv_unicast_packet *)(skb->data - hdr_size);
 
 	/* packet for me */
 	if (batadv_is_my_mac(bat_priv, unicast_packet->dest)) {
@@ -1001,8 +1004,7 @@ int batadv_recv_unicast_packet(struct sk_buff *skb,
 		orig_addr_gw = eth_hdr(skb)->h_source;
 		orig_node_gw = batadv_orig_hash_find(bat_priv, orig_addr_gw);
 		if (orig_node_gw) {
-			is_gw = batadv_bla_is_backbone_gw(skb, orig_node_gw,
-							  hdr_size);
+			is_gw = batadv_bla_is_backbone_gw(skb, orig_node_gw, 0);
 			batadv_orig_node_put(orig_node_gw);
 			if (is_gw) {
 				batadv_dbg(BATADV_DBG_BLA, bat_priv,
@@ -1014,7 +1016,8 @@ int batadv_recv_unicast_packet(struct sk_buff *skb,
 
 		if (is4addr) {
 			unicast_4addr_packet =
-				(struct batadv_unicast_4addr_packet *)skb->data;
+				(struct batadv_unicast_4addr_packet *)
+				unicast_packet;
 			subtype = unicast_4addr_packet->subtype;
 			batadv_dat_inc_counter(bat_priv, subtype);
 
@@ -1031,15 +1034,12 @@ int batadv_recv_unicast_packet(struct sk_buff *skb,
 			}
 		}
 
-		if (batadv_dat_snoop_incoming_arp_request(bat_priv, skb,
-							  hdr_size))
+		if (batadv_dat_snoop_incoming_arp_request(bat_priv, skb, 0))
 			goto rx_success;
-		if (batadv_dat_snoop_incoming_arp_reply(bat_priv, skb,
-							hdr_size))
+		if (batadv_dat_snoop_incoming_arp_reply(bat_priv, skb, 0))
 			goto rx_success;
 
-		batadv_interface_rx(recv_if->soft_iface, skb, hdr_size,
-				    orig_node);
+		batadv_interface_rx(recv_if->soft_iface, skb, false, orig_node);
 
 rx_success:
 		if (orig_node)
@@ -1048,6 +1048,8 @@ int batadv_recv_unicast_packet(struct sk_buff *skb,
 		return NET_RX_SUCCESS;
 	}
 
+	skb_push_rcsum(skb, hdr_size);
+
 	ret = batadv_route_unicast_packet(skb, recv_if);
 	/* skb was consumed */
 	skb = NULL;
@@ -1273,8 +1275,10 @@ int batadv_recv_bcast_packet(struct sk_buff *skb,
 	if (batadv_dat_snoop_incoming_arp_reply(bat_priv, skb, hdr_size))
 		goto rx_success;
 
+	skb_pull_rcsum(skb, hdr_size);
+
 	/* broadcast for me */
-	batadv_interface_rx(recv_if->soft_iface, skb, hdr_size, orig_node);
+	batadv_interface_rx(recv_if->soft_iface, skb, true, orig_node);
 
 rx_success:
 	ret = NET_RX_SUCCESS;
diff --git a/net/batman-adv/soft-interface.c b/net/batman-adv/soft-interface.c
index edeffcb9f3a2..370770759bb8 100644
--- a/net/batman-adv/soft-interface.c
+++ b/net/batman-adv/soft-interface.c
@@ -400,7 +400,7 @@ static int batadv_interface_tx(struct sk_buff *skb,
  * batadv_interface_rx() - receive ethernet frame on local batman-adv interface
  * @soft_iface: local interface which will receive the ethernet frame
  * @skb: ethernet frame for @soft_iface
- * @hdr_size: size of already parsed batman-adv header
+ * @is_bcast: true if the received frame is a batman-adv broadcast
  * @orig_node: originator from which the batman-adv packet was sent
  *
  * Sends a ethernet frame to the receive path of the local @soft_iface.
@@ -414,20 +414,14 @@ static int batadv_interface_tx(struct sk_buff *skb,
  * isolated clients.
  */
 void batadv_interface_rx(struct net_device *soft_iface,
-			 struct sk_buff *skb, int hdr_size,
+			 struct sk_buff *skb, bool is_bcast,
 			 struct batadv_orig_node *orig_node)
 {
-	struct batadv_bcast_packet *batadv_bcast_packet;
 	struct batadv_priv *bat_priv = netdev_priv(soft_iface);
 	struct vlan_ethhdr *vhdr;
 	struct ethhdr *ethhdr;
 	unsigned short vid;
-	bool is_bcast;
 
-	batadv_bcast_packet = (struct batadv_bcast_packet *)skb->data;
-	is_bcast = (batadv_bcast_packet->packet_type == BATADV_BCAST);
-
-	skb_pull_rcsum(skb, hdr_size);
 	skb_reset_mac_header(skb);
 
 	/* clean the netfilter state now that the batman-adv header has been
diff --git a/net/batman-adv/soft-interface.h b/net/batman-adv/soft-interface.h
index daf87f07fadd..53071d45093a 100644
--- a/net/batman-adv/soft-interface.h
+++ b/net/batman-adv/soft-interface.h
@@ -30,7 +30,7 @@ struct sk_buff;
 
 int batadv_skb_head_push(struct sk_buff *skb, unsigned int len);
 void batadv_interface_rx(struct net_device *soft_iface,
-			 struct sk_buff *skb, int hdr_size,
+			 struct sk_buff *skb, bool is_bcast,
 			 struct batadv_orig_node *orig_node);
 struct net_device *batadv_softif_create(struct net *net, const char *name);
 void batadv_softif_destroy_sysfs(struct net_device *soft_iface);
-- 
2.16.2

Re: [B.A.T.M.A.N.] [PATCH maint 3/3] batman-adv: do not modify batadv packet header before pulling it

[Sven Eckelmann]

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

On Freitag, 16. März 2018 11:29:11 CET Matthias Schiffer wrote:
> batadv_check_unicast_ttvn() may modify the batadv header, leading to
> checksum errors in the following processing of the packet.
> 
> Rather than fixing up the checksum, simply pull the batadv header before
> modifying it (and push it back in case the packet is rerouted).
> 
> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
> ---

Fixes: cea194d90b11 ("batman-adv: improved client announcement mechanism")

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [B.A.T.M.A.N.] [PATCH maint 3/3] batman-adv: do not modify batadv packet header before pulling it

[Sven Eckelmann]

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

On Freitag, 16. März 2018 11:29:11 CET Matthias Schiffer wrote:
> +       skb_push_rcsum(skb, hdr_size);

This function is only available since Linux 4.7. Compat code support must 
be added to avoid build problems with oler kernels. An example can be
found in 
https://git.open-mesh.org/batman-adv.git/commit/9f2b23430dec8caceedf996eb8ec75d3f8674144

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [B.A.T.M.A.N.] [PATCH maint 3/3] batman-adv: do not modify batadv packet header before pulling it

[Sven Eckelmann]

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

On Freitag, 16. März 2018 21:24:32 CET Sven Eckelmann wrote:
> On Freitag, 16. März 2018 11:29:11 CET Matthias Schiffer wrote:
> > +       skb_push_rcsum(skb, hdr_size);
> 
> This function is only available since Linux 4.7. Compat code support must 
> be added to avoid build problems with oler kernels. An example can be
> found in 
> https://git.open-mesh.org/batman-adv.git/commit/9f2b23430dec8caceedf996eb8ec75d3f8674144

Looks like the dependency skb_postpush_rcsum was added in 4.5. The
corrected version can be found in 
https://git.open-mesh.org/batman-adv.git/commit/9269709d9dabf13e70f1be2fe622278648f47017

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [B.A.T.M.A.N.] [PATCH maint 3/3] batman-adv: do not modify batadv packet header before pulling it

[Sven Eckelmann]

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

On Freitag, 16. März 2018 11:29:11 CET Matthias Schiffer wrote:
> batadv_check_unicast_ttvn() may modify the batadv header, leading to
> checksum errors in the following processing of the packet.
> 
> Rather than fixing up the checksum, simply pull the batadv header before
> modifying it (and push it back in case the packet is rerouted).
> 
> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
> ---

This patch is so invasive that it breaks the batadv_dbg_arp check which you've 
just fixed a patch before.

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [B.A.T.M.A.N.] [PATCH maint 3/3] batman-adv: do not modify batadv packet header before pulling it

[Matthias Schiffer]

[-- Attachment #1.1: Type: text/plain, Size: 1383 bytes --]

On 03/18/2018 09:15 AM, Sven Eckelmann wrote:
> On Freitag, 16. März 2018 11:29:11 CET Matthias Schiffer wrote:
>> batadv_check_unicast_ttvn() may modify the batadv header, leading to
>> checksum errors in the following processing of the packet.
>>
>> Rather than fixing up the checksum, simply pull the batadv header before
>> modifying it (and push it back in case the packet is rerouted).
>>
>> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
>> ---
> 
> This patch is so invasive that it breaks the batadv_dbg_arp check which you've 
> just fixed a patch before.
> 
> Kind regards,
> 	Sven
> 

Yes, it's the main reason I improved the check; I should have mentioned
that in the commit message. As mentioned on IRC, batadv_dbg_arp will still
print what information is added to the DAT cache, but it can't tell which
batadv packet type the ARP packet was encapsulated in anymore (this
information is still available through `batctl td`).

In my further cleanup patches, I plan to

* make the broadcast case match as well (pull before calling snooping
functions)
* move snooping calls to batadv_interface_rx
* remove header size argument from incoming snooping functions (will always
be 0)
* remove the now dead code from batadv_dbg_arp

This will improve symmetry between the incoming and outgoing snooping paths.

Matthias


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

Re: [B.A.T.M.A.N.] [PATCH maint 1/3] batman-adv: update data pointers after skb_cow()

[Sven Eckelmann]

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

On Freitag, 16. März 2018 11:29:09 CET Matthias Schiffer wrote:
> batadv_check_unicast_ttvn() calls skb_cow(), so pointers into the SKB data
> must be (re)set after calling it. The ethhdr variable is dropped
> altogether.
> 
> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>

Fixes: 78fc6bbe0aca ("batman-adv: add UNICAST_4ADDR packet type")

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [B.A.T.M.A.N.] [PATCH maint 1/3] batman-adv: update data pointers after skb_cow()

[Sven Eckelmann]

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

On Freitag, 16. März 2018 11:29:09 CET Matthias Schiffer wrote:
> batadv_check_unicast_ttvn() calls skb_cow(), so pointers into the SKB data
> must be (re)set after calling it. The ethhdr variable is dropped
> altogether.
> 
> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
> ---

The first two patches were applied in a155327b..7dfe729b [1]

Thanks,
	Sven

[1] https://git.open-mesh.org/batman-adv.git/commit/64d22c76a207ed313b2496f0709b2567719452c4
    https://git.open-mesh.org/batman-adv.git/commit/7dfe729b169b1217f47744edbd1616f473340fda

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]