netdev - Re: [net-next v2 1/1] net: seg6: Add support for SRv6 Headend Reduced Encapsulation

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]

Date:   Fri, 10 Jun 2022 13:59:58 +0200
From:   Andrea Mayer <andrea.mayer@...roma2.it>
To:     Anton Makarov <antonmakarov11235@...il.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        David Ahern <dsahern@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, david.lebrun@...ouvain.be,
        netdev@...r.kernel.org,
        Stefano Salsano <stefano.salsano@...roma2.it>,
        Paolo Lungaroni <paolo.lungaroni@...roma2.it>,
        Ahmed Abdelsalam <ahabdels.dev@...il.com>,
        Andrea Mayer <andrea.mayer@...roma2.it>
Subject: Re: [net-next v2 1/1] net: seg6: Add support for SRv6 Headend
 Reduced Encapsulation

Hi Anton,
please see my comments inline, thanks.

On Thu,  9 Jun 2022 16:27:50 +0300
Anton Makarov <antonmakarov11235@...il.com> wrote:

> SRv6 Headend H.Encaps.Red and H.Encaps.L2.Red behaviors are implemented
> accordingly to RFC 8986. The H.Encaps.Red is an optimization of
> The H.Encaps behavior. The H.Encaps.L2.Red is an optimization of
> the H.Encaps.L2 behavior. Both new behaviors reduce the length of
> the SRH by excluding the first SID in the SRH of the pushed IPv6 header.
> The first SID is only placed in the Destination Address field
> of the pushed IPv6 header.
> 
> The push of the SRH is omitted when the SRv6 Policy only contains
> one segment.
> 
> Signed-off-by: Anton Makarov <anton.makarov11235@...il.com>
> 
> ...
>  
> +/* encapsulate an IPv6 packet within an outer IPv6 header with reduced SRH */
> +int seg6_do_srh_encap_red(struct sk_buff *skb, struct ipv6_sr_hdr *osrh, int proto)
> +{
> +	struct dst_entry *dst = skb_dst(skb);
> +	struct net *net = dev_net(dst->dev);
> +	struct ipv6hdr *hdr, *inner_hdr6;
> +	struct iphdr *inner_hdr4;
> +	struct ipv6_sr_hdr *isrh;
> +	int hdrlen = 0, tot_len, err;

I suppose we should stick with the reverse XMAS tree code style.

> +	__be32 flowlabel = 0;

this initialization is unnecessary since the variable is accessed for the first
time in writing, later in the code.

> +	if (osrh->first_segment > 0)
> +		hdrlen = (osrh->hdrlen - 1) << 3;
> +
> +	tot_len = hdrlen + sizeof(struct ipv6hdr);
> +
> +	err = skb_cow_head(skb, tot_len + skb->mac_len);
> +	if (unlikely(err))
> +		return err;
> +
> +	inner_hdr6 = ipv6_hdr(skb);
> +	inner_hdr4 = ip_hdr(skb);

inner_hdr4 is only used in the *if* block that follows later on.

> +	flowlabel = seg6_make_flowlabel(net, skb, inner_hdr6);
> +
> +	skb_push(skb, tot_len);
> +	skb_reset_network_header(skb);
> +	skb_mac_header_rebuild(skb);
> +	hdr = ipv6_hdr(skb);
> +
> +	memset(skb->cb, 0, sizeof(skb->cb));

is there a specific reason why we should consider the whole CB size and not
only the part covered by the struct inet6_skb_parm?

> +	IP6CB(skb)->iif = skb->skb_iif;
> +
> +	if (skb->protocol == htons(ETH_P_IPV6)) {
> +		ip6_flow_hdr(hdr, ip6_tclass(ip6_flowinfo(inner_hdr6)),
> +			     flowlabel);
> +		hdr->hop_limit = inner_hdr6->hop_limit;
> +	} else if (skb->protocol == htons(ETH_P_IP)) {
> +		ip6_flow_hdr(hdr, (unsigned int) inner_hdr4->tos, flowlabel);
> +		hdr->hop_limit = inner_hdr4->ttl;
> +	}
> +

Don't IPv4 and IPv6 cover all possible cases?

> +	skb->protocol = htons(ETH_P_IPV6);
> +
> +	hdr->daddr = osrh->segments[osrh->first_segment];
> +	hdr->version = 6;
> +
> +	if (osrh->first_segment > 0) {
> +		hdr->nexthdr = NEXTHDR_ROUTING;
> +
> +		isrh = (void *)hdr + sizeof(struct ipv6hdr);
> +		memcpy(isrh, osrh, hdrlen);
> +
> +		isrh->nexthdr = proto;
> +		isrh->first_segment--;
> +		isrh->hdrlen -= 2;
> +	} else {
> +		hdr->nexthdr = proto;
> +	}
> +
> +	set_tun_src(net, dst->dev, &hdr->daddr, &hdr->saddr);
> +
> +#ifdef CONFIG_IPV6_SEG6_HMAC
> +	if (osrh->first_segment > 0 && sr_has_hmac(isrh)) {
> +		err = seg6_push_hmac(net, &hdr->saddr, isrh);
> +		if (unlikely(err))
> +			return err;
> +	}
> +#endif
> +

When there is only one SID and HMAC is configured, the SRH is not kept.
Aren't we losing information this way?

Andrea