Date: Tue, 3 Jan 2012 22:36:25 +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-mSOJ40XE3GH4aRw2coJz8LZ61dQ7PDJWWqK64H6cP5Updrw@mail.gmail.com> In-Reply-To: <201112231508.52861.jhb@freebsd.org> References: <201112231508.52861.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 structu=
re
> that it uses after dropping the IF_ADDR_LOCK(). =A0Based on other code th=
at uses
> a similar pattern of finding an ifa while under the lock and then using i=
t
> 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. =A0Th=
is
> (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
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- in6.c =A0 =A0 =A0 (revision 228777)
> +++ in6.c =A0 =A0 =A0 (working copy)
> @@ -1767,6 +1767,8 @@ in6_lifaddr_ioctl(struct socket *so, u_long cmd, c
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (IN6_ARE_ADDR_EQUAL(&ca=
ndidate, &match))
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0break;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (ifa !=3D NULL)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ifa_ref(ifa);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0IF_ADDR_UNLOCK(ifp);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (!ifa)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return EADDRNOTAVAIL;
> @@ -1779,16 +1781,20 @@ in6_lifaddr_ioctl(struct socket *so, u_long cmd, =
c
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0bcopy(&ia->ia_addr, &iflr-=
>addr, ia->ia_addr.sin6_len);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0error =3D sa6_recoverscope=
(
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(struct sockaddr_i=
n6 *)&iflr->addr);
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (error !=3D 0)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (error !=3D 0) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ifa_free(if=
a);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return (er=
ror);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if ((ifp->if_flags & IFF_P=
OINTOPOINT) !=3D 0) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0bcopy(&ia-=
>ia_dstaddr, &iflr->dstaddr,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ia=
->ia_dstaddr.sin6_len);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0error =3D =
sa6_recoverscope(
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(s=
truct sockaddr_in6 *)&iflr->dstaddr);
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (error !=
=3D 0)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (error !=
=3D 0) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 ifa_free(ifa);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0return (error);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} else
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0bzero(&ifl=
r->dstaddr, sizeof(iflr->dstaddr));
>
> @@ -1796,6 +1802,7 @@ in6_lifaddr_ioctl(struct socket *so, u_long cmd, c
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0in6_mask2len(&ia->=
ia_prefixmask.sin6_addr, NULL);
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0iflr->flags =3D ia->ia6_fl=
ags; =A0 =A0/* XXX */
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ifa_free(ifa);
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return 0;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} else {
> @@ -1819,6 +1826,7 @@ in6_lifaddr_ioctl(struct socket *so, u_long cmd, c
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ia->ia_prefixmask.=
sin6_len);
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ifra.ifra_flags =3D ia->ia=
6_flags;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ifa_free(ifa);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return in6_control(so, SIO=
CDIFADDR_IN6, (caddr_t)&ifra,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ifp, td);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
--=20
wbr,
pluknet
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAE-mSOJ40XE3GH4aRw2coJz8LZ61dQ7PDJWWqK64H6cP5Updrw>