[PATCH] Use of unreferenced ifa in in6

[Sergey Kandaurov]

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..


> Index: in6.c
> ===================================================================
> --- in6.c       (revision 228777)
> +++ in6.c       (working copy)
> @@ -1767,6 +1767,8 @@ in6_lifaddr_ioctl(struct socket *so, u_long cmd, c
>                        if (IN6_ARE_ADDR_EQUAL(&candidate, &match))
>                                break;
>                }
> +               if (ifa != NULL)
> +                       ifa_ref(ifa);
>                IF_ADDR_UNLOCK(ifp);
>                if (!ifa)
>                        return EADDRNOTAVAIL;
> @@ -1779,16 +1781,20 @@ in6_lifaddr_ioctl(struct socket *so, u_long cmd, c
>                        bcopy(&ia->ia_addr, &iflr->addr, ia->ia_addr.sin6_len);
>                        error = sa6_recoverscope(
>                            (struct sockaddr_in6 *)&iflr->addr);
> -                       if (error != 0)
> +                       if (error != 0) {
> +                               ifa_free(ifa);
>                                return (error);
> +                       }
>
>                        if ((ifp->if_flags & IFF_POINTOPOINT) != 0) {
>                                bcopy(&ia->ia_dstaddr, &iflr->dstaddr,
>                                    ia->ia_dstaddr.sin6_len);
>                                error = sa6_recoverscope(
>                                    (struct sockaddr_in6 *)&iflr->dstaddr);
> -                               if (error != 0)
> +                               if (error != 0) {
> +                                       ifa_free(ifa);
>                                        return (error);
> +                               }
>                        } else
>                                bzero(&iflr->dstaddr, sizeof(iflr->dstaddr));
>
> @@ -1796,6 +1802,7 @@ in6_lifaddr_ioctl(struct socket *so, u_long cmd, c
>                            in6_mask2len(&ia->ia_prefixmask.sin6_addr, NULL);
>
>                        iflr->flags = ia->ia6_flags;    /* XXX */
> +                       ifa_free(ifa);
>
>                        return 0;
>                } else {
> @@ -1819,6 +1826,7 @@ in6_lifaddr_ioctl(struct socket *so, u_long cmd, c
>                            ia->ia_prefixmask.sin6_len);
>
>                        ifra.ifra_flags = ia->ia6_flags;
> +                       ifa_free(ifa);
>                        return in6_control(so, SIOCDIFADDR_IN6, (caddr_t)&ifra,
>                            ifp, td);
>                }

-- 
wbr,
pluknet