[B.A.T.M.A.N.] [PATCH] batman-adv: Check skb size before using encapsulated ETH+VLAN header

[B.A.T.M.A.N.] [PATCH] batman-adv: Check skb size before using encapsulated ETH+VLAN header

[Sven Eckelmann]

The encapsulated ethernet and VLAN header may be outside the received
ethernet frame. Thus the skb buffer size has to be checked before it can be
parsed to find out if it encapsulates another batman-adv packet.

Fixes: 48628bb9419f ("batman-adv: softif bridge loop avoidance")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 net/batman-adv/soft-interface.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/batman-adv/soft-interface.c b/net/batman-adv/soft-interface.c
index 0710379..8a136b6 100644
--- a/net/batman-adv/soft-interface.c
+++ b/net/batman-adv/soft-interface.c
@@ -408,11 +408,17 @@ void batadv_interface_rx(struct net_device *soft_iface,
 	 */
 	nf_reset(skb);
 
+	if (unlikely(!pskb_may_pull(skb, ETH_HLEN)))
+		goto dropped;
+
 	vid = batadv_get_vid(skb, 0);
 	ethhdr = eth_hdr(skb);
 
 	switch (ntohs(ethhdr->h_proto)) {
 	case ETH_P_8021Q:
+		if (!pskb_may_pull(skb, VLAN_ETH_HLEN))
+			goto dropped;
+
 		vhdr = (struct vlan_ethhdr *)skb->data;
 
 		if (vhdr->h_vlan_encapsulated_proto != ethertype)
@@ -424,8 +430,6 @@ void batadv_interface_rx(struct net_device *soft_iface,
 	}
 
 	/* skb->dev & skb->pkt_type are set here */
-	if (unlikely(!pskb_may_pull(skb, ETH_HLEN)))
-		goto dropped;
 	skb->protocol = eth_type_trans(skb, soft_iface);
 
 	/* should not be necessary anymore as we use skb_pull_rcsum()
-- 
2.7.0

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: Check skb size before using encapsulated ETH+VLAN header

[Marek Lindner]

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

On Friday, February 26, 2016 17:56:13 Sven Eckelmann wrote:
> --- a/net/batman-adv/soft-interface.c
> +++ b/net/batman-adv/soft-interface.c
> @@ -408,11 +408,17 @@ void batadv_interface_rx(struct net_device
> *soft_iface, */
>  	nf_reset(skb);
> 
> +	if (unlikely(!pskb_may_pull(skb, ETH_HLEN)))
> +		goto dropped;
> +
>  	vid = batadv_get_vid(skb, 0);

batadv_get_vid() also calls pskb_may_pull() and checks for VLAN_ETH_HLEN 
length. Isn't that sufficient ?

On a related note - a few lines before your check you'll find:

/* check if enough space is available for pulling, and pull */
if (!pskb_may_pull(skb, hdr_size))

In its current form that check is useless because batadv_recv_unicast_packet() 
already calls batadv_check_unicast_packet() which does the same 
pskb_may_pull(skb, hdr_size). Am I overlooking something ?


>  	switch (ntohs(ethhdr->h_proto)) {
>  	case ETH_P_8021Q:
> +		if (!pskb_may_pull(skb, VLAN_ETH_HLEN))
> +			goto dropped;

Shouldn't this memory access be covered by the earlier check inside 
batadv_get_vid() ?


>  	/* skb->dev & skb->pkt_type are set here */
> -	if (unlikely(!pskb_may_pull(skb, ETH_HLEN)))
> -		goto dropped;

Agreed that this seems unnecessary.

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.] [PATCH] batman-adv: Check skb size before using encapsulated ETH+VLAN header

[Sven Eckelmann]

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

On Sunday 28 February 2016 08:49:02 Marek Lindner wrote:
> On Friday, February 26, 2016 17:56:13 Sven Eckelmann wrote:
> > --- a/net/batman-adv/soft-interface.c
> > +++ b/net/batman-adv/soft-interface.c
> > @@ -408,11 +408,17 @@ void batadv_interface_rx(struct net_device
> > *soft_iface, */
> > 
> >  	nf_reset(skb);
> > 
> > +	if (unlikely(!pskb_may_pull(skb, ETH_HLEN)))
> > +		goto dropped;
> > +
> > 
> >  	vid = batadv_get_vid(skb, 0);
> 
> batadv_get_vid() also calls pskb_may_pull() and checks for VLAN_ETH_HLEN
> length. Isn't that sufficient ?

No, because it doesn't signal the result of pskb_may_pull back to this 
function. At least not in a way that the _rx function drops the packet on an 
error.

> On a related note - a few lines before your check you'll find:
> 
> /* check if enough space is available for pulling, and pull */
> if (!pskb_may_pull(skb, hdr_size))

This only includes the the unicast/unicast_4addr/bcast batman-adv header. It 
doesn't check the size of the encapsulated data (this also means *not* the 
encapsulated ethernet header)

> 
> In its current form that check is useless because
> batadv_recv_unicast_packet() already calls batadv_check_unicast_packet()
> which does the same
> pskb_may_pull(skb, hdr_size). Am I overlooking something ?

Looks like it also only checks the size of the batman-adv header and the 
content of the outer (not the encapsulated) ethernet header.

> 
> >  	switch (ntohs(ethhdr->h_proto)) {
> > 
> >  	case ETH_P_8021Q:
> > +		if (!pskb_may_pull(skb, VLAN_ETH_HLEN))
> > +			goto dropped;
> 
> Shouldn't this memory access be covered by the earlier check inside
> batadv_get_vid() ?

Nope, no drop of the packet when the may_pull in batadv_get_vid fails.

> 
> >  	/* skb->dev & skb->pkt_type are set here */
> > 
> > -	if (unlikely(!pskb_may_pull(skb, ETH_HLEN)))
> > -		goto dropped;
> 
> Agreed that this seems unnecessary.


At least it is too late :)

Please check my statements twice. Maybe I've overlooked something.

Kind regards,
	Sven

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

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: Check skb size before using encapsulated ETH+VLAN header

[Antonio Quartulli]

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

On Sun, Feb 28, 2016 at 07:36:40AM +0100, Sven Eckelmann wrote:
> On Sunday 28 February 2016 08:49:02 Marek Lindner wrote:
> > On Friday, February 26, 2016 17:56:13 Sven Eckelmann wrote:
> > 
> > In its current form that check is useless because
> > batadv_recv_unicast_packet() already calls batadv_check_unicast_packet()
> > which does the same
> > pskb_may_pull(skb, hdr_size). Am I overlooking something ?
> 
> Looks like it also only checks the size of the batman-adv header and the 
> content of the outer (not the encapsulated) ethernet header.

I think you are both right here :-) in batadv_check_unicast_packet() we
perform a may_pull with the same hdr_size, therefore it here once again
is not useful. It is not really related to this patch, but I think that
the removal of the useless pskb_may_pull() in interface_rx() could be
done here since you are cleaning up all the pulls in this function.

Cheers,

-- 
Antonio Quartulli

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

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: Check skb size before using encapsulated ETH+VLAN header

[Sven Eckelmann]

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

On Sunday 28 February 2016 17:02:27 Antonio Quartulli wrote:
> On Sun, Feb 28, 2016 at 07:36:40AM +0100, Sven Eckelmann wrote:
> > On Sunday 28 February 2016 08:49:02 Marek Lindner wrote:
> > > On Friday, February 26, 2016 17:56:13 Sven Eckelmann wrote:
> > > 
> > > In its current form that check is useless because
> > > batadv_recv_unicast_packet() already calls batadv_check_unicast_packet()
> > > which does the same
> > > pskb_may_pull(skb, hdr_size). Am I overlooking something ?
> > 
> > Looks like it also only checks the size of the batman-adv header and the
> > content of the outer (not the encapsulated) ethernet header.
> 
> I think you are both right here :-) in batadv_check_unicast_packet() we
> perform a may_pull with the same hdr_size, therefore it here once again
> is not useful. It is not really related to this patch, but I think that
> the removal of the useless pskb_may_pull() in interface_rx() could be
> done here since you are cleaning up all the pulls in this function.

Ok, I misunderstood his comment. This one is not necessary when each path to 
this function uses batadv_check_unicast_packet or does "if 
(!pskb_may_pull(skb, hdr_size))" directly. The only callers are 
batadv_recv_bcast_packet (does "if (unlikely(!pskb_may_pull(skb, hdr_size)))") 
and batadv_recv_unicast_packet (calls batadv_check_unicast_packet). I would 
say that an extra patch for that can be introduced later to remove it. But it 
should not be part of this patch because this fix should not contain cleanup 
stuff ("batman-adv header size check") which is not related to the 
encapsulated ethernet (vlan) header.

Kind regards,
	Sven

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

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: Check skb size before using encapsulated ETH+VLAN header

[Antonio Quartulli]

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

On Sun, Feb 28, 2016 at 10:20:30AM +0100, Sven Eckelmann wrote:
> Ok, I misunderstood his comment. This one is not necessary when each path to 
> this function uses batadv_check_unicast_packet or does "if 
> (!pskb_may_pull(skb, hdr_size))" directly. The only callers are 
> batadv_recv_bcast_packet (does "if (unlikely(!pskb_may_pull(skb, hdr_size)))") 
> and batadv_recv_unicast_packet (calls batadv_check_unicast_packet). I would 
> say that an extra patch for that can be introduced later to remove it. But it 
> should not be part of this patch because this fix should not contain cleanup 
> stuff ("batman-adv header size check") which is not related to the 
> encapsulated ethernet (vlan) header.

True.
Better to keep the fix small and address that in a later patch.

Cheers,

-- 
Antonio Quartulli

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

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: Check skb size before using encapsulated ETH+VLAN header

[Marek Lindner]

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

On Friday, February 26, 2016 17:56:13 Sven Eckelmann wrote:
> The encapsulated ethernet and VLAN header may be outside the received
> ethernet frame. Thus the skb buffer size has to be checked before it can be
> parsed to find out if it encapsulates another batman-adv packet.
> 
> Fixes: 48628bb9419f ("batman-adv: softif bridge loop avoidance")
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> ---
>  net/batman-adv/soft-interface.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)

Applied in revision 7d2f8a7.

Thanks,
Marek

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