netdev - Re: Leak in ipv6_gso

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEfhGizesRT9YOfKs09mbnA+PKnG5i2ERmPNyx_+Z6LZ1_bcFQ@mail.gmail.com>
Date:   Fri, 2 Jun 2017 14:25:15 -0400
From:   Craig Gallek <kraigatgoog@...il.com>
To:     David Miller <davem@...emloft.net>
Cc:     Ben Hutchings <ben@...adent.org.uk>,
        netdev <netdev@...r.kernel.org>
Subject: Re: Leak in ipv6_gso_segment()?

On Fri, Jun 2, 2017 at 2:05 PM, David Miller <davem@...emloft.net> wrote:
> From: Ben Hutchings <ben@...adent.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)