* Re: [PATCH net] udp: fix a skb extensions leak
2020-03-26 9:06 [PATCH net] udp: fix a skb extensions leak Xin Long
@ 2020-03-26 9:28 ` Xin Long
2020-03-30 4:54 ` David Miller
` (2 subsequent siblings)
3 siblings, 0 replies; 11+ messages in thread
From: Xin Long @ 2020-03-26 9:28 UTC (permalink / raw)
To: network dev; +Cc: davem, Paolo Abeni, Steffen Klassert
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] udp: fix a skb extensions leak
2020-03-26 9:06 [PATCH net] udp: fix a skb extensions leak Xin Long
2020-03-26 9:28 ` Xin Long
@ 2020-03-30 4:54 ` David Miller
2020-03-30 8:29 ` Steffen Klassert
2020-03-30 13:27 ` Florian Westphal
3 siblings, 0 replies; 11+ messages in thread
From: David Miller @ 2020-03-30 4:54 UTC (permalink / raw)
To: lucien.xin; +Cc: netdev, pabeni, steffen.klassert
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] udp: fix a skb extensions leak
2020-03-26 9:06 [PATCH net] udp: fix a skb extensions leak Xin Long
2020-03-26 9:28 ` Xin Long
2020-03-30 4:54 ` David Miller
@ 2020-03-30 8:29 ` Steffen Klassert
2020-03-30 16:13 ` Xin Long
2020-03-30 13:27 ` Florian Westphal
3 siblings, 1 reply; 11+ messages in thread
From: Steffen Klassert @ 2020-03-30 8:29 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, davem, Paolo Abeni, Florian Westphal
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] udp: fix a skb extensions leak
2020-03-30 8:29 ` Steffen Klassert
@ 2020-03-30 16:13 ` Xin Long
2020-03-30 16:13 ` Florian Westphal
0 siblings, 1 reply; 11+ messages in thread
From: Xin Long @ 2020-03-30 16:13 UTC (permalink / raw)
To: Steffen Klassert; +Cc: network dev, davem, Paolo Abeni, Florian Westphal
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] udp: fix a skb extensions leak
2020-03-30 16:13 ` Xin Long
@ 2020-03-30 16:13 ` Florian Westphal
0 siblings, 0 replies; 11+ messages in thread
From: Florian Westphal @ 2020-03-30 16:13 UTC (permalink / raw)
To: Xin Long
Cc: Steffen Klassert, network dev, davem, Paolo Abeni, 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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] udp: fix a skb extensions leak
2020-03-26 9:06 [PATCH net] udp: fix a skb extensions leak Xin Long
` (2 preceding siblings ...)
2020-03-30 8:29 ` Steffen Klassert
@ 2020-03-30 13:27 ` Florian Westphal
2020-03-30 13:45 ` Steffen Klassert
2020-03-30 16:14 ` Xin Long
3 siblings, 2 replies; 11+ messages in thread
From: Florian Westphal @ 2020-03-30 13:27 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, davem, Paolo Abeni, steffen.klassert
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.
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net] udp: fix a skb extensions leak
2020-03-30 13:27 ` Florian Westphal
@ 2020-03-30 13:45 ` Steffen Klassert
2020-03-30 14:11 ` Florian Westphal
2020-03-30 16:14 ` Xin Long
1 sibling, 1 reply; 11+ messages in thread
From: Steffen Klassert @ 2020-03-30 13:45 UTC (permalink / raw)
To: Florian Westphal; +Cc: Xin Long, network dev, davem, Paolo Abeni
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] udp: fix a skb extensions leak
2020-03-30 13:45 ` Steffen Klassert
@ 2020-03-30 14:11 ` Florian Westphal
2020-03-30 14:39 ` Steffen Klassert
0 siblings, 1 reply; 11+ messages in thread
From: Florian Westphal @ 2020-03-30 14:11 UTC (permalink / raw)
To: Steffen Klassert
Cc: Florian Westphal, Xin Long, network dev, davem, Paolo Abeni
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).
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] udp: fix a skb extensions leak
2020-03-30 14:11 ` Florian Westphal
@ 2020-03-30 14:39 ` Steffen Klassert
0 siblings, 0 replies; 11+ messages in thread
From: Steffen Klassert @ 2020-03-30 14:39 UTC (permalink / raw)
To: Florian Westphal; +Cc: Xin Long, network dev, davem, Paolo Abeni
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] udp: fix a skb extensions leak
2020-03-30 13:27 ` Florian Westphal
2020-03-30 13:45 ` Steffen Klassert
@ 2020-03-30 16:14 ` Xin Long
1 sibling, 0 replies; 11+ messages in thread
From: Xin Long @ 2020-03-30 16:14 UTC (permalink / raw)
To: Florian Westphal; +Cc: network dev, davem, Paolo Abeni, Steffen Klassert
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>
^ permalink raw reply [flat|nested] 11+ messages in thread