* Leak in ipv6_gso_segment()?
@ 2017-05-31 12:26 Ben Hutchings
2017-06-02 18:05 ` David Miller
0 siblings, 1 reply; 5+ messages in thread
From: Ben Hutchings @ 2017-05-31 12:26 UTC (permalink / raw)
To: Craig Gallek, David S. Miller; +Cc: netdev
[-- Attachment #1: Type: text/plain, Size: 337 bytes --]
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.
Ben.
--
Ben Hutchings
Lowery's Law:
If it jams, force it. If it breaks, it needed replacing anyway.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Leak in ipv6_gso_segment()?
2017-05-31 12:26 Leak in ipv6_gso_segment()? Ben Hutchings
@ 2017-06-02 18:05 ` David Miller
2017-06-02 18:25 ` Craig Gallek
0 siblings, 1 reply; 5+ messages in thread
From: David Miller @ 2017-06-02 18:05 UTC (permalink / raw)
To: ben; +Cc: kraig, netdev
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.
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)
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: Leak in ipv6_gso_segment()?
2017-06-02 18:05 ` David Miller
@ 2017-06-02 18:25 ` Craig Gallek
2017-06-02 19:27 ` Craig Gallek
0 siblings, 1 reply; 5+ messages in thread
From: Craig Gallek @ 2017-06-02 18:25 UTC (permalink / raw)
To: David Miller; +Cc: Ben Hutchings, netdev
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)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Leak in ipv6_gso_segment()?
2017-06-02 18:25 ` Craig Gallek
@ 2017-06-02 19:27 ` Craig Gallek
2017-06-05 1:41 ` David Miller
0 siblings, 1 reply; 5+ messages in thread
From: Craig Gallek @ 2017-06-02 19:27 UTC (permalink / raw)
To: David Miller; +Cc: Ben Hutchings, netdev
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).
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Leak in ipv6_gso_segment()?
2017-06-02 19:27 ` Craig Gallek
@ 2017-06-05 1:41 ` David Miller
0 siblings, 0 replies; 5+ messages in thread
From: David Miller @ 2017-06-05 1:41 UTC (permalink / raw)
To: kraigatgoog; +Cc: ben, netdev
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-06-05 1:41 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-31 12:26 Leak in ipv6_gso_segment()? Ben Hutchings
2017-06-02 18:05 ` David Miller
2017-06-02 18:25 ` Craig Gallek
2017-06-02 19:27 ` Craig Gallek
2017-06-05 1:41 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.