[PATCH] Use of unreferenced ifa in in6

[John Baldwin]

On Tuesday, January 03, 2012 3:44:50 pm Sergey Kandaurov wrote:
> On 4 January 2012 00:17, John Baldwin <jhb at freebsd.org> wrote:
> > On Tuesday, January 03, 2012 2:36:25 pm Sergey Kandaurov wrote:
> >> 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..
> >
> > 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.

Hmmm.  Actually, it seems to be a bit more broken.  Note that it is expecting
to get a sockaddr_in, but it is checking for AF_INET6, not AF_INET in its
loop.  That bug seems to go back to the original import from KAME.  I'm not
sure if the two can be merged since they work on different underyling data
structures though.

-- 
John Baldwin