| Message ID | e17fe23a0a5f652866ec623ef0cde1e6ef5dbcf5.1585213585.git.lucien.xin@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] udp: fix a skb extensions leak | expand |
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 >
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.
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.
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.
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.
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).
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.
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.
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.
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>
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 */
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(+)