Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 3 Aug 2015 08:52:42 +0200
From:      =?UTF-8?Q?Ermal_Lu=C3=A7i?= <eri@freebsd.org>
To:        Mark Johnston <markj@freebsd.org>
Cc:        src-committers@freebsd.org, svn-src-all@freebsd.org,  svn-src-head@freebsd.org
Subject:   Re: svn commit: r286028 - head/sys/netinet
Message-ID:  <CAPBZQG3HFMBFc-ckCKmPb%2B3ajj=HA8n_sf7=5o=EdOqo%2BFPCSQ@mail.gmail.com>
In-Reply-To: <20150802184021.GB59626@raichu>
References:  <201507291804.t6TI42iH065403@repo.freebsd.org> <20150802184021.GB59626@raichu>

next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, Aug 2, 2015 at 8:40 PM, Mark Johnston <markj@freebsd.org> wrote:

> On Wed, Jul 29, 2015 at 06:04:02PM +0000, Ermal Lu=C3=A7i 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
> >
> =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=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D
> > --- 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 =3D NULL;
> > +     struct in_addr odst;
> > +     struct ip *ip;
> > +
> > +     ip =3D mtod(m, struct ip *);
> > +
> > +     /* Run through list of hooks for output packets. */
> > +     odst.s_addr =3D ip->ip_dst.s_addr;
> > +     *error =3D pfil_run_hooks(&V_inet_pfil_hook, &m, ifp, PFIL_OUT, i=
np);
> > +     if ((*error) !=3D 0 || m =3D=3D 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 =3D NULL;
> +       struct mbuf *m;
>         struct in_addr odst;
>         struct ip *ip;
>
> +       m =3D *mp;
>         ip =3D mtod(m, struct ip *);
>
>         /* Run through list of hooks for output packets. */
>         odst.s_addr =3D ip->ip_dst.s_addr;
> -       *error =3D pfil_run_hooks(&V_inet_pfil_hook, &m, ifp, PFIL_OUT, i=
np);
> +       *error =3D pfil_run_hooks(&V_inet_pfil_hook, mp, ifp, PFIL_OUT, i=
np);
> +       m =3D *mp;
>         if ((*error) !=3D 0 || m =3D=3D 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;
>
>


--=20
Ermal



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAPBZQG3HFMBFc-ckCKmPb%2B3ajj=HA8n_sf7=5o=EdOqo%2BFPCSQ>