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: Fri, 31 Jul 2020 19:31:20 +0200 From: Ahmed Abdelsalam <ahabdels@...il.com> To: David Miller <davem@...emloft.net> Cc: kuznet@....inr.ac.ru, yoshfuji@...ux-ipv6.org, kuba@...nel.org, linux-kernel@...r.kernel.org, netdev@...r.kernel.org, andrea.mayer@...roma2.it Subject: Re: [net-next] seg6: using DSCP of inner IPv4 packets I will refactor the code of this function and submit a new patch. Ahmed On 31/07/2020 01:44, David Miller wrote: > From: Ahmed Abdelsalam <ahabdels@...il.com> > Date: Tue, 28 Jul 2020 12:20:44 +0000 > >> This patch allows copying the DSCP from inner IPv4 header to the >> outer IPv6 header, when doing SRv6 Encapsulation. >> >> This allows forwarding packet across the SRv6 fabric based on their >> original traffic class. >> >> Signed-off-by: Ahmed Abdelsalam <ahabdels@...il.com> > > The conditionals in this function are now a mess. > >> - inner_hdr = ipv6_hdr(skb); >> + if (skb->protocol == htons(ETH_P_IPV6)) >> + inner_hdr = ipv6_hdr(skb); >> + else >> + inner_ipv4_hdr = ip_hdr(skb); >> + > > You assume that if skb->protocol is not ipv6 then it is ipv4. > >> @@ -138,6 +143,10 @@ int seg6_do_srh_encap(struct sk_buff *skb, struct ipv6_sr_hdr *osrh, int proto) >> ip6_flow_hdr(hdr, ip6_tclass(ip6_flowinfo(inner_hdr)), >> flowlabel); >> hdr->hop_limit = inner_hdr->hop_limit; >> + } else if (skb->protocol == htons(ETH_P_IP)) { >> + ip6_flow_hdr(hdr, inner_ipv4_hdr->tos, flowlabel); >> + hdr->hop_limit = inner_ipv4_hdr->ttl; >> + memset(IP6CB(skb), 0, sizeof(*IP6CB(skb))); >> } else { >> ip6_flow_hdr(hdr, 0, flowlabel); >> hdr->hop_limit = ip6_dst_hoplimit(skb_dst(skb)); > > But this code did not make that assumption at all. > > Only one of the two can be correct. > > The conditional assignment is also very ugly, you have two pointers > conditionally initialized. The compiler is going to have a hard time > figuring out that each pointer is only used in the code path where it > is guaranteed to be initialiazed. > > And it can't do that, as far as the compiler knows, skb->protocol can > change between those two locations. It MUST assume that can happen if > there are any functions calls whatsoever between these two code points. > > This function has to be sanitized, with better handling of access to > the inner protocol header values, before I am willing to apply this. >
Powered by blists - more mailing lists