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

Sven Eckelmann
2016-02-28T06:36:40Z

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