| Message ID | 1456505773-1059-1-git-send-email-sven@narfation.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Commit | 7d2f8a773bae5e07cadc9a9f74ac21abe32cde13 |
| Delegated to: | Marek Lindner |
| Headers |
Return-Path: <b.a.t.m.a.n-bounces@lists.open-mesh.org> X-Original-To: patchwork@open-mesh.org Delivered-To: patchwork@open-mesh.org Received: from open-mesh.org (localhost [127.0.0.1]) by open-mesh.org (Postfix) with ESMTP id 1AA2E806A6; Fri, 26 Feb 2016 17:56:21 +0100 (CET) Authentication-Results: open-mesh.org; dkim=fail reason="verification failed; unprotected key" header.d=narfation.org header.i=@narfation.org header.b=n5Pas4h8; dkim-adsp=fail (unprotected policy); dkim-atps=neutral Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=79.140.41.39; helo=v3-1039.vlinux.de; envelope-from=sven@narfation.org; receiver=b.a.t.m.a.n@lists.open-mesh.org Authentication-Results: open-mesh.org; dmarc=pass header.from=narfation.org Received: from v3-1039.vlinux.de (narfation.org [79.140.41.39]) by open-mesh.org (Postfix) with ESMTPS id 995278058E for <b.a.t.m.a.n@lists.open-mesh.org>; Fri, 26 Feb 2016 17:56:19 +0100 (CET) Received: from sven-desktop.home.narfation.org (x4d05f02d.dyn.telefonica.de [77.5.240.45]) by v3-1039.vlinux.de (Postfix) with ESMTPSA id 277171100E6; Fri, 26 Feb 2016 17:56:19 +0100 (CET) Authentication-Results: v3-1039.vlinux.de; dmarc=none header.from=narfation.org DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=narfation.org; s=20121; t=1456505779; bh=Y2+b2J6sP/9l6S1roR+WbOQ4d/MdLMYt7p6FlF7M+OQ=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=n5Pas4h8VFd1cGr5SIaHCOVPVLPPkj1gRNAtHlVKfSSfxj7Sj2gODD9HTroWruxfi KQ0sdKDN+GC8lYyl3Gp0ZVch3TusOfpg21mgSNA7wWuQxR0Xmy8caInj6/SWEzsWDh 31FgTL5yc+2d2R6WKTdr3/wch3Ggbcm1mJLLDJIc= From: Sven Eckelmann <sven@narfation.org> To: b.a.t.m.a.n@lists.open-mesh.org Date: Fri, 26 Feb 2016 17:56:13 +0100 Message-Id: <1456505773-1059-1-git-send-email-sven@narfation.org> X-Mailer: git-send-email 2.7.0 In-Reply-To: <b.a.t.m.a.n@lists.open-mesh.org> References: <b.a.t.m.a.n@lists.open-mesh.org> Subject: [B.A.T.M.A.N.] [PATCH] batman-adv: Check skb size before using encapsulated ETH+VLAN header X-BeenThere: b.a.t.m.a.n@lists.open-mesh.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: The list for a Better Approach To Mobile Ad-hoc Networking <b.a.t.m.a.n.lists.open-mesh.org> List-Unsubscribe: <https://lists.open-mesh.org/mm/options/b.a.t.m.a.n>, <mailto:b.a.t.m.a.n-request@lists.open-mesh.org?subject=unsubscribe> List-Archive: <http://lists.open-mesh.org/pipermail/b.a.t.m.a.n/> List-Post: <mailto:b.a.t.m.a.n@lists.open-mesh.org> List-Help: <mailto:b.a.t.m.a.n-request@lists.open-mesh.org?subject=help> List-Subscribe: <https://lists.open-mesh.org/mm/listinfo/b.a.t.m.a.n>, <mailto:b.a.t.m.a.n-request@lists.open-mesh.org?subject=subscribe> Reply-To: The list for a Better Approach To Mobile Ad-hoc Networking <b.a.t.m.a.n@lists.open-mesh.org> Errors-To: b.a.t.m.a.n-bounces@lists.open-mesh.org Sender: "B.A.T.M.A.N" <b.a.t.m.a.n-bounces@lists.open-mesh.org> |
Commit Message
Sven Eckelmann
Feb. 26, 2016, 4:56 p.m. UTC
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(-)
Comments
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
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()