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
| ||
|
Date: Thu, 8 Oct 2020 11:47:00 +0200 From: Eric Dumazet <eric.dumazet@...il.com> To: Hangbin Liu <liuhangbin@...il.com> Cc: netdev@...r.kernel.org, "David S . Miller" <davem@...emloft.net>, Alexey Kuznetsov <kuznet@....inr.ac.ru>, Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>, Jakub Kicinski <kuba@...nel.org>, Willem de Bruijn <willemb@...gle.com> Subject: Re: [PATCH net 2/2] IPv6: reply ICMP error if the first fragment don't include all headers On 10/8/20 10:30 AM, Hangbin Liu wrote: > Hi Eric, > > Thanks for the comments. I should add "RFC" in subject next time for the > uncertain fix patch. > > On Wed, Oct 07, 2020 at 11:35:41AM +0200, Eric Dumazet wrote: >> >> >> On 10/7/20 5:55 AM, Hangbin Liu wrote: >> >>> kfree_skb(skb); >>> @@ -282,6 +285,21 @@ static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev, >>> } >>> } >>> >>> + /* RFC 8200, Section 4.5 Fragment Header: >>> + * If the first fragment does not include all headers through an >>> + * Upper-Layer header, then that fragment should be discarded and >>> + * an ICMP Parameter Problem, Code 3, message should be sent to >>> + * the source of the fragment, with the Pointer field set to zero. >>> + */ >>> + nexthdr = hdr->nexthdr; >>> + offset = ipv6_skip_exthdr(skb, skb_transport_offset(skb), &nexthdr, &frag_off); >>> + if (frag_off == htons(IP6_MF) && !pskb_may_pull(skb, offset + 1)) { >>> + __IP6_INC_STATS(net, idev, IPSTATS_MIB_INHDRERRORS); >>> + icmpv6_param_prob(skb, ICMPV6_HDR_INCOMP, 0); >>> + rcu_read_unlock(); >>> + return NULL; >>> + } >>> + >>> rcu_read_unlock(); >>> >>> /* Must drop socket now because of tproxy. */ >>> >> >> Ouch, this is quite a buggy patch. >> >> I doubt we want to add yet another ipv6_skip_exthdr() call in IPv6 fast path. >> >> Surely the presence of NEXTHDR_FRAGMENT is already tested elsewhere ? > > Would you like to help point where NEXTHDR_FRAGMENT was tested before IPv6 > defragment? I think we have to ask the question : Should routers enforce the rule, or only end points ? End points must handle NEXTHDR_FRAGMENT, in ipv6_frag_rcv() > >> >> Also, ipv6_skip_exthdr() does not pull anything in skb->head, it would be strange >> to force a pull of hundreds of bytes just because you want to check if an extra byte is there, >> if the packet could be forwarded as is, without additional memory allocations. >> >> Testing skb->len should be more than enough at this stage. > > Ah, yes, I shouldn't call pskb_may_pull here. >> >> Also ipv6_skip_exthdr() can return an error. > > it returns -1 as error, If we tested it by (offset + 1 > skb->len), does > that count as an error handler? If there is an error, then you want to send the ICMP, right ? The (offset + 1) expression will become 0, and surely the test will be false, so you wont send the ICMP... > > Thanks > Hangbin >
Powered by blists - more mailing lists