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