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

Marek Lindner
2016-02-28T00:49:02Z

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