netdev - Re: vxlan: use after free error

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]

Date:   Mon, 29 May 2017 11:37:22 -0700
From:   Roopa Prabhu <roopa@...ulusnetworks.com>
To:     Mark Bloch <markb@...lanox.com>
Cc:     "davem@...emloft.net" <davem@...emloft.net>,
        Jiri Benc <jbenc@...hat.com>, pravin shelar <pshelar@....org>,
        Alexander Duyck <aduyck@...antis.com>,
        Nicolas Dichtel <nicolas.dichtel@...nd.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Balki Raman <ramanb@...ulusnetworks.com>
Subject: Re: vxlan: use after free error

On Sun, May 28, 2017 at 11:28 PM, Mark Bloch <markb@...lanox.com> wrote:
> Hi Roopa,
>
> On 29/05/2017 05:50, Roopa Prabhu wrote:
>> On Sun, May 28, 2017 at 3:49 AM, Mark Bloch <markb@...lanox.com> wrote:

[snip]

>>
>> From: Balakrishnan Raman <ramanb@...ulusnetworks.com>
>>
>> Date: Sun, 28 May 2017 19:34:25 -0700
>>
>> Subject: [PATCH net-next] vxlan: remove vxlan device from socket's device
>>
>>  list in vxlan_stop
>>
>>
>> Signed-off-by: Balakrishnan Raman <ramanb@...ulusnetworks.com>
>>
>> Signed-off-by: Roopa Prabhu <roopa@...ulusnetworks.com>
>>
>> ---
>>
>>  drivers/net/vxlan.c | 13 ++++++++++++-
>>
>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>
>>
>> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
>>
>> index 328b471..26dac52 100644
>>
>> --- a/drivers/net/vxlan.c
>>
>> +++ b/drivers/net/vxlan.c
>>
>> @@ -2352,6 +2352,16 @@ static void vxlan_vs_add_dev(struct vxlan_sock
>> *vs, struct vxlan_dev *vxlan)
>>
>>         spin_unlock(&vn->sock_lock);
>>
>>  }
>>
>>
>> +static void vxlan_vs_del_dev(struct vxlan_dev *vxlan)
>>
>> +{
>>
>> +       struct vxlan_net *vn = net_generic(vxlan->net, vxlan_net_id);
>>
>> +
>>
>> +       spin_lock(&vn->sock_lock);
>>
>> +       if (!hlist_unhashed(&vxlan->hlist))
>
> We are calling this from vxlan_stop(), which means we did vxlan_open(), there
> we do vxlan_sock_add() and added the vxlan to the socket. so the check
> is unnecessary and we can just call hlist_del_init_rcu() directly.

What you say looks correct..., but does not hurt to leave this check in there..
given rest of the changes you are proposing below.

Looking at git blame, this check was added for OVS in dellink...but it
could have been because
 it was being called before stop in dellink.

>
>>
>> +               hlist_del_init_rcu(&vxlan->hlist);
>>
>> +       spin_unlock(&vn->sock_lock);
>>
>> +}
>>
>> +
>>
>>  /* Setup stats when device is created */
>>
>>  static int vxlan_init(struct net_device *dev)
>>
>>  {
>>
>> @@ -2443,6 +2453,7 @@ static int vxlan_stop(struct net_device *dev)
>>
>>         del_timer_sync(&vxlan->age_timer);
>>
>>
>>         vxlan_flush(vxlan, false);
>>
>> +       vxlan_vs_del_dev(vxlan);
>
> In my patch I've added the code inside vxlan_sock_release()
> after we do:
>         rcu_assign_pointer(vxlan->vn6_sock, NULL);
>         rcu_assign_pointer(vxlan->vn4_sock, NULL);
>         synchronize_net();
> this way we make sure we are done handling packets that may access the
> vxlan struct. seems cleaner to me, what do you think?

sure, that works. But, lets keep the function vxlan_vs_del_dev(vxlan)
for symmetry with
vxlan_vs_add_dev(vxlan) and call it from vxlan_sock_release().

>
>>
>>         vxlan_sock_release(vxlan);
>>
>>
>>         return ret;
>>
>>
>> @@ -3292,7 +3303,7 @@ static void vxlan_dellink(struct net_device
>> *dev, struct list_head *head)
>>
>>
>>         spin_lock(&vn->sock_lock);
>>
>>         if (!hlist_unhashed(&vxlan->hlist))
>>
>> -               hlist_del_rcu(&vxlan->hlist);
>>
>> +               hlist_del_init_rcu(&vxlan->hlist);
>>
>>         spin_unlock(&vn->sock_lock);
>>
>
> in vxlan_dellink() we call unregister_netdevice_queue(). which means if
> the interface is open, the kernel would stop it. (and we'll hit the code in vxlan_stop()).
> We can remove this entire block of code it seems.

That seems right. It does look redundant if we hit the same code via
vxlan_stop during dellink.

This code is also hit via the OVS path, and i don't see a problem with
your changes and analysis but i am not too familiar with the ovs call
path. I see that the relevant developers are CC'ed.

thanks.