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

[Sven Eckelmann <sven@narfation.org>]

[-- 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 --]