Re: kern/49264: vlan(4): concurrent executions of ifconfig cause a fatal page fault

[Ryota Ozaki <ozaki-r%netbsd.org@localhost>]

After some investigation, I found a real problem is that
if_detach(*) deallocates data of an ifnet before removing the ifnet
from ifnet_list. Use-after-free can happen on the data of the
ifnet between the deallocations and the removal of the ifnet from
ifnet_list. I think we should remove the ifnet from ifnet_list
before deallocating the data of it. (FreeBSD does so.)

(*) http://nxr.netbsd.org/xref/src/sys/net/if.c#717

  ozaki-r


On Fri, Oct 10, 2014 at 4:07 PM, Ryota Ozaki <ozaki-r%netbsd.org@localhost> wrote:
> On Fri, Oct 10, 2014 at 4:00 PM, Manuel Bouyer <bouyer%antioche.eu.org@localhost> wrote:
>> On Fri, Oct 10, 2014 at 03:43:14PM +0900, Ryota Ozaki wrote:
>>> With the same configuration, I got another kind of fatal page
>>> faults (see backtraces below).
>>>
>>> In both cases, it seems that a ifnet data of vlan encounters
>>> use after free. I can work around the issue with this patch:
>>>
>>> diff --git a/sys/net/if_vlan.c b/sys/net/if_vlan.c
>>> index 70a5940..d6aac2c 100644
>>> --- a/sys/net/if_vlan.c
>>> +++ b/sys/net/if_vlan.c
>>> @@ -251,10 +251,10 @@ vlan_clone_destroy(struct ifnet *ifp)
>>>         s = splnet();
>>>         LIST_REMOVE(ifv, ifv_list);
>>>         vlan_unconfig(ifp);
>>> -       splx(s);
>>>
>>>         if_detach(ifp);
>>>         free(ifv, M_DEVBUF);
>>> +       splx(s);
>>>
>>>         return (0);
>>>  }
>>>
>>> I'm not sure if this fix is correct.
>>
>> At first glance, I think the splx(s) needs to be between if_detach()
>> and free().
>> if_detach() needs to be called at splnet() but free() doesn't.
>
> Sure :) Updated.
>
> diff --git a/sys/net/if_vlan.c b/sys/net/if_vlan.c
> index 70a5940..7b68ae9 100644
> --- a/sys/net/if_vlan.c
> +++ b/sys/net/if_vlan.c
> @@ -251,9 +251,9 @@ vlan_clone_destroy(struct ifnet *ifp)
>         s = splnet();
>         LIST_REMOVE(ifv, ifv_list);
>         vlan_unconfig(ifp);
> -       splx(s);
> -
>         if_detach(ifp);
> +       splx(s);
> +
>         free(ifv, M_DEVBUF);
>
>         return (0);
>
> Thanks,
>   ozaki-r
>
>>
>> --
>> Manuel Bouyer <bouyer%antioche.eu.org@localhost>
>>      NetBSD: 26 ans d'experience feront toujours la difference
>> --