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

Sven Eckelmann
2016-02-28T09:20:30Z

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