Date: Tue, 3 Jan 2012 23:44:50 +0300 From: Sergey Kandaurov <pluknet@freebsd.org> To: John Baldwin <jhb@freebsd.org> Cc: Bjoern Zeeb <bz@freebsd.org>, net@freebsd.org Subject: Re: [PATCH] Use of unreferenced ifa in in6 Message-ID: <CAE-mSOLhJ0O5DCjje%2BhHj1%2BO6c=MKR=7K0p5trj9T_XkPgWgng@mail.gmail.com> In-Reply-To: <201201031517.36251.jhb@freebsd.org> References: <201112231508.52861.jhb@freebsd.org> <CAE-mSOJ40XE3GH4aRw2coJz8LZ61dQ7PDJWWqK64H6cP5Updrw@mail.gmail.com> <201201031517.36251.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 4 January 2012 00:17, John Baldwin <jhb@freebsd.org> wrote:
> 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.
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAE-mSOLhJ0O5DCjje%2BhHj1%2BO6c=MKR=7K0p5trj9T_XkPgWgng>