netdev - Re: [PATCH] bonding: L2L3 xmit doesn't support IPv6

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] [day] [month] [year] [list]

Message-ID: <4E960366.4060302@yahoo-inc.com>
Date:	Wed, 12 Oct 2011 14:15:18 -0700
From:	John Eaglesham <johneagl@...oo-inc.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
CC:	Yinglin Sun <yinglin.s@...il.com>,
	Andy Gospodarek <andy@...yhouse.net>,
	Jay Vosburgh <fubar@...ibm.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>, linux@...2.net
Subject: Re: [PATCH] bonding: L2L3 xmit doesn't support IPv6

Eric Dumazet wrote:
> Le mardi 11 octobre 2011 à 20:39 -0700, Yinglin Sun a écrit :
>> On Tue, Oct 11, 2011 at 7:51 PM, Andy Gospodarek <andy@...yhouse.net> wrote:
>>>        if (skb->protocol == htons(ETH_P_IP)) {
>>> +               struct iphdr *iph = ip_hdr(skb);
>>> +               __be16 *layer4hdr = (__be16 *)((u32 *)iph + iph->ihl);
>>>                if (!ip_is_fragment(iph) &&
>>>                    (iph->protocol == IPPROTO_TCP ||
>>>                     iph->protocol == IPPROTO_UDP)) {
>>> @@ -3398,7 +3407,18 @@ static int bond_xmit_hash_policy_l34(struct sk_buff *skb, int count)
>>>                }
>>>                return (layer4_xor ^
>>>                        ((ntohl(iph->saddr ^ iph->daddr)) & 0xffff)) % count;
>>> -
>>> +       } else if (skb->protocol == htons(ETH_P_IPV6)) {
>>> +               struct ipv6hdr *ipv6h = ipv6_hdr(skb);
>>> +               __be16 *layer4hdrv6 = (__be16 *)((u8 *)ipv6h + sizeof(*ipv6h));
>>> +               if (ipv6h->nexthdr == IPPROTO_TCP || ipv6h->nexthdr == IPPROTO_UDP) {
>> Does this work if this is a fragmentation packet? and if
>> ipv6h->nexthdr is not IPPROTO_TCP/IPPROTO_UDP, it doesn't mean this is
>> not TCP/UDP packet. We need to go through the extension header chain
>> and look at the last one. It's likely there are some other extension
>> headers before L4 header.
>>
> 
> Its a best effort.
> 
> If first header is not IPPROTO_TCP/UDP, I am not sure its wise to spend
> time in hope to find layer4 info (missing anyway in fragments)
> 
> By the way I see no bound checking on SKB head : malicious packet could
> make bond_xmit_hash_policy_l34() access unitialized memory.
> 
> We have same 'fastpath' in __skb_get_rxhash()
> 
> 

Thanks for keeping me in the loop on this.

I caught the bounds checking bug and added checks to code I haven't 
submitted in yet (I still need to test that it works before I submit a 
patch).

I don't like the idea of sticking a loop in this part of the code, 
especially one traversing a list of length controlled by outside users 
with no guarantee it is properly formed. I ran some tests gathering 
traffic and I never saw any packets with headers between the IPv6 and 
TCP or UDP, so at least in the real world right now this code should 
behave as expected for the vast majority of traffic. Things might change 
in the future, but if this code is clear/clean/simple and solves the 
problem today, I'd be happier with that than trying to parse a full IPv6 
header.

I'll test and post my revised patch (including bounds checking and 
documentation updates) in a day or so.

John
--
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