[PATCH net] udp: fix a skb extensions leak

[PATCH net] udp: fix a skb extensions leak

[Xin Long]

On udp rx path udp_rcv_segment() may do segment where the frag skbs
will get the header copied from the head skb in skb_segment_list()
by calling __copy_skb_header(), which could overwrite the frag skbs'
extensions by __skb_ext_copy() and cause a leak.

This issue was found after loading esp_offload where a sec path ext
is set in the skb.

On udp tx gso path, it works well as the frag skbs' extensions are
not set. So this issue should be fixed on udp's rx path only and
release the frag skbs' extensions before going to do segment.

Reported-by: Xiumei Mu <xmu@redhat.com>
Fixes: cf329aa42b66 ("udp: cope with UDP GRO packet misdirection")
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/net/udp.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/net/udp.h b/include/net/udp.h
index e55d5f7..7bf0ca5 100644
--- a/include/net/udp.h
+++ b/include/net/udp.h
@@ -486,6 +486,10 @@ static inline struct sk_buff *udp_rcv_segment(struct sock *sk,
 	if (skb->pkt_type == PACKET_LOOPBACK)
 		skb->ip_summed = CHECKSUM_PARTIAL;
 
+	if (skb_has_frag_list(skb) && skb_has_extensions(skb))
+		skb_walk_frags(skb, segs)
+			skb_ext_put(segs);
+
 	/* the GSO CB lays after the UDP one, no need to save and restore any
 	 * CB fragment
 	 */
-- 
2.1.0

Re: [PATCH net] udp: fix a skb extensions leak

[Xin Long]

CC Steffen Klassert <steffen.klassert@secunet.com>

On Thu, Mar 26, 2020 at 5:06 PM Xin Long <lucien.xin@gmail.com> wrote:
>
> On udp rx path udp_rcv_segment() may do segment where the frag skbs
> will get the header copied from the head skb in skb_segment_list()
> by calling __copy_skb_header(), which could overwrite the frag skbs'
> extensions by __skb_ext_copy() and cause a leak.
>
> This issue was found after loading esp_offload where a sec path ext
> is set in the skb.
>
> On udp tx gso path, it works well as the frag skbs' extensions are
> not set. So this issue should be fixed on udp's rx path only and
> release the frag skbs' extensions before going to do segment.
>
> Reported-by: Xiumei Mu <xmu@redhat.com>
> Fixes: cf329aa42b66 ("udp: cope with UDP GRO packet misdirection")
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  include/net/udp.h | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/include/net/udp.h b/include/net/udp.h
> index e55d5f7..7bf0ca5 100644
> --- a/include/net/udp.h
> +++ b/include/net/udp.h
> @@ -486,6 +486,10 @@ static inline struct sk_buff *udp_rcv_segment(struct sock *sk,
>         if (skb->pkt_type == PACKET_LOOPBACK)
>                 skb->ip_summed = CHECKSUM_PARTIAL;
>
> +       if (skb_has_frag_list(skb) && skb_has_extensions(skb))
> +               skb_walk_frags(skb, segs)
> +                       skb_ext_put(segs);
> +
>         /* the GSO CB lays after the UDP one, no need to save and restore any
>          * CB fragment
>          */
> --
> 2.1.0
>

Re: [PATCH net] udp: fix a skb extensions leak

[David Miller]

From: Xin Long <lucien.xin@gmail.com>
Date: Thu, 26 Mar 2020 17:06:25 +0800

> On udp rx path udp_rcv_segment() may do segment where the frag skbs
> will get the header copied from the head skb in skb_segment_list()
> by calling __copy_skb_header(), which could overwrite the frag skbs'
> extensions by __skb_ext_copy() and cause a leak.
> 
> This issue was found after loading esp_offload where a sec path ext
> is set in the skb.
> 
> On udp tx gso path, it works well as the frag skbs' extensions are
> not set. So this issue should be fixed on udp's rx path only and
> release the frag skbs' extensions before going to do segment.
> 
> Reported-by: Xiumei Mu <xmu@redhat.com>
> Fixes: cf329aa42b66 ("udp: cope with UDP GRO packet misdirection")
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Steffen, please review.

Re: [PATCH net] udp: fix a skb extensions leak

[Steffen Klassert]

On Thu, Mar 26, 2020 at 05:06:25PM +0800, Xin Long wrote:
> On udp rx path udp_rcv_segment() may do segment where the frag skbs
> will get the header copied from the head skb in skb_segment_list()
> by calling __copy_skb_header(), which could overwrite the frag skbs'
> extensions by __skb_ext_copy() and cause a leak.
> 
> This issue was found after loading esp_offload where a sec path ext
> is set in the skb.
> 
> On udp tx gso path, it works well as the frag skbs' extensions are
> not set. So this issue should be fixed on udp's rx path only and
> release the frag skbs' extensions before going to do segment.

Are you sure that this affects only the RX path? What if such
a packet is forwarded? Also, I think TCP has the same problem.

> 
> Reported-by: Xiumei Mu <xmu@redhat.com>
> Fixes: cf329aa42b66 ("udp: cope with UDP GRO packet misdirection")
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  include/net/udp.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/net/udp.h b/include/net/udp.h
> index e55d5f7..7bf0ca5 100644
> --- a/include/net/udp.h
> +++ b/include/net/udp.h
> @@ -486,6 +486,10 @@ static inline struct sk_buff *udp_rcv_segment(struct sock *sk,
>  	if (skb->pkt_type == PACKET_LOOPBACK)
>  		skb->ip_summed = CHECKSUM_PARTIAL;
>  
> +	if (skb_has_frag_list(skb) && skb_has_extensions(skb))
> +		skb_walk_frags(skb, segs)
> +			skb_ext_put(segs);

If a skb in the fraglist has a secpath, it is still valid.
So maybe instead of dropping it here and assign the one
from the head skb, we could just keep the secpath. But
I don't know about other extensions. I've CCed Florian,
he might know a bit more about other extensions. Also,
it might be good to check if the extensions of the GRO
packets are all the same before merging.

Re: [PATCH net] udp: fix a skb extensions leak

[Florian Westphal]

Xin Long <lucien.xin@gmail.com> wrote:
> On udp rx path udp_rcv_segment() may do segment where the frag skbs
> will get the header copied from the head skb in skb_segment_list()
> by calling __copy_skb_header(), which could overwrite the frag skbs'
> extensions by __skb_ext_copy() and cause a leak.
> 
> This issue was found after loading esp_offload where a sec path ext
> is set in the skb.
> 
> On udp tx gso path, it works well as the frag skbs' extensions are
> not set. So this issue should be fixed on udp's rx path only and
> release the frag skbs' extensions before going to do segment.
> Reported-by: Xiumei Mu <xmu@redhat.com>
> Fixes: cf329aa42b66 ("udp: cope with UDP GRO packet misdirection")

Hmm, I suspect this bug came in via
3a1296a38d0cf62bffb9a03c585cbd5dbf15d596 , net: Support GRO/GSO fraglist chaining.

I suspect correct fix is:

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 621b4479fee1..7e29590482ce 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3668,6 +3668,7 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,

                skb_push(nskb, -skb_network_offset(nskb) + offset);

+               skb_release_head_state(nskb);
                 __copy_skb_header(nskb, skb);

                skb_headers_offset_update(nskb, skb_headroom(nskb) - skb_headroom(skb));

AFAICS we not only leak reference of extensions, but also skb->dst and skb->_nfct.

Re: [PATCH net] udp: fix a skb extensions leak

[Steffen Klassert]

On Mon, Mar 30, 2020 at 03:27:59PM +0200, Florian Westphal wrote:
> Xin Long <lucien.xin@gmail.com> wrote:
> > On udp rx path udp_rcv_segment() may do segment where the frag skbs
> > will get the header copied from the head skb in skb_segment_list()
> > by calling __copy_skb_header(), which could overwrite the frag skbs'
> > extensions by __skb_ext_copy() and cause a leak.
> > 
> > This issue was found after loading esp_offload where a sec path ext
> > is set in the skb.
> > 
> > On udp tx gso path, it works well as the frag skbs' extensions are
> > not set. So this issue should be fixed on udp's rx path only and
> > release the frag skbs' extensions before going to do segment.
> > Reported-by: Xiumei Mu <xmu@redhat.com>
> > Fixes: cf329aa42b66 ("udp: cope with UDP GRO packet misdirection")
> 
> Hmm, I suspect this bug came in via
> 3a1296a38d0cf62bffb9a03c585cbd5dbf15d596 , net: Support GRO/GSO fraglist chaining.

I overlooked the explicit mentioning of skb_segment_list()
in the commit message. Fraglist GRO is disabled by default,
this means that it was enabled explicitely. So yes, the bug
came via that commit.

> 
> I suspect correct fix is:
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 621b4479fee1..7e29590482ce 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3668,6 +3668,7 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,
> 
>                 skb_push(nskb, -skb_network_offset(nskb) + offset);
> 
> +               skb_release_head_state(nskb);
>                  __copy_skb_header(nskb, skb);
> 
>                 skb_headers_offset_update(nskb, skb_headroom(nskb) - skb_headroom(skb));
> 
> AFAICS we not only leak reference of extensions, but also skb->dst and skb->_nfct.

Would be nice if we would not need to drop the resources
just to add them back again in the next line. But it is ok
as a quick fix for the bug.

Re: [PATCH net] udp: fix a skb extensions leak

[Florian Westphal]

Steffen Klassert <steffen.klassert@secunet.com> wrote:
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index 621b4479fee1..7e29590482ce 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -3668,6 +3668,7 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,
> > 
> >                 skb_push(nskb, -skb_network_offset(nskb) + offset);
> > 
> > +               skb_release_head_state(nskb);
> >                  __copy_skb_header(nskb, skb);
> > 
> >                 skb_headers_offset_update(nskb, skb_headroom(nskb) - skb_headroom(skb));
> > 
> > AFAICS we not only leak reference of extensions, but also skb->dst and skb->_nfct.
> 
> Would be nice if we would not need to drop the resources
> just to add them back again in the next line. But it is ok
> as a quick fix for the bug.

Yes, but are these the same resources?  AFAIU thats not the case, i.e.
the skb on fraglist can have different skb->{dst,extension,_nfct} data
than the skb head one, and we can't tell if that data is still valid
(rerouting for example).

Re: [PATCH net] udp: fix a skb extensions leak

[Steffen Klassert]

On Mon, Mar 30, 2020 at 04:11:47PM +0200, Florian Westphal wrote:
> Steffen Klassert <steffen.klassert@secunet.com> wrote:
> > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > > index 621b4479fee1..7e29590482ce 100644
> > > --- a/net/core/skbuff.c
> > > +++ b/net/core/skbuff.c
> > > @@ -3668,6 +3668,7 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,
> > > 
> > >                 skb_push(nskb, -skb_network_offset(nskb) + offset);
> > > 
> > > +               skb_release_head_state(nskb);
> > >                  __copy_skb_header(nskb, skb);
> > > 
> > >                 skb_headers_offset_update(nskb, skb_headroom(nskb) - skb_headroom(skb));
> > > 
> > > AFAICS we not only leak reference of extensions, but also skb->dst and skb->_nfct.
> > 
> > Would be nice if we would not need to drop the resources
> > just to add them back again in the next line. But it is ok
> > as a quick fix for the bug.
> 
> Yes, but are these the same resources?  AFAIU thats not the case, i.e.
> the skb on fraglist can have different skb->{dst,extension,_nfct} data
> than the skb head one, and we can't tell if that data is still valid
> (rerouting for example).

Some are the same, others not. Originally, I used
a subset of __copy_skb_header here. But decided then
to use __copy_skb_header to make sure I don't forget
anything. So this fix is ok for now.

Re: [PATCH net] udp: fix a skb extensions leak

[Xin Long]

On Mon, Mar 30, 2020 at 4:29 PM Steffen Klassert
<steffen.klassert@secunet.com> wrote:
>
> On Thu, Mar 26, 2020 at 05:06:25PM +0800, Xin Long wrote:
> > On udp rx path udp_rcv_segment() may do segment where the frag skbs
> > will get the header copied from the head skb in skb_segment_list()
> > by calling __copy_skb_header(), which could overwrite the frag skbs'
> > extensions by __skb_ext_copy() and cause a leak.
> >
> > This issue was found after loading esp_offload where a sec path ext
> > is set in the skb.
> >
> > On udp tx gso path, it works well as the frag skbs' extensions are
> > not set. So this issue should be fixed on udp's rx path only and
> > release the frag skbs' extensions before going to do segment.
>
> Are you sure that this affects only the RX path? What if such
> a packet is forwarded? Also, I think TCP has the same problem.
You're right, just confirm it exists on the forwarded path.
__copy_skb_header() is also called by skb_segment(), but
I don't have tests to reproduce it on other protocols like TCP.

>
> >
> > Reported-by: Xiumei Mu <xmu@redhat.com>
> > Fixes: cf329aa42b66 ("udp: cope with UDP GRO packet misdirection")
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> >  include/net/udp.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/include/net/udp.h b/include/net/udp.h
> > index e55d5f7..7bf0ca5 100644
> > --- a/include/net/udp.h
> > +++ b/include/net/udp.h
> > @@ -486,6 +486,10 @@ static inline struct sk_buff *udp_rcv_segment(struct sock *sk,
> >       if (skb->pkt_type == PACKET_LOOPBACK)
> >               skb->ip_summed = CHECKSUM_PARTIAL;
> >
> > +     if (skb_has_frag_list(skb) && skb_has_extensions(skb))
> > +             skb_walk_frags(skb, segs)
> > +                     skb_ext_put(segs);
>
> If a skb in the fraglist has a secpath, it is still valid.
> So maybe instead of dropping it here and assign the one
> from the head skb, we could just keep the secpath. But
> I don't know about other extensions. I've CCed Florian,
> he might know a bit more about other extensions. Also,
> it might be good to check if the extensions of the GRO
> packets are all the same before merging.
>
Not sure if we can improve __copy_skb_header() or add
a new function to copy these members ONLY when nskb's
are not set.

Re: [PATCH net] udp: fix a skb extensions leak

[Florian Westphal]

Xin Long <lucien.xin@gmail.com> wrote:
> On Mon, Mar 30, 2020 at 4:29 PM Steffen Klassert
> <steffen.klassert@secunet.com> wrote:
> >
> > On Thu, Mar 26, 2020 at 05:06:25PM +0800, Xin Long wrote:
> > > On udp rx path udp_rcv_segment() may do segment where the frag skbs
> > > will get the header copied from the head skb in skb_segment_list()
> > > by calling __copy_skb_header(), which could overwrite the frag skbs'
> > > extensions by __skb_ext_copy() and cause a leak.
> > >
> > > This issue was found after loading esp_offload where a sec path ext
> > > is set in the skb.
> > >
> > > On udp tx gso path, it works well as the frag skbs' extensions are
> > > not set. So this issue should be fixed on udp's rx path only and
> > > release the frag skbs' extensions before going to do segment.
> >
> > Are you sure that this affects only the RX path? What if such
> > a packet is forwarded? Also, I think TCP has the same problem.
> You're right, just confirm it exists on the forwarded path.
> __copy_skb_header() is also called by skb_segment(), but
> I don't have tests to reproduce it on other protocols like TCP.

skb_segment() is fine, either nskb is a new allocation or a clone
with head state already discarded.

> > If a skb in the fraglist has a secpath, it is still valid.
> > So maybe instead of dropping it here and assign the one
> > from the head skb, we could just keep the secpath. But
> > I don't know about other extensions. I've CCed Florian,
> > he might know a bit more about other extensions. Also,
> > it might be good to check if the extensions of the GRO
> > packets are all the same before merging.
> >
> Not sure if we can improve __copy_skb_header() or add
> a new function to copy these members ONLY when nskb's
> are not set.

I don't see how.

Re: [PATCH net] udp: fix a skb extensions leak

[Xin Long]

On Mon, Mar 30, 2020 at 9:28 PM Florian Westphal <fw@strlen.de> wrote:
>
> Xin Long <lucien.xin@gmail.com> wrote:
> > On udp rx path udp_rcv_segment() may do segment where the frag skbs
> > will get the header copied from the head skb in skb_segment_list()
> > by calling __copy_skb_header(), which could overwrite the frag skbs'
> > extensions by __skb_ext_copy() and cause a leak.
> >
> > This issue was found after loading esp_offload where a sec path ext
> > is set in the skb.
> >
> > On udp tx gso path, it works well as the frag skbs' extensions are
> > not set. So this issue should be fixed on udp's rx path only and
> > release the frag skbs' extensions before going to do segment.
> > Reported-by: Xiumei Mu <xmu@redhat.com>
> > Fixes: cf329aa42b66 ("udp: cope with UDP GRO packet misdirection")
>
> Hmm, I suspect this bug came in via
> 3a1296a38d0cf62bffb9a03c585cbd5dbf15d596 , net: Support GRO/GSO fraglist chaining.
>
> I suspect correct fix is:
>
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 621b4479fee1..7e29590482ce 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3668,6 +3668,7 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,
>
>                 skb_push(nskb, -skb_network_offset(nskb) + offset);
>
> +               skb_release_head_state(nskb);
>                  __copy_skb_header(nskb, skb);
>
>                 skb_headers_offset_update(nskb, skb_headroom(nskb) - skb_headroom(skb));
>
> AFAICS we not only leak reference of extensions, but also skb->dst and skb->_nfct.
Works for this issue.

Tested-by: Xin Long <lucien.xin@gmail.com>