[B.A.T.M.A.N.] [PATCH] batman-adv: Check skb size before using encapsulated ETH+VLAN header
The encapsulated ethernet and VLAN header may be outside the received ethernet frame. Thus the skb buffer size has to be checked before it can be parsed to find out if it encapsulates another batman-adv packet.
Fixes: 48628bb9419f ("batman-adv: softif bridge loop avoidance") Signed-off-by: Sven Eckelmann sven@narfation.org --- net/batman-adv/soft-interface.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/net/batman-adv/soft-interface.c b/net/batman-adv/soft-interface.c index 0710379..8a136b6 100644 --- 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); ethhdr = eth_hdr(skb);
switch (ntohs(ethhdr->h_proto)) { case ETH_P_8021Q: + if (!pskb_may_pull(skb, VLAN_ETH_HLEN)) + goto dropped; + vhdr = (struct vlan_ethhdr *)skb->data;
if (vhdr->h_vlan_encapsulated_proto != ethertype) @@ -424,8 +430,6 @@ void batadv_interface_rx(struct net_device *soft_iface, }
/* skb->dev & skb->pkt_type are set here */ - if (unlikely(!pskb_may_pull(skb, ETH_HLEN))) - goto dropped; skb->protocol = eth_type_trans(skb, soft_iface);
/* should not be necessary anymore as we use skb_pull_rcsum()
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
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
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.
Cheers,
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
On Sun, Feb 28, 2016 at 10:20:30AM +0100, Sven Eckelmann wrote:
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.
True. Better to keep the fix small and address that in a later patch.
Cheers,
On Friday, February 26, 2016 17:56:13 Sven Eckelmann wrote:
The encapsulated ethernet and VLAN header may be outside the received ethernet frame. Thus the skb buffer size has to be checked before it can be parsed to find out if it encapsulates another batman-adv packet.
Fixes: 48628bb9419f ("batman-adv: softif bridge loop avoidance") Signed-off-by: Sven Eckelmann sven@narfation.org
net/batman-adv/soft-interface.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
Applied in revision 7d2f8a7.
Thanks, Marek
b.a.t.m.a.n@lists.open-mesh.org
-
Antonio Quartulli -
Marek Lindner -
Sven Eckelmann