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:   Sat, 11 Jun 2022 20:23:35 +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>, 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 consider my answers below. Thanks.

On Fri, 10 Jun 2022 21:21:08 +0300
Anton Makarov <antonmakarov11235@...il.com> wrote:

> Hi Andrea,
> Thank you very much for your feedback! Plese look at my response inline
> and let me know what you think about that. Many thanks!
> 

you are welcome!

> 
> > 
> > > +	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.
> 
> Do you mean it has to be defined inside *if* block and assigned via
> inner_ip_hdr()?
> 

I think it is correct to define inner_hdr4 as you have already done, but
initialize and use it only within the *else if* block (the IPv4 one, of
course), as it is no further accessed outside.

> > > +	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?
> 
> In fact while case SEG6_IPTUN_MODE_ENCAP in seg6_do_srh() does
> preliminary check of protocol value, case SEG6_IPTUN_MODE_L2ENCAP does
> not. So potentially skb->protocol can be of any value. Although
> additional check brings extra impact on performance, sure.
> 

If you fill the pushed IPv6 header in the same way as is done in
seg6_do_srh_encap(), I don't think you need to provide other checks
on skb->protocol (thus avoiding the *if else if* in favor of only *if else*).

In your solution, if the protocol is neither IP6 nor IP4, then the pushed
IPv6 header is partially initialized. In particular, it seems to me that the
traffic class, flow label, and hop limit are not set anywhere else.

> > 
> > > +	skb->protocol = htons(ETH_P_IPV6);
> > > +

This seems to be redundant, since in seg6_do_srh() the protocol is set after
calling the seg6_do_srh_encap{_red}() functions.

> > > +	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?
> 
> Yes, but HMAC is just an optional part of SRH. RFC 8986 allows us to
> omit entire SRH in reduced encapsulation when the SRv6 Policy only
> contains one segment. 
> And it seems to be the most usefull approach as
> far as:
> 1) About all hardware implementations do not procede HMAC at all
> 2) Too many networking guys have a great concern about huge overhead of
> SRv6 in compare with MPLS, so they are not happy to get extra 256 bits
> 3) If one consider HMAC mandatory then there is still basic (not
> reduced) encapsulation option
> 
> What do you think about it?
> 

Thanks for the explanation.

However, considering the RFC 8986 Section 5.2:
  "The push of the SRH MAY be omitted when the SRv6 Policy only contains one
   segment and there is no need to use any flag, tag, or TLV."

Hence, if a user needs to use HMAC (rare case) or any other type of supported
flags, tags and TLVs, then the SRH should not be removed, even if there is
only one SID.

Andrea