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

[Marek Lindner <mareklindner@neomailbox.ch>]

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