netdev - Re: [PATCH] NET: SRv6: seg6_local: Fixed SRH processing when segments left is 0

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>] [day] [month] [year] [list]
Message-Id: <20210203095005.4de51bc6632f2da2bb55c70a@gmail.com>
Date:   Wed, 3 Feb 2021 09:50:05 +0100
From:   Ahmed Abdelsalam <ahabdels.dev@...il.com>
To:     Suprit Japagal <suprit.japagal@...il.com>
Cc:     David Ahern <dsahern@...il.com>, davem@...emloft.net,
        yoshfuji@...ux-ipv6.org, dsahern@...nel.org,
        David Lebrun <david.lebrun@...ouvain.be>,
        netdev@...r.kernel.org
Subject: Re: [PATCH] NET: SRv6: seg6_local: Fixed SRH processing when
 segments left is 0

Hi Suprit, 

As you see the in the pseudocode, if Segments Left = 0, the End function will 
stop processing the SRH and will process upper layer protocols for OAM (e.g., ICMP). 

The packet will not be forwarded further. 

What you propose here is a different thing, you want to decap the SRv6 
headers and forward the packet further if Segments Left = 0.

This is not a correct symantic of SRv6 End Behavior.   


But if you need such behavior in your network, it already defined by another 
flavor of the End function called Ultimate Segment Decapsulation (USD) [2]

Please feel free to submit a patch to support this USD flavor of END.  

[2] https://tools.ietf.org/html/draft-ietf-spring-srv6-network-programming-28#section-4.16.3

Thanks
Ahmed
 


On Tue, 2 Feb 2021 16:50:58 +0530
Suprit Japagal <suprit.japagal@...il.com> wrote:

> Hi,
> 
> As per the section 4.1 of [1]:
> 
> "When N receives a packet whose IPv6 DA is S and S is a local *End SID*,
> 
>    N does:
> 
>    S01. When an SRH is processed {
>    S02.   If (Segments Left == 0) {
>    S03.      Stop processing the SRH, and proceed to process the next
>                 header in the packet, whose type is identified by
>                 the Next Header field in the routing header.
> 
>    S04. }
> "
> S is the DA which is a local End SID and when SRH is processed, the
> segments left field can be 0 as mentioned above.
> Same goes for End.X SID and End.T SID as per section 4.2 and 4.3 of [1]
> respectively.
> 
> Packets processed by End, End.X and End.T behaviors can have a Segment Left
> Value
> of zero.
> 
> [1]https://tools.ietf.org/html/draft-ietf-spring-srv6-network-programming-28
> 
> Thanks,
> Suprit J
> 
> On Tue, Feb 2, 2021 at 3:22 PM Ahmed Abdelsalam <ahabdels.dev@...il.com>
> wrote:
> 
> > The current implementation is correct. This patch is introducing incorrect
> > symantic to SRv6 End and End.T behaviors.
> >
> > SRv6 End and End.T behaviors (among other behaviors) are defined in the
> > SRv6 Network Programming draft (Soon to published as an RFC) [1].
> >
> > SRv6 End and End.T behaviors are used to implement Traffic Engineering (TE)
> > use-cases, where a node recieves the packet and send it to the next SID
> > from
> > the SRH SIDList.
> >
> > Packets processed by End and End.T behaviors can not have a Segment Left
> > Value
> > of zero.
> >
> > Please refer to sections 4.1 and 4.3 of [1].
> >
> >
> > [1]
> > https://tools.ietf.org/html/draft-ietf-spring-srv6-network-programming-28
> >
> > Ahmed
> >
> >
> > On Sun, 31 Jan 2021 10:33:14 -0700
> > David Ahern <dsahern@...il.com> wrote:
> >
> > > [ cc David Lebrun, author of the original code ]
> > >
> > > On 1/31/21 6:08 AM, Suprit Japagal wrote:
> > > > From: "Suprit.Japagal" <suprit.japagal@...il.com>
> > > >
> > > > According to the standard IETF RFC 8754, section 4.3.1.1
> > > > (https://tools.ietf.org/html/rfc8754#section-4.3.1.1)
> > > > When the segments left in SRH equals to 0, proceed to process the
> > > > next header in the packet, whose type is identified by the
> > > > Next header field of the routing header.
> > > >
> > > > Signed-off-by: Suprit.Japagal <suprit.japagal@...il.com>
> > > > ---
> > > >  net/ipv6/seg6_local.c | 54
> > +++++++++++++++++++++++++++++++++++++++++++++------
> > > >  1 file changed, 48 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c
> > > > index b07f7c1..b17f9dc 100644
> > > > --- a/net/ipv6/seg6_local.c
> > > > +++ b/net/ipv6/seg6_local.c
> > > > @@ -273,11 +273,25 @@ static int input_action_end(struct sk_buff *skb,
> > struct seg6_local_lwt *slwt)
> > > >  {
> > > >     struct ipv6_sr_hdr *srh;
> > > >
> > > > -   srh = get_and_validate_srh(skb);
> > > > +   srh = get_srh(skb);
> > > >     if (!srh)
> > > >             goto drop;
> > > >
> > > > -   advance_nextseg(srh, &ipv6_hdr(skb)->daddr);
> > > > +#ifdef CONFIG_IPV6_SEG6_HMAC
> > > > +   if (srh->segments_left > 0)
> > > > +           if (!seg6_hmac_validate_skb(skb))
> > > > +                   goto drop;
> > > > +#endif
> > > > +
> > > > +   if (srh->segments_left == 0) {
> > > > +           if (!decap_and_validate(skb, srh->nexthdr))
> > > > +                   goto drop;
> > > > +
> > > > +           if (!pskb_may_pull(skb, sizeof(struct ipv6hdr)))
> > > > +                   goto drop;
> > > > +   } else {
> > > > +           advance_nextseg(srh, &ipv6_hdr(skb)->daddr);
> > > > +   }
> > > >
> > > >     seg6_lookup_nexthop(skb, NULL, 0);
> > > >
> > > > @@ -293,11 +307,25 @@ static int input_action_end_x(struct sk_buff
> > *skb, struct seg6_local_lwt *slwt)
> > > >  {
> > > >     struct ipv6_sr_hdr *srh;
> > > >
> > > > -   srh = get_and_validate_srh(skb);
> > > > +   srh = get_srh(skb);
> > > >     if (!srh)
> > > >             goto drop;
> > > >
> > > > -   advance_nextseg(srh, &ipv6_hdr(skb)->daddr);
> > > > +#ifdef CONFIG_IPV6_SEG6_HMAC
> > > > +   if (srh->segments_left > 0)
> > > > +           if (!seg6_hmac_validate_skb(skb))
> > > > +                   goto drop;
> > > > +#endif
> > > > +
> > > > +   if (srh->segments_left == 0) {
> > > > +           if (!decap_and_validate(skb, srh->nexthdr))
> > > > +                   goto drop;
> > > > +
> > > > +           if (!pskb_may_pull(skb, sizeof(struct ipv6hdr)))
> > > > +                   goto drop;
> > > > +   } else {
> > > > +           advance_nextseg(srh, &ipv6_hdr(skb)->daddr);
> > > > +   }
> > > >
> > > >     seg6_lookup_nexthop(skb, &slwt->nh6, 0);
> > > >
> > > > @@ -312,11 +340,25 @@ static int input_action_end_t(struct sk_buff
> > *skb, struct seg6_local_lwt *slwt)
> > > >  {
> > > >     struct ipv6_sr_hdr *srh;
> > > >
> > > > -   srh = get_and_validate_srh(skb);
> > > > +   srh = get_srh(skb);
> > > >     if (!srh)
> > > >             goto drop;
> > > >
> > > > -   advance_nextseg(srh, &ipv6_hdr(skb)->daddr);
> > > > +#ifdef CONFIG_IPV6_SEG6_HMAC
> > > > +   if (srh->segments_left > 0)
> > > > +           if (!seg6_hmac_validate_skb(skb))
> > > > +                   goto drop;
> > > > +#endif
> > > > +
> > > > +   if (srh->segments_left == 0) {
> > > > +           if (!decap_and_validate(skb, srh->nexthdr))
> > > > +                   goto drop;
> > > > +
> > > > +           if (!pskb_may_pull(skb, sizeof(struct ipv6hdr)))
> > > > +                   goto drop;
> > > > +   } else {
> > > > +           advance_nextseg(srh, &ipv6_hdr(skb)->daddr);
> > > > +   }
> > > >
> > > >     seg6_lookup_nexthop(skb, NULL, slwt->table);
> > > >
> > > >
> > >
> >
> >
> > --
> > Ahmed Abdelsalam <ahabdels.dev@...il.com>
> >


-- 
Ahmed Abdelsalam <ahabdels.dev@...il.com>