svn commit: r286028 - head/sys/netinet

Ermal Luçi eri at freebsd.org
Mon Aug 3 06:52:43 UTC 2015


On Sun, Aug 2, 2015 at 8:40 PM, Mark Johnston <markj at freebsd.org> wrote:

> On Wed, Jul 29, 2015 at 06:04:02PM +0000, Ermal Luçi wrote:
> > Author: eri
> > Date: Wed Jul 29 18:04:01 2015
> > New Revision: 286028
> > URL: https://svnweb.freebsd.org/changeset/base/286028
> >
> > Log:
> >   ip_output normalization and fixes
> >
> >   ip_output has a big chunk of code used to handle special cases with
> pfil consumers which also forces a reloop on it.
> >   Gather all this code together to make it readable and properly handle
> the reloop cases.
> >
> >   Some of the issues identified:
> >
> >   M_IP_NEXTHOP is not handled properly in existing code.
> >   route reference leaking is possible with in FIB number change
> >   route flags checking is not consistent in the function
> >
> >   Differential Revision:      https://reviews.freebsd.org/D3022
> >   Reviewed by:        gnn
> >   Approved by:        gnn(mentor)
> >   MFC after:  4 weeks
> >
> > Modified:
> >   head/sys/netinet/ip_output.c
> >
> > Modified: head/sys/netinet/ip_output.c
> >
> ==============================================================================
> > --- head/sys/netinet/ip_output.c      Wed Jul 29 17:59:13 2015
> (r286027)
> > +++ head/sys/netinet/ip_output.c      Wed Jul 29 18:04:01 2015
> (r286028)
> > @@ -106,6 +106,94 @@ static void      ip_mloopback
> >  extern int in_mcast_loop;
> >  extern       struct protosw inetsw[];
> >
> > +static inline int
> > +ip_output_pfil(struct mbuf *m, struct ifnet *ifp, struct inpcb *inp,
> > +     struct sockaddr_in *dst, int *fibnum, int *error)
> > +{
> > +     struct m_tag *fwd_tag = NULL;
> > +     struct in_addr odst;
> > +     struct ip *ip;
> > +
> > +     ip = mtod(m, struct ip *);
> > +
> > +     /* Run through list of hooks for output packets. */
> > +     odst.s_addr = ip->ip_dst.s_addr;
> > +     *error = pfil_run_hooks(&V_inet_pfil_hook, &m, ifp, PFIL_OUT, inp);
> > +     if ((*error) != 0 || m == NULL)
> > +             return 1; /* Finished */
> > +
>
> This can result in a use-after-free in ip_output() if a pfil hook
> consumes the first mbuf in the chain. This happens for example when ipfw
> nat is in use: m_megapullup() copies the input packet into a single
> cluster, which is returned above. However, ip_output() will continue to
> reference the original mbuf chain.
>
> The patch below fixes the problem for me.
>
>
Good catch just push it in.
Ok for me.


> Thanks,
> -Mark
>
> diff --git a/sys/netinet/ip_output.c b/sys/netinet/ip_output.c
> index 0790777..086a8c9 100644
> --- a/sys/netinet/ip_output.c
> +++ b/sys/netinet/ip_output.c
> @@ -107,18 +107,21 @@ extern int in_mcast_loop;
>  extern struct protosw inetsw[];
>
>  static inline int
> -ip_output_pfil(struct mbuf *m, struct ifnet *ifp, struct inpcb *inp,
> -       struct sockaddr_in *dst, int *fibnum, int *error)
> +ip_output_pfil(struct mbuf **mp, struct ifnet *ifp, struct inpcb *inp,
> +    struct sockaddr_in *dst, int *fibnum, int *error)
>  {
>         struct m_tag *fwd_tag = NULL;
> +       struct mbuf *m;
>         struct in_addr odst;
>         struct ip *ip;
>
> +       m = *mp;
>         ip = mtod(m, struct ip *);
>
>         /* Run through list of hooks for output packets. */
>         odst.s_addr = ip->ip_dst.s_addr;
> -       *error = pfil_run_hooks(&V_inet_pfil_hook, &m, ifp, PFIL_OUT, inp);
> +       *error = pfil_run_hooks(&V_inet_pfil_hook, mp, ifp, PFIL_OUT, inp);
> +       m = *mp;
>         if ((*error) != 0 || m == NULL)
>                 return 1; /* Finished */
>
> @@ -552,7 +555,7 @@ sendit:
>
>         /* Jump over all PFIL processing if hooks are not active. */
>         if (PFIL_HOOKED(&V_inet_pfil_hook)) {
> -               switch (ip_output_pfil(m, ifp, inp, dst, &fibnum, &error))
> {
> +               switch (ip_output_pfil(&m, ifp, inp, dst, &fibnum,
> &error)) {
>                 case 1: /* Finished */
>                         goto done;
>
>


-- 
Ermal


More information about the svn-src-all mailing list