Return-path: Received: from mail.vyatta.com ([76.74.103.46]:44422 "EHLO mail.vyatta.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752049Ab0L3TGO (ORCPT ); Thu, 30 Dec 2010 14:06:14 -0500 Date: Thu, 30 Dec 2010 11:06:09 -0800 From: Stephen Hemminger To: Johannes Berg Cc: Tomas Winkler , davem@davemloft.net, netdev@vger.kernel.org, linux-wireless@vger.kernel.org Subject: Re: [PATCH net-2.6] bridge: fix br_multicast_ipv6_rcv for paged skbs Message-ID: <20101230110609.08b7ee38@nehalam> In-Reply-To: <1293735134.21956.1.camel@jlt3.sipsolutions.net> References: <1293708753-17728-1-git-send-email-tomas.winkler@intel.com> <20101230104656.6c5a4b4e@nehalam> <1293735134.21956.1.camel@jlt3.sipsolutions.net> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-wireless-owner@vger.kernel.org List-ID: On Thu, 30 Dec 2010 19:52:14 +0100 Johannes Berg wrote: > On Thu, 2010-12-30 at 10:46 -0800, Stephen Hemminger wrote: > > > This doesn't look correct. The calculation of the offset doesn't look correct. > > Just following the skb_clone(), the skb_pull value is "offset". > > Also, the other checks return -EINVAL for incorrectly formed packet. > > > > --- a/net/bridge/br_multicast.c 2010-12-30 10:29:58.579510488 -0800 > > +++ b/net/bridge/br_multicast.c 2010-12-30 10:43:27.273386691 -0800 > > @@ -1464,6 +1464,9 @@ static int br_multicast_ipv6_rcv(struct > > if (offset < 0 || nexthdr != IPPROTO_ICMPV6) > > return 0; > > > > + if (!pskb_may_pull(skb, offset)) > > + return -EINVAL; > > + > > /* Okay, we found ICMPv6 header */ > > skb2 = skb_clone(skb, GFP_ATOMIC); > > if (!skb2) > > Wouldn't that make more sense after the clone anyway? But if you look at > my email, you'll find that there's potentially, and conditionally, more > stuff that will be read from the skb's header, which hasn't necessarily > been pulled in, so I think this still won't fix all the issues. > > Seeing how this only affects some ICMPv6 packets, maybe we should just > use skb_copy() instead? It comes out cleaner, and the check can be simplified. --- a/net/bridge/br_multicast.c 2010-12-30 10:47:12.031733855 -0800 +++ b/net/bridge/br_multicast.c 2010-12-30 11:00:12.135801266 -0800 @@ -1465,19 +1465,19 @@ static int br_multicast_ipv6_rcv(struct return 0; /* Okay, we found ICMPv6 header */ - skb2 = skb_clone(skb, GFP_ATOMIC); + skb2 = skb_copy(skb, GFP_ATOMIC); if (!skb2) return -ENOMEM; + err = -EINVAL; + if (skb2->len < offset + sizeof(*icmp6h)) + goto out; + len -= offset - skb_network_offset(skb2); __skb_pull(skb2, offset); skb_reset_transport_header(skb2); - err = -EINVAL; - if (!pskb_may_pull(skb2, sizeof(*icmp6h))) - goto out; - icmp6h = icmp6_hdr(skb2); switch (icmp6h->icmp6_type) { --