netdev - Re: [PATCHv3] netns: oops in ip[6]_frag

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20090318.232651.99085030.davem@davemloft.net>
Date:	Wed, 18 Mar 2009 23:26:51 -0700 (PDT)
From:	David Miller <davem@...emloft.net>
To:	jarkao2@...il.com
Cc:	jorge@...2.net, netdev@...r.kernel.org
Subject: Re: [PATCHv3] netns: oops in ip[6]_frag_reasm incrementing stats

From: Jarek Poplawski <jarkao2@...il.com>
Date: Wed, 18 Mar 2009 07:26:58 +0000

> On 17-03-2009 14:54, Jorge Boncompte [DTI2] wrote:
> > Jarek Poplawski escribió:
> >> On Tue, Mar 17, 2009 at 12:55:42PM +0100, Jorge Boncompte [DTI2] wrote:
> >>> dev can be NULL in ip[6]_frag_reasm for skb's coming from RAW sockets.
> >>>
> >>> Quagga's OSPFD sends fragmented packets on a RAW socket, when netfilter
> >>> conntrack reassembles them on the OUTPUT path you hit this code path.
> >>>
> >>> You can test it with something like "hping2 -0 -d 2000 -f AA.BB.CC.DD"
> >>>
> >>> Changes from v2: (address comments from Jarek Poplawski)
> >>> 	- Patch reworked to get the net pointer with container_of()
> >>> 	  instead of passing it to function calls.
> >>> 	- Fix IPv6 code
> >>> Changes from v1:
> >>> 	- Fixed description
> >> I guess David will be interested only with the final state of changes,
> >> so v1 & v2 are not necessary here...
> >>
> >> Anyway, ipv4 looks OK to me, but ipv6 looks like something is
> >> different:
> >>> +	IP6_INC_STATS_BH(net, __in6_dev_get(dev), IPSTATS_MIB_REASMFAILS);
> >> It still depends on dev != NULL in __in6_dev_get(). I see there
> >> is also used skb->dst for similar things in ip6_frag_queue(), so I
> >> don't know: it needs rethinking, and maybe these patches should be
> >> separated if you prefer.
> > 
> > 	Not my day! :-) I should not look at code at 2 am and write patches
> > the day after, I confused _idev and idev in the check for != NULL in _DEVINC.
> > 
> > 	I think this bug was first introduced by patch "[IPV6]: Per-interface
> > statistics support." from YOSHIFUJI Hideaki on Nov 4, 2006.
> > 
> > 	If someone with more knowledge could confirm that using something like...
> > 
> > "(skb->dev ? skb->dev : skb->dst->dev)"
> > 
> > ... here is fine I'll redo this part and resend. I do not have an IPv6 setup where
> > I can test this.
> > 
> 
> skb->dst->dev is used in ip6_frag_queue anyway, so it shouldn't be
> worse if you do this similarly in ip6_frag_reasm. If you send it as
> a separate patch, and write it's not tested, David will decide if he
> wants it. Otherwise you can resend this ipv4 patch only.

Netfilter on ipv6 handles reassembly differently and in a way
that won't result in dev being NULL here.

I've applied Jorge's patch, thanks everyone.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html