* [PATCH net 0/3] ipv6: exthdrs: fix three SRH issues
@ 2023-05-17 21:31 Eric Dumazet
2023-05-17 21:31 ` [PATCH net 1/3] ipv6: exthdrs: fix potential use-after-free in ipv6_rpl_srh_rcv() Eric Dumazet
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: Eric Dumazet @ 2023-05-17 21:31 UTC (permalink / raw
To: David S . Miller, Jakub Kicinski, Paolo Abeni
Cc: Alexander Aring, David Lebrun, netdev, eric.dumazet, 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH net 1/3] ipv6: exthdrs: fix potential use-after-free in ipv6_rpl_srh_rcv()
2023-05-17 21:31 [PATCH net 0/3] ipv6: exthdrs: fix three SRH issues Eric Dumazet
@ 2023-05-17 21:31 ` Eric Dumazet
2023-05-18 17:05 ` Simon Horman
2023-05-17 21:31 ` [PATCH net 2/3] ipv6: exthdrs: avoid potential NULL deref in ipv6_srh_rcv() Eric Dumazet
` (2 subsequent siblings)
3 siblings, 1 reply; 14+ messages in thread
From: Eric Dumazet @ 2023-05-17 21:31 UTC (permalink / raw
To: David S . Miller, Jakub Kicinski, Paolo Abeni
Cc: Alexander Aring, David Lebrun, netdev, eric.dumazet, 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH net 2/3] ipv6: exthdrs: avoid potential NULL deref in ipv6_srh_rcv()
2023-05-17 21:31 [PATCH net 0/3] ipv6: exthdrs: fix three SRH issues Eric Dumazet
2023-05-17 21:31 ` [PATCH net 1/3] ipv6: exthdrs: fix potential use-after-free in ipv6_rpl_srh_rcv() Eric Dumazet
@ 2023-05-17 21:31 ` Eric Dumazet
2023-05-18 17:06 ` Simon Horman
2023-05-17 21:31 ` [PATCH net 3/3] ipv6: exthdrs: avoid potential NULL deref in ipv6_rpl_srh_rcv() Eric Dumazet
2023-05-18 1:53 ` [PATCH net 0/3] ipv6: exthdrs: fix three SRH issues Alexander Aring
3 siblings, 1 reply; 14+ messages in thread
From: Eric Dumazet @ 2023-05-17 21:31 UTC (permalink / raw
To: David S . Miller, Jakub Kicinski, Paolo Abeni
Cc: Alexander Aring, David Lebrun, netdev, eric.dumazet, 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH net 3/3] ipv6: exthdrs: avoid potential NULL deref in ipv6_rpl_srh_rcv()
2023-05-17 21:31 [PATCH net 0/3] ipv6: exthdrs: fix three SRH issues Eric Dumazet
2023-05-17 21:31 ` [PATCH net 1/3] ipv6: exthdrs: fix potential use-after-free in ipv6_rpl_srh_rcv() Eric Dumazet
2023-05-17 21:31 ` [PATCH net 2/3] ipv6: exthdrs: avoid potential NULL deref in ipv6_srh_rcv() Eric Dumazet
@ 2023-05-17 21:31 ` Eric Dumazet
2023-05-18 17:06 ` Simon Horman
2023-05-18 1:53 ` [PATCH net 0/3] ipv6: exthdrs: fix three SRH issues Alexander Aring
3 siblings, 1 reply; 14+ messages in thread
From: Eric Dumazet @ 2023-05-17 21:31 UTC (permalink / raw
To: David S . Miller, Jakub Kicinski, Paolo Abeni
Cc: Alexander Aring, David Lebrun, netdev, eric.dumazet, 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH net 0/3] ipv6: exthdrs: fix three SRH issues
2023-05-17 21:31 [PATCH net 0/3] ipv6: exthdrs: fix three SRH issues Eric Dumazet
` (2 preceding siblings ...)
2023-05-17 21:31 ` [PATCH net 3/3] ipv6: exthdrs: avoid potential NULL deref in ipv6_rpl_srh_rcv() Eric Dumazet
@ 2023-05-18 1:53 ` Alexander Aring
2023-05-21 17:05 ` Eric Dumazet
3 siblings, 1 reply; 14+ messages in thread
From: Alexander Aring @ 2023-05-18 1:53 UTC (permalink / raw
To: Eric Dumazet
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, Alexander Aring,
David Lebrun, netdev, eric.dumazet
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net 1/3] ipv6: exthdrs: fix potential use-after-free in ipv6_rpl_srh_rcv()
2023-05-17 21:31 ` [PATCH net 1/3] ipv6: exthdrs: fix potential use-after-free in ipv6_rpl_srh_rcv() Eric Dumazet
@ 2023-05-18 17:05 ` Simon Horman
2023-05-21 18:22 ` Eric Dumazet
0 siblings, 1 reply; 14+ messages in thread
From: Simon Horman @ 2023-05-18 17:05 UTC (permalink / raw
To: Eric Dumazet
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, Alexander Aring,
David Lebrun, netdev, eric.dumazet
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
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net 2/3] ipv6: exthdrs: avoid potential NULL deref in ipv6_srh_rcv()
2023-05-17 21:31 ` [PATCH net 2/3] ipv6: exthdrs: avoid potential NULL deref in ipv6_srh_rcv() Eric Dumazet
@ 2023-05-18 17:06 ` Simon Horman
0 siblings, 0 replies; 14+ messages in thread
From: Simon Horman @ 2023-05-18 17:06 UTC (permalink / raw
To: Eric Dumazet
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, Alexander Aring,
David Lebrun, netdev, eric.dumazet
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>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net 3/3] ipv6: exthdrs: avoid potential NULL deref in ipv6_rpl_srh_rcv()
2023-05-17 21:31 ` [PATCH net 3/3] ipv6: exthdrs: avoid potential NULL deref in ipv6_rpl_srh_rcv() Eric Dumazet
@ 2023-05-18 17:06 ` Simon Horman
0 siblings, 0 replies; 14+ messages in thread
From: Simon Horman @ 2023-05-18 17:06 UTC (permalink / raw
To: Eric Dumazet
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, Alexander Aring,
David Lebrun, netdev, eric.dumazet
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>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net 0/3] ipv6: exthdrs: fix three SRH issues
2023-05-18 1:53 ` [PATCH net 0/3] ipv6: exthdrs: fix three SRH issues Alexander Aring
@ 2023-05-21 17:05 ` Eric Dumazet
0 siblings, 0 replies; 14+ messages in thread
From: Eric Dumazet @ 2023-05-21 17:05 UTC (permalink / raw
To: Alexander Aring
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, Alexander Aring,
David Lebrun, netdev, 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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net 1/3] ipv6: exthdrs: fix potential use-after-free in ipv6_rpl_srh_rcv()
2023-05-18 17:05 ` Simon Horman
@ 2023-05-21 18:22 ` Eric Dumazet
2023-05-22 20:00 ` Jakub Kicinski
0 siblings, 1 reply; 14+ messages in thread
From: Eric Dumazet @ 2023-05-21 18:22 UTC (permalink / raw
To: Simon Horman
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, Alexander Aring,
David Lebrun, netdev, 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;
}
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net 1/3] ipv6: exthdrs: fix potential use-after-free in ipv6_rpl_srh_rcv()
2023-05-21 18:22 ` Eric Dumazet
@ 2023-05-22 20:00 ` Jakub Kicinski
2023-05-23 8:47 ` Simon Horman
0 siblings, 1 reply; 14+ messages in thread
From: Jakub Kicinski @ 2023-05-22 20:00 UTC (permalink / raw
To: Eric Dumazet
Cc: Simon Horman, David S . Miller, Paolo Abeni, Alexander Aring,
David Lebrun, netdev, eric.dumazet
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net 1/3] ipv6: exthdrs: fix potential use-after-free in ipv6_rpl_srh_rcv()
2023-05-22 20:00 ` Jakub Kicinski
@ 2023-05-23 8:47 ` Simon Horman
2023-05-23 10:45 ` Eric Dumazet
0 siblings, 1 reply; 14+ messages in thread
From: Simon Horman @ 2023-05-23 8:47 UTC (permalink / raw
To: Jakub Kicinski
Cc: Eric Dumazet, David S . Miller, Paolo Abeni, Alexander Aring,
David Lebrun, netdev, eric.dumazet
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net 1/3] ipv6: exthdrs: fix potential use-after-free in ipv6_rpl_srh_rcv()
2023-05-23 8:47 ` Simon Horman
@ 2023-05-23 10:45 ` Eric Dumazet
2023-05-23 14:41 ` Simon Horman
0 siblings, 1 reply; 14+ messages in thread
From: Eric Dumazet @ 2023-05-23 10:45 UTC (permalink / raw
To: Simon Horman
Cc: Jakub Kicinski, David S . Miller, Paolo Abeni, Alexander Aring,
netdev, 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)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net 1/3] ipv6: exthdrs: fix potential use-after-free in ipv6_rpl_srh_rcv()
2023-05-23 10:45 ` Eric Dumazet
@ 2023-05-23 14:41 ` Simon Horman
0 siblings, 0 replies; 14+ messages in thread
From: Simon Horman @ 2023-05-23 14:41 UTC (permalink / raw
To: Eric Dumazet
Cc: Jakub Kicinski, David S . Miller, Paolo Abeni, Alexander Aring,
netdev, eric.dumazet
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2023-05-23 14:41 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-17 21:31 [PATCH net 0/3] ipv6: exthdrs: fix three SRH issues Eric Dumazet
2023-05-17 21:31 ` [PATCH net 1/3] ipv6: exthdrs: fix potential use-after-free in ipv6_rpl_srh_rcv() Eric Dumazet
2023-05-18 17:05 ` Simon Horman
2023-05-21 18:22 ` Eric Dumazet
2023-05-22 20:00 ` Jakub Kicinski
2023-05-23 8:47 ` Simon Horman
2023-05-23 10:45 ` Eric Dumazet
2023-05-23 14:41 ` Simon Horman
2023-05-17 21:31 ` [PATCH net 2/3] ipv6: exthdrs: avoid potential NULL deref in ipv6_srh_rcv() Eric Dumazet
2023-05-18 17:06 ` Simon Horman
2023-05-17 21:31 ` [PATCH net 3/3] ipv6: exthdrs: avoid potential NULL deref in ipv6_rpl_srh_rcv() Eric Dumazet
2023-05-18 17:06 ` Simon Horman
2023-05-18 1:53 ` [PATCH net 0/3] ipv6: exthdrs: fix three SRH issues Alexander Aring
2023-05-21 17:05 ` Eric Dumazet
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).