Date: Tue, 3 Jan 2012 15:17:36 -0500 From: John Baldwin <jhb@freebsd.org> To: Sergey Kandaurov <pluknet@freebsd.org> Cc: Bjoern Zeeb <bz@freebsd.org>, net@freebsd.org Subject: Re: [PATCH] Use of unreferenced ifa in in6 Message-ID: <201201031517.36251.jhb@freebsd.org> In-Reply-To: <CAE-mSOJ40XE3GH4aRw2coJz8LZ61dQ7PDJWWqK64H6cP5Updrw@mail.gmail.com> References: <201112231508.52861.jhb@freebsd.org> <CAE-mSOJ40XE3GH4aRw2coJz8LZ61dQ7PDJWWqK64H6cP5Updrw@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tuesday, January 03, 2012 2:36:25 pm Sergey Kandaurov wrote:
> On 24 December 2011 00:08, John Baldwin <jhb@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.
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));
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201201031517.36251.jhb>