[PATCH net 0/3] ipv6: exthdrs: fix three SRH issues

[PATCH net 0/3] ipv6: exthdrs: fix three SRH issues

[Eric Dumazet]

While looking at a related CVE, I found three problems worth fixing
in ipv6_rpl_srh_rcv() and ipv6_srh_rcv().

Eric Dumazet (3):
  ipv6: exthdrs: fix potential use-after-free in ipv6_rpl_srh_rcv()
  ipv6: exthdrs: avoid potential NULL deref in ipv6_srh_rcv()
  ipv6: exthdrs: avoid potential NULL deref in ipv6_rpl_srh_rcv()

 net/ipv6/exthdrs.c | 29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)

-- 
2.40.1.606.ga4b1b128d6-goog

[PATCH net 1/3] ipv6: exthdrs: fix potential use-after-free in ipv6_rpl_srh_rcv()

[Eric Dumazet]

After calls to pskb_may_pull() we need to reload @hdr variable,
because skb->head might have changed.

We need to move up first pskb_may_pull() call right after
looped_back label.

Fixes: 8610c7c6e3bd ("net: ipv6: add support for rpl sr exthdr")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Alexander Aring <alex.aring@gmail.com>
Cc: David Lebrun <david.lebrun@uclouvain.be>
---
 net/ipv6/exthdrs.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
index a8d961d3a477f6516f542025dfbcfc6f47407a70..b129e982205ee43cbf74f4900c3031827d962dc2 100644
--- a/net/ipv6/exthdrs.c
+++ b/net/ipv6/exthdrs.c
@@ -511,6 +511,10 @@ static int ipv6_rpl_srh_rcv(struct sk_buff *skb)
 	}
 
 looped_back:
+	if (!pskb_may_pull(skb, sizeof(*hdr))) {
+		kfree_skb(skb);
+		return -1;
+	}
 	hdr = (struct ipv6_rpl_sr_hdr *)skb_transport_header(skb);
 
 	if (hdr->segments_left == 0) {
@@ -544,12 +548,6 @@ static int ipv6_rpl_srh_rcv(struct sk_buff *skb)
 
 		return 1;
 	}
-
-	if (!pskb_may_pull(skb, sizeof(*hdr))) {
-		kfree_skb(skb);
-		return -1;
-	}
-
 	n = (hdr->hdrlen << 3) - hdr->pad - (16 - hdr->cmpre);
 	r = do_div(n, (16 - hdr->cmpri));
 	/* checks if calculation was without remainder and n fits into
@@ -592,6 +590,7 @@ static int ipv6_rpl_srh_rcv(struct sk_buff *skb)
 		kfree_skb(skb);
 		return -1;
 	}
+	hdr = (struct ipv6_rpl_sr_hdr *)skb_transport_header(skb);
 
 	hdr->segments_left--;
 	i = n - hdr->segments_left;
-- 
2.40.1.606.ga4b1b128d6-goog

Re: [PATCH net 1/3] ipv6: exthdrs: fix potential use-after-free in ipv6_rpl_srh_rcv()

[Simon Horman]

On Wed, May 17, 2023 at 09:31:16PM +0000, Eric Dumazet wrote:
> After calls to pskb_may_pull() we need to reload @hdr variable,
> because skb->head might have changed.
> 
> We need to move up first pskb_may_pull() call right after
> looped_back label.
> 
> Fixes: 8610c7c6e3bd ("net: ipv6: add support for rpl sr exthdr")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Alexander Aring <alex.aring@gmail.com>
> Cc: David Lebrun <david.lebrun@uclouvain.be>
> ---
>  net/ipv6/exthdrs.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
> index a8d961d3a477f6516f542025dfbcfc6f47407a70..b129e982205ee43cbf74f4900c3031827d962dc2 100644
> --- a/net/ipv6/exthdrs.c
> +++ b/net/ipv6/exthdrs.c
> @@ -511,6 +511,10 @@ static int ipv6_rpl_srh_rcv(struct sk_buff *skb)
>  	}
>  
>  looped_back:
> +	if (!pskb_may_pull(skb, sizeof(*hdr))) {
> +		kfree_skb(skb);
> +		return -1;
> +	}
>  	hdr = (struct ipv6_rpl_sr_hdr *)skb_transport_header(skb);
>  
>  	if (hdr->segments_left == 0) {

Hi Eric,

Not far below this line there is a call to pskb_pull():

                if (hdr->nexthdr == NEXTHDR_IPV6) {
                        int offset = (hdr->hdrlen + 1) << 3;

                        skb_postpull_rcsum(skb, skb_network_header(skb),
                                           skb_network_header_len(skb));

                        if (!pskb_pull(skb, offset)) {
                                kfree_skb(skb);
                                return -1;
                        }
                        skb_postpull_rcsum(skb, skb_transport_header(skb),
                                           offset);

Should hdr be reloaded after the call to pskb_pull() too?

> @@ -544,12 +548,6 @@ static int ipv6_rpl_srh_rcv(struct sk_buff *skb)
>  
>  		return 1;
>  	}
> -
> -	if (!pskb_may_pull(skb, sizeof(*hdr))) {
> -		kfree_skb(skb);
> -		return -1;
> -	}
> -
>  	n = (hdr->hdrlen << 3) - hdr->pad - (16 - hdr->cmpre);
>  	r = do_div(n, (16 - hdr->cmpri));
>  	/* checks if calculation was without remainder and n fits into
> @@ -592,6 +590,7 @@ static int ipv6_rpl_srh_rcv(struct sk_buff *skb)
>  		kfree_skb(skb);
>  		return -1;
>  	}
> +	hdr = (struct ipv6_rpl_sr_hdr *)skb_transport_header(skb);
>  
>  	hdr->segments_left--;
>  	i = n - hdr->segments_left;
> -- 
> 2.40.1.606.ga4b1b128d6-goog
> 
> 

Re: [PATCH net 1/3] ipv6: exthdrs: fix potential use-after-free in ipv6_rpl_srh_rcv()

[Eric Dumazet]

On Thu, May 18, 2023 at 7:05 PM Simon Horman <simon.horman@corigine.com> wrote:
>
> On Wed, May 17, 2023 at 09:31:16PM +0000, Eric Dumazet wrote:
> > After calls to pskb_may_pull() we need to reload @hdr variable,
> > because skb->head might have changed.
> >
> > We need to move up first pskb_may_pull() call right after
> > looped_back label.
> >
> > Fixes: 8610c7c6e3bd ("net: ipv6: add support for rpl sr exthdr")
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Cc: Alexander Aring <alex.aring@gmail.com>
> > Cc: David Lebrun <david.lebrun@uclouvain.be>
> > ---
> >  net/ipv6/exthdrs.c | 11 +++++------
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> >
> > diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
> > index a8d961d3a477f6516f542025dfbcfc6f47407a70..b129e982205ee43cbf74f4900c3031827d962dc2 100644
> > --- a/net/ipv6/exthdrs.c
> > +++ b/net/ipv6/exthdrs.c
> > @@ -511,6 +511,10 @@ static int ipv6_rpl_srh_rcv(struct sk_buff *skb)
> >       }
> >
> >  looped_back:
> > +     if (!pskb_may_pull(skb, sizeof(*hdr))) {
> > +             kfree_skb(skb);
> > +             return -1;
> > +     }
> >       hdr = (struct ipv6_rpl_sr_hdr *)skb_transport_header(skb);
> >
> >       if (hdr->segments_left == 0) {
>
> Hi Eric,
>
> Not far below this line there is a call to pskb_pull():
>
>                 if (hdr->nexthdr == NEXTHDR_IPV6) {
>                         int offset = (hdr->hdrlen + 1) << 3;
>
>                         skb_postpull_rcsum(skb, skb_network_header(skb),
>                                            skb_network_header_len(skb));
>
>                         if (!pskb_pull(skb, offset)) {
>                                 kfree_skb(skb);
>                                 return -1;
>                         }
>                         skb_postpull_rcsum(skb, skb_transport_header(skb),
>                                            offset);
>
> Should hdr be reloaded after the call to pskb_pull() too?

I do not think so, because @hdr is not used between this pskb_pull()
and the return -1:

       if (hdr->nexthdr == NEXTHDR_IPV6) {
            int offset = (hdr->hdrlen + 1) << 3;

            skb_postpull_rcsum(skb, skb_network_header(skb),
                       skb_network_header_len(skb));

            if (!pskb_pull(skb, offset)) {
                kfree_skb(skb);
                return -1;
            }
            skb_postpull_rcsum(skb, skb_transport_header(skb),
                       offset);

            skb_reset_network_header(skb);
            skb_reset_transport_header(skb);
            skb->encapsulation = 0;

            __skb_tunnel_rx(skb, skb->dev, net);

            netif_rx(skb);
            return -1;
        }

Re: [PATCH net 1/3] ipv6: exthdrs: fix potential use-after-free in ipv6_rpl_srh_rcv()

[Jakub Kicinski]

On Sun, 21 May 2023 20:22:16 +0200 Eric Dumazet wrote:
> On Thu, May 18, 2023 at 7:05 PM Simon Horman <simon.horman@corigine.com> wrote:
> > Not far below this line there is a call to pskb_pull():
> >
> >                 if (hdr->nexthdr == NEXTHDR_IPV6) {
> >                         int offset = (hdr->hdrlen + 1) << 3;
> >
> >                         skb_postpull_rcsum(skb, skb_network_header(skb),
> >                                            skb_network_header_len(skb));
> >
> >                         if (!pskb_pull(skb, offset)) {
> >                                 kfree_skb(skb);
> >                                 return -1;
> >                         }
> >                         skb_postpull_rcsum(skb, skb_transport_header(skb),
> >                                            offset);
> >
> > Should hdr be reloaded after the call to pskb_pull() too?  
> 
> I do not think so, because @hdr is not used between this pskb_pull()
> and the return -1:
> 
>        if (hdr->nexthdr == NEXTHDR_IPV6) {
>             int offset = (hdr->hdrlen + 1) << 3;
> 
>             skb_postpull_rcsum(skb, skb_network_header(skb),
>                        skb_network_header_len(skb));
> 
>             if (!pskb_pull(skb, offset)) {
>                 kfree_skb(skb);
>                 return -1;
>             }
>             skb_postpull_rcsum(skb, skb_transport_header(skb),
>                        offset);
> 
>             skb_reset_network_header(skb);
>             skb_reset_transport_header(skb);
>             skb->encapsulation = 0;
> 
>             __skb_tunnel_rx(skb, skb->dev, net);
> 
>             netif_rx(skb);
>             return -1;
>         }

Hum, there's very similar code in ipv6_srh_rcv() (a different function
but with a very similar name) which calls pskb_pull() and then checks
if hdr->nexthdr is v4. I'm guessing that's the one Simon was referring
to.

Re: [PATCH net 1/3] ipv6: exthdrs: fix potential use-after-free in ipv6_rpl_srh_rcv()

[Simon Horman]

On Mon, May 22, 2023 at 01:00:50PM -0700, Jakub Kicinski wrote:
> On Sun, 21 May 2023 20:22:16 +0200 Eric Dumazet wrote:
> > On Thu, May 18, 2023 at 7:05 PM Simon Horman <simon.horman@corigine.com> wrote:
> > > Not far below this line there is a call to pskb_pull():
> > >
> > >                 if (hdr->nexthdr == NEXTHDR_IPV6) {
> > >                         int offset = (hdr->hdrlen + 1) << 3;
> > >
> > >                         skb_postpull_rcsum(skb, skb_network_header(skb),
> > >                                            skb_network_header_len(skb));
> > >
> > >                         if (!pskb_pull(skb, offset)) {
> > >                                 kfree_skb(skb);
> > >                                 return -1;
> > >                         }
> > >                         skb_postpull_rcsum(skb, skb_transport_header(skb),
> > >                                            offset);
> > >
> > > Should hdr be reloaded after the call to pskb_pull() too?  
> > 
> > I do not think so, because @hdr is not used between this pskb_pull()
> > and the return -1:
> > 
> >        if (hdr->nexthdr == NEXTHDR_IPV6) {
> >             int offset = (hdr->hdrlen + 1) << 3;
> > 
> >             skb_postpull_rcsum(skb, skb_network_header(skb),
> >                        skb_network_header_len(skb));
> > 
> >             if (!pskb_pull(skb, offset)) {
> >                 kfree_skb(skb);
> >                 return -1;
> >             }
> >             skb_postpull_rcsum(skb, skb_transport_header(skb),
> >                        offset);
> > 
> >             skb_reset_network_header(skb);
> >             skb_reset_transport_header(skb);
> >             skb->encapsulation = 0;
> > 
> >             __skb_tunnel_rx(skb, skb->dev, net);
> > 
> >             netif_rx(skb);
> >             return -1;
> >         }
> 
> Hum, there's very similar code in ipv6_srh_rcv() (a different function
> but with a very similar name) which calls pskb_pull() and then checks
> if hdr->nexthdr is v4. I'm guessing that's the one Simon was referring
> to.

Yes, that does seem to be the case.

Re: [PATCH net 1/3] ipv6: exthdrs: fix potential use-after-free in ipv6_rpl_srh_rcv()

[Eric Dumazet]

On Tue, May 23, 2023 at 10:47 AM Simon Horman <simon.horman@corigine.com> wrote:
>
> On Mon, May 22, 2023 at 01:00:50PM -0700, Jakub Kicinski wrote:
> > On Sun, 21 May 2023 20:22:16 +0200 Eric Dumazet wrote:
> > > On Thu, May 18, 2023 at 7:05 PM Simon Horman <simon.horman@corigine.com> wrote:
> > > > Not far below this line there is a call to pskb_pull():
> > > >
> > > >                 if (hdr->nexthdr == NEXTHDR_IPV6) {
> > > >                         int offset = (hdr->hdrlen + 1) << 3;
> > > >
> > > >                         skb_postpull_rcsum(skb, skb_network_header(skb),
> > > >                                            skb_network_header_len(skb));
> > > >
> > > >                         if (!pskb_pull(skb, offset)) {
> > > >                                 kfree_skb(skb);
> > > >                                 return -1;
> > > >                         }
> > > >                         skb_postpull_rcsum(skb, skb_transport_header(skb),
> > > >                                            offset);
> > > >
> > > > Should hdr be reloaded after the call to pskb_pull() too?
> > >
> > > I do not think so, because @hdr is not used between this pskb_pull()
> > > and the return -1:
> > >
> > >        if (hdr->nexthdr == NEXTHDR_IPV6) {
> > >             int offset = (hdr->hdrlen + 1) << 3;
> > >
> > >             skb_postpull_rcsum(skb, skb_network_header(skb),
> > >                        skb_network_header_len(skb));
> > >
> > >             if (!pskb_pull(skb, offset)) {
> > >                 kfree_skb(skb);
> > >                 return -1;
> > >             }
> > >             skb_postpull_rcsum(skb, skb_transport_header(skb),
> > >                        offset);
> > >
> > >             skb_reset_network_header(skb);
> > >             skb_reset_transport_header(skb);
> > >             skb->encapsulation = 0;
> > >
> > >             __skb_tunnel_rx(skb, skb->dev, net);
> > >
> > >             netif_rx(skb);
> > >             return -1;
> > >         }
> >
> > Hum, there's very similar code in ipv6_srh_rcv() (a different function
> > but with a very similar name) which calls pskb_pull() and then checks
> > if hdr->nexthdr is v4. I'm guessing that's the one Simon was referring
> > to.
>
> Yes, that does seem to be the case.

I think ipv6_srh_rcv() is fine.

The "goto looped_back" does not need to reload hdr.

The only point where skb->head can change is at the pskb_expand_head() call,
which is properly followed by:

hdr = (struct ipv6_sr_hdr *)skb_transport_header(skb);

I will send a V2, because this first patch in the series can also make
ipv6_rpl_srh_rcv() similar.
(No need to move around the pskb_may_pull(skb, sizeof(*hdr)

Re: [PATCH net 1/3] ipv6: exthdrs: fix potential use-after-free in ipv6_rpl_srh_rcv()

[Simon Horman]

On Tue, May 23, 2023 at 12:45:34PM +0200, Eric Dumazet wrote:
> On Tue, May 23, 2023 at 10:47 AM Simon Horman <simon.horman@corigine.com> wrote:
> >
> > On Mon, May 22, 2023 at 01:00:50PM -0700, Jakub Kicinski wrote:
> > > On Sun, 21 May 2023 20:22:16 +0200 Eric Dumazet wrote:
> > > > On Thu, May 18, 2023 at 7:05 PM Simon Horman <simon.horman@corigine.com> wrote:
> > > > > Not far below this line there is a call to pskb_pull():
> > > > >
> > > > >                 if (hdr->nexthdr == NEXTHDR_IPV6) {
> > > > >                         int offset = (hdr->hdrlen + 1) << 3;
> > > > >
> > > > >                         skb_postpull_rcsum(skb, skb_network_header(skb),
> > > > >                                            skb_network_header_len(skb));
> > > > >
> > > > >                         if (!pskb_pull(skb, offset)) {
> > > > >                                 kfree_skb(skb);
> > > > >                                 return -1;
> > > > >                         }
> > > > >                         skb_postpull_rcsum(skb, skb_transport_header(skb),
> > > > >                                            offset);
> > > > >
> > > > > Should hdr be reloaded after the call to pskb_pull() too?
> > > >
> > > > I do not think so, because @hdr is not used between this pskb_pull()
> > > > and the return -1:
> > > >
> > > >        if (hdr->nexthdr == NEXTHDR_IPV6) {
> > > >             int offset = (hdr->hdrlen + 1) << 3;
> > > >
> > > >             skb_postpull_rcsum(skb, skb_network_header(skb),
> > > >                        skb_network_header_len(skb));
> > > >
> > > >             if (!pskb_pull(skb, offset)) {
> > > >                 kfree_skb(skb);
> > > >                 return -1;
> > > >             }
> > > >             skb_postpull_rcsum(skb, skb_transport_header(skb),
> > > >                        offset);
> > > >
> > > >             skb_reset_network_header(skb);
> > > >             skb_reset_transport_header(skb);
> > > >             skb->encapsulation = 0;
> > > >
> > > >             __skb_tunnel_rx(skb, skb->dev, net);
> > > >
> > > >             netif_rx(skb);
> > > >             return -1;
> > > >         }
> > >
> > > Hum, there's very similar code in ipv6_srh_rcv() (a different function
> > > but with a very similar name) which calls pskb_pull() and then checks
> > > if hdr->nexthdr is v4. I'm guessing that's the one Simon was referring
> > > to.
> >
> > Yes, that does seem to be the case.
> 
> I think ipv6_srh_rcv() is fine.
> 
> The "goto looped_back" does not need to reload hdr.
> 
> The only point where skb->head can change is at the pskb_expand_head() call,
> which is properly followed by:
> 
> hdr = (struct ipv6_sr_hdr *)skb_transport_header(skb);
> 
> I will send a V2, because this first patch in the series can also make
> ipv6_rpl_srh_rcv() similar.
> (No need to move around the pskb_may_pull(skb, sizeof(*hdr)
> 

Hi Eric,

Thanks for the analysis.
And sorry for the noise on this one.

[PATCH net 2/3] ipv6: exthdrs: avoid potential NULL deref in ipv6_srh_rcv()

[Eric Dumazet]

There is some chance __in6_dev_get() returns NULL, we should
not crash if that happens.

ipv6_srh_rcv() caller (ipv6_rthdr_rcv()) correctly deals with
a NULL idev, we can use the same idea.

Same problem was later added in ipv6_rpl_srh_rcv(),
this is handled in a separate patch to ease stable backports.

Fixes: 1ababeba4a21 ("ipv6: implement dataplane support for rthdr type 4 (Segment Routing Header)")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: David Lebrun <david.lebrun@uclouvain.be>
Cc: Alexander Aring <alex.aring@gmail.com>
---
 net/ipv6/exthdrs.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
index b129e982205ee43cbf74f4900c3031827d962dc2..4f874f70b3fb1f6b372b937fcfe6ebd1a56b921d 100644
--- a/net/ipv6/exthdrs.c
+++ b/net/ipv6/exthdrs.c
@@ -366,21 +366,18 @@ static void seg6_update_csum(struct sk_buff *skb)
 			   (__be32 *)addr);
 }
 
-static int ipv6_srh_rcv(struct sk_buff *skb)
+static int ipv6_srh_rcv(struct sk_buff *skb, struct inet6_dev *idev)
 {
 	struct inet6_skb_parm *opt = IP6CB(skb);
 	struct net *net = dev_net(skb->dev);
 	struct ipv6_sr_hdr *hdr;
-	struct inet6_dev *idev;
 	struct in6_addr *addr;
 	int accept_seg6;
 
 	hdr = (struct ipv6_sr_hdr *)skb_transport_header(skb);
 
-	idev = __in6_dev_get(skb->dev);
-
 	accept_seg6 = net->ipv6.devconf_all->seg6_enabled;
-	if (accept_seg6 > idev->cnf.seg6_enabled)
+	if (idev && accept_seg6 > idev->cnf.seg6_enabled)
 		accept_seg6 = idev->cnf.seg6_enabled;
 
 	if (!accept_seg6) {
@@ -711,7 +708,7 @@ static int ipv6_rthdr_rcv(struct sk_buff *skb)
 	switch (hdr->type) {
 	case IPV6_SRCRT_TYPE_4:
 		/* segment routing */
-		return ipv6_srh_rcv(skb);
+		return ipv6_srh_rcv(skb, idev);
 	case IPV6_SRCRT_TYPE_3:
 		/* rpl segment routing */
 		return ipv6_rpl_srh_rcv(skb);
-- 
2.40.1.606.ga4b1b128d6-goog

Re: [PATCH net 2/3] ipv6: exthdrs: avoid potential NULL deref in ipv6_srh_rcv()

[Simon Horman]

On Wed, May 17, 2023 at 09:31:17PM +0000, Eric Dumazet wrote:
> There is some chance __in6_dev_get() returns NULL, we should
> not crash if that happens.
> 
> ipv6_srh_rcv() caller (ipv6_rthdr_rcv()) correctly deals with
> a NULL idev, we can use the same idea.
> 
> Same problem was later added in ipv6_rpl_srh_rcv(),
> this is handled in a separate patch to ease stable backports.
> 
> Fixes: 1ababeba4a21 ("ipv6: implement dataplane support for rthdr type 4 (Segment Routing Header)")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: David Lebrun <david.lebrun@uclouvain.be>
> Cc: Alexander Aring <alex.aring@gmail.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

[PATCH net 3/3] ipv6: exthdrs: avoid potential NULL deref in ipv6_rpl_srh_rcv()

[Eric Dumazet]

There is some chance __in6_dev_get() returns NULL, we should
not crash if that happens.

ipv6_rpl_srh_rcv() caller (ipv6_rthdr_rcv()) correctly deals with
a NULL idev, we can use the same idea.

Fixes: 8610c7c6e3bd ("net: ipv6: add support for rpl sr exthdr")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: David Lebrun <david.lebrun@uclouvain.be>
Cc: Alexander Aring <alex.aring@gmail.com>
---
 net/ipv6/exthdrs.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
index 4f874f70b3fb1f6b372b937fcfe6ebd1a56b921d..cf86d07227d0c4fe7081a45a61124f8aaae4ec3a 100644
--- a/net/ipv6/exthdrs.c
+++ b/net/ipv6/exthdrs.c
@@ -483,12 +483,11 @@ static int ipv6_srh_rcv(struct sk_buff *skb, struct inet6_dev *idev)
 	return -1;
 }
 
-static int ipv6_rpl_srh_rcv(struct sk_buff *skb)
+static int ipv6_rpl_srh_rcv(struct sk_buff *skb, struct inet6_dev *idev)
 {
 	struct ipv6_rpl_sr_hdr *hdr, *ohdr, *chdr;
 	struct inet6_skb_parm *opt = IP6CB(skb);
 	struct net *net = dev_net(skb->dev);
-	struct inet6_dev *idev;
 	struct ipv6hdr *oldhdr;
 	unsigned char *buf;
 	int accept_rpl_seg;
@@ -496,10 +495,8 @@ static int ipv6_rpl_srh_rcv(struct sk_buff *skb)
 	u64 n = 0;
 	u32 r;
 
-	idev = __in6_dev_get(skb->dev);
-
 	accept_rpl_seg = net->ipv6.devconf_all->rpl_seg_enabled;
-	if (accept_rpl_seg > idev->cnf.rpl_seg_enabled)
+	if (idev && accept_rpl_seg > idev->cnf.rpl_seg_enabled)
 		accept_rpl_seg = idev->cnf.rpl_seg_enabled;
 
 	if (!accept_rpl_seg) {
@@ -711,7 +708,7 @@ static int ipv6_rthdr_rcv(struct sk_buff *skb)
 		return ipv6_srh_rcv(skb, idev);
 	case IPV6_SRCRT_TYPE_3:
 		/* rpl segment routing */
-		return ipv6_rpl_srh_rcv(skb);
+		return ipv6_rpl_srh_rcv(skb, idev);
 	default:
 		break;
 	}
-- 
2.40.1.606.ga4b1b128d6-goog

Re: [PATCH net 3/3] ipv6: exthdrs: avoid potential NULL deref in ipv6_rpl_srh_rcv()

[Simon Horman]

On Wed, May 17, 2023 at 09:31:18PM +0000, Eric Dumazet wrote:
> There is some chance __in6_dev_get() returns NULL, we should
> not crash if that happens.
> 
> ipv6_rpl_srh_rcv() caller (ipv6_rthdr_rcv()) correctly deals with
> a NULL idev, we can use the same idea.
> 
> Fixes: 8610c7c6e3bd ("net: ipv6: add support for rpl sr exthdr")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: David Lebrun <david.lebrun@uclouvain.be>
> Cc: Alexander Aring <alex.aring@gmail.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH net 0/3] ipv6: exthdrs: fix three SRH issues

[Alexander Aring]

Hi,

On Wed, May 17, 2023 at 5:31 PM Eric Dumazet <edumazet@google.com> wrote:
>
> While looking at a related CVE, I found three problems worth fixing
> in ipv6_rpl_srh_rcv() and ipv6_srh_rcv().

thanks, for looking into it. I got some reproducers for the CVE (I
hope we are talking about the same one), I believe it has something to
do with what Jakub already pointed out. It's about
IPV6_RPL_SRH_WORST_SWAP_SIZE [0] is not correct, if the last address
in the segment address array is completely different than all other
segment addresses the source header will grow a lot, about (number of
segment addresses * sizeof(struct in6_addr)). Maybe there can be more
intelligent ways to find the right number here... however I tried to
change it without success to fix the problem. :-/

- Alex

[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/ipv6/exthdrs.c?h=v6.4-rc2#n572

Re: [PATCH net 0/3] ipv6: exthdrs: fix three SRH issues

[Eric Dumazet]

On Thu, May 18, 2023 at 3:53 AM Alexander Aring <aahringo@redhat.com> wrote:
>
> Hi,
>
> On Wed, May 17, 2023 at 5:31 PM Eric Dumazet <edumazet@google.com> wrote:
> >
> > While looking at a related CVE, I found three problems worth fixing
> > in ipv6_rpl_srh_rcv() and ipv6_srh_rcv().
>
> thanks, for looking into it. I got some reproducers for the CVE (I
> hope we are talking about the same one), I believe it has something to
> do with what Jakub already pointed out. It's about
> IPV6_RPL_SRH_WORST_SWAP_SIZE [0] is not correct, if the last address
> in the segment address array is completely different than all other
> segment addresses the source header will grow a lot, about (number of
> segment addresses * sizeof(struct in6_addr)). Maybe there can be more
> intelligent ways to find the right number here... however I tried to
> change it without success to fix the problem. :-/

Hmmm... this patch series fixes other generic issues.

I have not claimed to solve this CVE yet.