[PATCH] Use of unreferenced ifa in in6

[Sergey Kandaurov]

On 4 January 2012 00:17, John Baldwin <jhb at freebsd.org> wrote:
> On Tuesday, January 03, 2012 2:36:25 pm Sergey Kandaurov wrote:
>> On 24 December 2011 00:08, John Baldwin <jhb at freebsd.org> wrote:
>> > The code to handle the SIOCGLIFADDR and SIOCDLIFADDR ioctls in
>> > in6_lifaddr_ioctl() does not grab a reference to an ifnet address structure
>> > that it uses after dropping the IF_ADDR_LOCK().  Based on other code that uses
>> > a similar pattern of finding an ifa while under the lock and then using it
>> > after dropping the lock, I believe it should be acquiring a reference on the
>> > ifa and then dropping that reference when it is done using the ifa.  This
>> > (untested) patch should fix this I believe:
>>
>> [Some thoughts on this.]
>>
>> FYI, a similar code exists in in_lifaddr_ioctl() under netinet/ which uses
>> an unreferenced ifa. Even when ifa reference is acquired, does this protect
>> ifa internals from concurrent changes? I would additionally lock ifa to
>> serialize multiple bcopy() operations. To do that, IFA_LOCK/UNLOCK() pair
>> exists to lock ifa with ifa_mtx. But there is only one place where such
>> locking is used explicitly. Initially IFA_LOCK/UNLOCK() were introduced in
>> 2002 and used implicitly in IFAREF()/IFAFREE() to lock up ifaddr reference
>> counts. Two years later ifa_mtx started to be used explicitly in one place
>> to protect SIOCSIFNAME in net/if.c:ifhwioctl(). In 8.0 they are removed in
>> favor of refcount(9), and IFAREF/IFAFREE() moved to ifa_ref()/ifa_free(),
>> and now as said in r194602: "The ifa_mtx is now used for exactly one ioctl,
>> and possibly should be removed."
>>
>> Now I'm losing the chain, sorry..
>
> Hmm, I'm not sure if ifa objects become immutable or not once they are
> referenced in the list.  Other places in the code seem to use the ifa
> without locking it though, merely obtaining a reference.

Yes, this is a main concern.

> The in.c code doesn't even grab the IF_ADDR_LOCK(). :(  The below patch
> should fix that and add the same fix as done to the in6.c code.
>
> Index: in.c
> ===================================================================
> --- in.c        (revision 229406)
> +++ in.c        (working copy)
> @@ -784,6 +784,7 @@ in_lifaddr_ioctl(struct socket *so, u_long cmd, ca
>                        }
>                }
>
> +               IF_ADDR_LOCK(ifp);
>                TAILQ_FOREACH(ifa, &ifp->if_addrhead, ifa_link) {
>                        if (ifa->ifa_addr->sa_family != AF_INET6)
>                                continue;
> @@ -794,6 +795,9 @@ in_lifaddr_ioctl(struct socket *so, u_long cmd, ca
>                        if (candidate.s_addr == match.s_addr)
>                                break;
>                }
> +               if (ifa != NULL)
> +                       ifa_ref(ifa);
> +               IF_ADDR_UNLOCK(ifp);
>                if (ifa == NULL)
>                        return (EADDRNOTAVAIL);
>                ia = (struct in_ifaddr *)ifa;
> @@ -812,6 +816,7 @@ in_lifaddr_ioctl(struct socket *so, u_long cmd, ca
>                                in_mask2len(&ia->ia_sockmask.sin_addr);
>
>                        iflr->flags = 0;        /*XXX*/
> +                       ifa_free(ifa);
>
>                        return (0);
>                } else {
> @@ -830,6 +835,7 @@ in_lifaddr_ioctl(struct socket *so, u_long cmd, ca
>                        }
>                        bcopy(&ia->ia_sockmask, &ifra.ifra_dstaddr,
>                                ia->ia_sockmask.sin_len);
> +                       ifa_free(ifa);
>
>                        return (in_control(so, SIOCDIFADDR, (caddr_t)&ifra,
>                            ifp, td));
>
>

With this patch in_lifaddr_ioctl() now looks more syntactically similar
to in6_lifaddr_ioctl(). They could look even more similar by eliminating
a lot of whitespace changes present here or there.

-- 
wbr,
pluknet