[B.A.T.M.A.N.] [PATCHv2] batman-adv: drop QinQ claim frames in bridge loop avoidance

[B.A.T.M.A.N.] [PATCHv2] batman-adv: drop QinQ claim frames in bridge loop avoidance

[Simon Wunderlich]

From: Simon Wunderlich <simon@open-mesh.com>

Since bridge loop avoidance only supports untagged or simple 802.1q
tagged VLAN claim frames, claim frames with stacked VLAN headers (QinQ)
should be detected and dropped. Transporting the over the mesh may cause
problems on the receivers, or create bogus entries in the local tt
tables.

Reported-by: Antonio Quartulli <antonio@open-mesh.com>
Signed-off-by: Simon Wunderlich <simon@open-mesh.com>
---
Changes to PATCHv1 (thanks Antonio):
 * add short description, fix capitalization for return
 * move drop debug message to batadv_bla_process_claim()
---
 bridge_loop_avoidance.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 85 insertions(+)

diff --git a/bridge_loop_avoidance.c b/bridge_loop_avoidance.c
index 6f0d9ec..9e7a5ab 100644
--- a/bridge_loop_avoidance.c
+++ b/bridge_loop_avoidance.c
@@ -851,6 +851,73 @@ static int batadv_check_claim_group(struct batadv_priv *bat_priv,
 	return 2;
 }
 
+/**
+ * batadv_is_encapsulated_claim - checks if a claim frame is encapsulated
+ * @bat_priv: the bat priv with all the soft interface information
+ * @skb: skb to be checked
+ *
+ * Check if this frame is a claim frame encapsulated in at least two layers
+ * of VLANs.
+ *
+ * Returns 1 if this is an encapsulated claim frame, 0 otherwise.
+ */
+static int batadv_is_encapsulated_claim(struct batadv_priv *bat_priv,
+					struct sk_buff *skb)
+{
+	struct batadv_bla_claim_dst *bla_dst, *bla_dst_own;
+	struct vlan_hdr vhdr, *vhdr_ptr;
+	uint8_t *hw_src, *hw_dst;
+	struct ethhdr *ethhdr;
+	struct arphdr *arphdr;
+	int headlen;
+
+	/* Traverse the VLAN/Ethertypes until an ARP header is found.
+	 *
+	 * At this point it is known that the first two protocols
+	 * are VLAN headers, so start checking at the encapsulated protocol
+	 * of the second header.
+	 */
+	headlen = VLAN_ETH_HLEN;
+	do {
+		vhdr_ptr = skb_header_pointer(skb, headlen, VLAN_HLEN, &vhdr);
+		if (!vhdr_ptr)
+			return 0;
+
+		headlen += VLAN_HLEN;
+	} while (vhdr_ptr->h_vlan_encapsulated_proto == htons(ETH_P_8021Q));
+
+	if (vhdr_ptr->h_vlan_encapsulated_proto != htons(ETH_P_ARP))
+		return 0;
+
+	if (unlikely(!pskb_may_pull(skb, headlen + arp_hdr_len(skb->dev))))
+		return 0;
+
+	/* pskb_may_pull() may have modified the pointers, get ethhdr again */
+	ethhdr = eth_hdr(skb);
+	arphdr = (struct arphdr *)((uint8_t *)ethhdr + headlen);
+
+	/* Check whether the ARP frame carries a valid IP information */
+	if (arphdr->ar_hrd != htons(ARPHRD_ETHER))
+		return 0;
+	if (arphdr->ar_pro != htons(ETH_P_IP))
+		return 0;
+	if (arphdr->ar_hln != ETH_ALEN)
+		return 0;
+	if (arphdr->ar_pln != 4)
+		return 0;
+
+	hw_src = (uint8_t *)arphdr + sizeof(struct arphdr);
+	hw_dst = hw_src + ETH_ALEN + 4;
+	bla_dst = (struct batadv_bla_claim_dst *)hw_dst;
+	bla_dst_own = &bat_priv->bla.claim_dest;
+
+	/* check if it is a claim frame in general */
+	if (memcmp(bla_dst->magic, bla_dst_own->magic,
+		   sizeof(bla_dst->magic)) != 0)
+		return 0;
+
+	return 1;
+}
 
 /**
  * batadv_bla_process_claim
@@ -885,6 +952,24 @@ static int batadv_bla_process_claim(struct batadv_priv *bat_priv,
 		vhdr = vlan_eth_hdr(skb);
 		proto = vhdr->h_vlan_encapsulated_proto;
 		headlen += VLAN_HLEN;
+
+		if (proto == htons(ETH_P_8021Q)) {
+			/* check if there is a claim frame encapsulated
+			 * deeper in (QinQ) and drop that, as this is
+			 * not supported by BLA but should also not be
+			 * sent via the mesh.
+			 *
+			 * if its not, let the frame pass without further
+			 * checks, as it is not a claim frame anyway.
+			 */
+			if (batadv_is_encapsulated_claim(bat_priv, skb)) {
+				batadv_dbg(BATADV_DBG_BLA, bat_priv,
+					   "Dropping encapsulated claim frame\n");
+				return 1;
+			} else {
+				return 0;
+			}
+		}
 	}
 
 	if (proto != htons(ETH_P_ARP))
-- 
1.9.1

Re: [B.A.T.M.A.N.] [PATCHv2] batman-adv: drop QinQ claim frames in bridge loop avoidance

[Marek Lindner]

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

On Friday 13 June 2014 11:15:50 Simon Wunderlich wrote:
> +/**
> + * batadv_is_encapsulated_claim - checks if a claim frame is encapsulated
> + * @bat_priv: the bat priv with all the soft interface information
> + * @skb: skb to be checked
> + *
> + * Check if this frame is a claim frame encapsulated in at least two layers
> + * of VLANs.

Maybe you mention QinQ here ? 'Two layers of VLANs' isn't that obvious 
(without the commit message).


> + * Returns 1 if this is an encapsulated claim frame, 0 otherwise.

Boolean functions should return bool instead of 0 / 1.


> +	 * At this point it is known that the first two protocols
> +	 * are VLAN headers, so start checking at the encapsulated protocol
> +	 * of the second header.

The first two protocols ? Are you trying to say that at this point we already 
know we are having QinQ ?


> +	 */
> +	headlen = VLAN_ETH_HLEN;
> +	do {
> +		vhdr_ptr = skb_header_pointer(skb, headlen, VLAN_HLEN, &vhdr);
> +		if (!vhdr_ptr)
> +			return 0;
> +
> +		headlen += VLAN_HLEN;
> +	} while (vhdr_ptr->h_vlan_encapsulated_proto == htons(ETH_P_8021Q));

Is there a length check somewhere in skb_header_pointer() ? We wouldn't want 
to read more than we have in the skb, right ?


> +	if (vhdr_ptr->h_vlan_encapsulated_proto != htons(ETH_P_ARP))
> +		return 0;
> +
> +	if (unlikely(!pskb_may_pull(skb, headlen + arp_hdr_len(skb->dev))))
> +		return 0;
> +
> +	/* pskb_may_pull() may have modified the pointers, get ethhdr again */
> +	ethhdr = eth_hdr(skb);
> +	arphdr = (struct arphdr *)((uint8_t *)ethhdr + headlen);
> +
> +	/* Check whether the ARP frame carries a valid IP information */
> +	if (arphdr->ar_hrd != htons(ARPHRD_ETHER))
> +		return 0;
> +	if (arphdr->ar_pro != htons(ETH_P_IP))
> +		return 0;
> +	if (arphdr->ar_hln != ETH_ALEN)
> +		return 0;
> +	if (arphdr->ar_pln != 4)
> +		return 0;
> +
> +	hw_src = (uint8_t *)arphdr + sizeof(struct arphdr);
> +	hw_dst = hw_src + ETH_ALEN + 4;
> +	bla_dst = (struct batadv_bla_claim_dst *)hw_dst;
> +	bla_dst_own = &bat_priv->bla.claim_dest;

Looks like copy & paste from batadv_bla_process_claim() ? How about using a 
shared function ?

Cheers,
Marek

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

Re: [B.A.T.M.A.N.] [PATCHv2] batman-adv: drop QinQ claim frames in bridge loop avoidance

[Simon Wunderlich]

> On Friday 13 June 2014 11:15:50 Simon Wunderlich wrote:
> > +/**
> > + * batadv_is_encapsulated_claim - checks if a claim frame is
> > encapsulated + * @bat_priv: the bat priv with all the soft interface
> > information + * @skb: skb to be checked
> > + *
> > + * Check if this frame is a claim frame encapsulated in at least two
> > layers + * of VLANs.
> 
> Maybe you mention QinQ here ? 'Two layers of VLANs' isn't that obvious
> (without the commit message).

OK, sure.

> 
> > + * Returns 1 if this is an encapsulated claim frame, 0 otherwise.
> 
> Boolean functions should return bool instead of 0 / 1.

OK.

> 
> > +	 * At this point it is known that the first two protocols
> > +	 * are VLAN headers, so start checking at the encapsulated protocol
> > +	 * of the second header.
> 
> The first two protocols ? Are you trying to say that at this point we
> already know we are having QinQ ?
> 

Yep. I can make that more clear.

> > +	 */
> > +	headlen = VLAN_ETH_HLEN;
> > +	do {
> > +		vhdr_ptr = skb_header_pointer(skb, headlen, VLAN_HLEN, &vhdr);
> > +		if (!vhdr_ptr)
> > +			return 0;
> > +
> > +		headlen += VLAN_HLEN;
> > +	} while (vhdr_ptr->h_vlan_encapsulated_proto == htons(ETH_P_8021Q));
> 
> Is there a length check somewhere in skb_header_pointer() ? We wouldn't
> want to read more than we have in the skb, right ?
> 

Yes, skb_header_pointer (or also skb_copy_bits() which is called by that) 
checks the length, so we don't have to do that on our own.

> > +	if (vhdr_ptr->h_vlan_encapsulated_proto != htons(ETH_P_ARP))
> > +		return 0;
> > +
> > +	if (unlikely(!pskb_may_pull(skb, headlen + arp_hdr_len(skb->dev))))
> > +		return 0;
> > +
> > +	/* pskb_may_pull() may have modified the pointers, get ethhdr again */
> > +	ethhdr = eth_hdr(skb);
> > +	arphdr = (struct arphdr *)((uint8_t *)ethhdr + headlen);
> > +
> > +	/* Check whether the ARP frame carries a valid IP information */
> > +	if (arphdr->ar_hrd != htons(ARPHRD_ETHER))
> > +		return 0;
> > +	if (arphdr->ar_pro != htons(ETH_P_IP))
> > +		return 0;
> > +	if (arphdr->ar_hln != ETH_ALEN)
> > +		return 0;
> > +	if (arphdr->ar_pln != 4)
> > +		return 0;
> > +
> > +	hw_src = (uint8_t *)arphdr + sizeof(struct arphdr);
> > +	hw_dst = hw_src + ETH_ALEN + 4;
> > +	bla_dst = (struct batadv_bla_claim_dst *)hw_dst;
> > +	bla_dst_own = &bat_priv->bla.claim_dest;
> 
> Looks like copy & paste from batadv_bla_process_claim() ? How about using a
> shared function ?

Yeah, it's copy and pasted. The only thing which we could put into a common 
function are the 4 checks, the local variables are used in a different way for 
the further code in each function. This didn't really look worth it so I 
didn't bother to refactor in that patch, if you feel otherwise let me know and 
i'll refactor in v2.

Thanks!
    Simon

Re: [B.A.T.M.A.N.] [PATCHv2] batman-adv: drop QinQ claim frames in bridge loop avoidance

[Marek Lindner]

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

On Saturday 21 June 2014 16:57:42 Simon Wunderlich wrote:
> > Looks like copy & paste from batadv_bla_process_claim() ? How about using
> > a
> > shared function ?
> 
> Yeah, it's copy and pasted. The only thing which we could put into a common 
> function are the 4 checks, the local variables are used in a different way
> for the further code in each function. This didn't really look worth it so
> I didn't bother to refactor in that patch, if you feel otherwise let me
> know and i'll refactor in v2.

How about moving the while loop into batadv_bla_process_claim() itself ?

Something like:

	if (vid & BATADV_VLAN_HAS_TAG) {
		vhdr = vlan_eth_hdr(skb);
		proto = vhdr->h_vlan_encapsulated_proto;
		headlen += VLAN_HLEN;
	}

	if (proto == htons(ETH_P_8021Q)) {
		do {
			vhdr_ptr = skb_header_pointer(skb, headlen, VLAN_HLEN, &vhdr);
			if (!vhdr_ptr)
				return 0;

			headlen += VLAN_HLEN;

		} while (vhdr_ptr->h_vlan_encapsulated_proto == htons(ETH_P_8021Q));

		proto = vhdr->h_vlan_encapsulated_proto;
	}

	if (proto != htons(ETH_P_ARP))
		return 0; /* not a claim frame */

Towards the BLA claim check you can then check whether this BLA frame was 
encapsulated more than once (through a local variable).

Cheers,
Marek

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