netdev - Re: [net-next] seg6: using DSCP of inner IPv4 packets

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: <20200730.164424.85007408369570229.davem@davemloft.net>
Date:   Thu, 30 Jul 2020 16:44:24 -0700 (PDT)
From:   David Miller <davem@...emloft.net>
To:     ahabdels@...il.com
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

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.