On Mon, Jan 25, 2021 at 06:17:02PM +0100, Alexandr Nedvedicky wrote:
> Hello,
>
> pf_route() might leak a refence to ifp.
oh no :(
> > Index: sys/net/pf.c
> > ===================================================================
> > RCS file: /cvs/src/sys/net/pf.c,v
> > retrieving revision 1.1101
> > diff -u -p -r1.1101 pf.c
> > --- sys/net/pf.c 19 Jan 2021 22:22:23 -0000 1.1101
> > +++ sys/net/pf.c 22 Jan 2021 07:33:31 -0000
>
> </snip>
>
> > @@ -5998,48 +5994,40 @@ pf_route(struct pf_pdesc *pd, struct pf_
> >
> > ip = mtod(m0, struct ip *);
> >
> </snip>
> > +
> > + ifp = if_get(rt->rt_ifidx);
> > if (ifp == NULL)
> > goto bad;
>
> here we get a reference to ifp.
yep.
> > @@ -6082,9 +6060,9 @@ pf_route(struct pf_pdesc *pd, struct pf_
> > */
> > if (ip->ip_off & htons(IP_DF)) {
> > ipstat_inc(ips_cantfrag);
> > - if (r->rt != PF_DUPTO)
> > + if (s->rt != PF_DUPTO)
> > pf_send_icmp(m0, ICMP_UNREACH, ICMP_UNREACH_NEEDFRAG,
> > - ifp->if_mtu, pd->af, r, pd->rdomain);
> > + ifp->if_mtu, pd->af, s->rule.ptr, pd->rdomain);
> > goto bad;
>
> here we do 'goto bad', which does not call if_put().
yes it does. the whole chunk with the diff applied is:
done:
if (s->rt != PF_DUPTO)
pd->m = NULL;
if_put(ifp);
rtfree(rt);
return;
bad:
m_freem(m0);
goto done;
}
bad drops the mbuf and then goes to done.
