| Message ID | 20170602.140506.1833605091602891315.davem@davemloft.net |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, Jun 2, 2017 at 2:05 PM, David Miller <davem@davemloft.net> wrote: > From: Ben Hutchings <ben@decadent.org.uk> > Date: Wed, 31 May 2017 13:26:02 +0100 > >> If I'm not mistaken, ipv6_gso_segment() now leaks segs if >> ip6_find_1stfragopt() fails. I'm not sure whether the fix would be as >> simple as adding a kfree_skb(segs) or whether more complex cleanup is >> needed. > > I think we need to use kfree_skb_list(), like the following. I think this is problematic as well. ipv6_gso_segment could previously return errors, in which case the caller uses kfree_skb (ex validate_xmit_skb() -> skb_gso_segment -> ... callbacks.gso_segment()). Having the kfree_skb_list here would cause a double free if I'm reading this correctly. My first guess was going to be skb_gso_error_unwind(), but I'm still trying to understand that code... Sorry again for the fallout from this bug fix. This is my first time down this code path and I clearly didn't understand it fully :/ > Can someone please audit and review this for me? We need to > get this to -stable. > > Thanks. > > diff --git a/net/ipv6/ip6_offload.c b/net/ipv6/ip6_offload.c > index 280268f..cdb3728 100644 > --- a/net/ipv6/ip6_offload.c > +++ b/net/ipv6/ip6_offload.c > @@ -116,8 +116,10 @@ static struct sk_buff *ipv6_gso_segment(struct sk_buff *skb, > > if (udpfrag) { > int err = ip6_find_1stfragopt(skb, &prevhdr); > - if (err < 0) > + if (err < 0) { > + kfree_skb_list(segs); > return ERR_PTR(err); > + } > fptr = (struct frag_hdr *)((u8 *)ipv6h + err); > fptr->frag_off = htons(offset); > if (skb->next)
On Fri, Jun 2, 2017 at 2:25 PM, Craig Gallek <kraigatgoog@gmail.com> wrote: > On Fri, Jun 2, 2017 at 2:05 PM, David Miller <davem@davemloft.net> wrote: >> From: Ben Hutchings <ben@decadent.org.uk> >> Date: Wed, 31 May 2017 13:26:02 +0100 >> >>> If I'm not mistaken, ipv6_gso_segment() now leaks segs if >>> ip6_find_1stfragopt() fails. I'm not sure whether the fix would be as >>> simple as adding a kfree_skb(segs) or whether more complex cleanup is >>> needed. >> >> I think we need to use kfree_skb_list(), like the following. > I think this is problematic as well. ipv6_gso_segment could > previously return errors, in which case the caller uses kfree_skb (ex > validate_xmit_skb() -> skb_gso_segment -> ... > callbacks.gso_segment()). Having the kfree_skb_list here would cause > a double free if I'm reading this correctly. > > My first guess was going to be skb_gso_error_unwind(), but I'm still > trying to understand that code... > > Sorry again for the fallout from this bug fix. This is my first time > down this code path and I clearly didn't understand it fully :/ Ok, I take it back. I believe your kfree_skb_list suggestion is correct. I was assuming that skb_segment consumed the original skb upon successful segmentation. It does not. This is exactly why validate_xmit_skb calls consume_skb when segments are returned. Further, there is at least one existing example of kfree_skb_list in a similar post-semgent cleanup path (esp6_gso_segment).
From: Craig Gallek <kraigatgoog@gmail.com> Date: Fri, 2 Jun 2017 15:27:41 -0400 > On Fri, Jun 2, 2017 at 2:25 PM, Craig Gallek <kraigatgoog@gmail.com> wrote: >> On Fri, Jun 2, 2017 at 2:05 PM, David Miller <davem@davemloft.net> wrote: >>> From: Ben Hutchings <ben@decadent.org.uk> >>> Date: Wed, 31 May 2017 13:26:02 +0100 >>> >>>> If I'm not mistaken, ipv6_gso_segment() now leaks segs if >>>> ip6_find_1stfragopt() fails. I'm not sure whether the fix would be as >>>> simple as adding a kfree_skb(segs) or whether more complex cleanup is >>>> needed. >>> >>> I think we need to use kfree_skb_list(), like the following. >> I think this is problematic as well. ipv6_gso_segment could >> previously return errors, in which case the caller uses kfree_skb (ex >> validate_xmit_skb() -> skb_gso_segment -> ... >> callbacks.gso_segment()). Having the kfree_skb_list here would cause >> a double free if I'm reading this correctly. >> >> My first guess was going to be skb_gso_error_unwind(), but I'm still >> trying to understand that code... >> >> Sorry again for the fallout from this bug fix. This is my first time >> down this code path and I clearly didn't understand it fully :/ > > Ok, I take it back. I believe your kfree_skb_list suggestion is correct. > > I was assuming that skb_segment consumed the original skb upon > successful segmentation. It does not. This is exactly why > validate_xmit_skb calls consume_skb when segments are returned. > Further, there is at least one existing example of kfree_skb_list in a > similar post-semgent cleanup path (esp6_gso_segment). Great, thanks for reviewing. I've pushed this into 'net' and queued it up for -stable.
diff --git a/net/ipv6/ip6_offload.c b/net/ipv6/ip6_offload.c index 280268f..cdb3728 100644 --- a/net/ipv6/ip6_offload.c +++ b/net/ipv6/ip6_offload.c @@ -116,8 +116,10 @@ static struct sk_buff *ipv6_gso_segment(struct sk_buff *skb, if (udpfrag) { int err = ip6_find_1stfragopt(skb, &prevhdr); - if (err < 0) + if (err < 0) { + kfree_skb_list(segs); return ERR_PTR(err); + } fptr = (struct frag_hdr *)((u8 *)ipv6h + err); fptr->frag_off = htons(offset); if (skb->next)