Date: Tue, 3 Jan 2012 13:52:44 -0500 From: John Baldwin <jhb@freebsd.org> To: "Bjoern A. Zeeb" <bz@freebsd.org> Cc: net@freebsd.org Subject: Re: [PATCH] Use of unreferenced ifa in in6 Message-ID: <201201031352.45069.jhb@freebsd.org> In-Reply-To: <F6ADC1D1-6C6B-4F62-A38C-853CBF127A83@freebsd.org> References: <201112231508.52861.jhb@freebsd.org> <F6ADC1D1-6C6B-4F62-A38C-853CBF127A83@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tuesday, January 03, 2012 12:35:30 pm Bjoern A. Zeeb wrote:
>
> On 23. Dec 2011, at 20:08 , John Baldwin 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:
>
> I almost assume it's been tested by now. From reading it looks right.
Hmm, I don't have a good way to test it. :( I've booted a GENERIC kernel
with it, but I don't have IPv6 setup for my test machines.
> /bz
>
> > 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);
> > }
> >
> >
> > --
> > John Baldwin
>
> --
> Bjoern A. Zeeb You have to have visions!
> It does not matter how good you are. It matters what good you do!
>
>
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201201031352.45069.jhb>