Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 25 May 2012 13:25:32 +0200 (CEST)
From:      Joerg Pulz <Joerg.Pulz@frm2.tum.de>
To:        Daniel Hartmeier <daniel@benzedrine.cx>
Cc:        bug-followup@freebsd.org, freebsd-pf@freebsd.org
Subject:   Re: kern/168190: [pf] panic when using pf and route-to (maybe: bad fragment handling?)
Message-ID:  <alpine.BSF.2.00.1205251319280.89783@unqrf.nqzva.sez2>
In-Reply-To: <20120525091627.GA27514@insomnia.benzedrine.cx>
References:  <201205250730.q4P7UGu0006036@freefall.freebsd.org> <20120525091627.GA27514@insomnia.benzedrine.cx>

next in thread | previous in thread | raw e-mail | index | archive | help
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1


On Fri, 25 May 2012, Daniel Hartmeier wrote:

> On Fri, May 25, 2012 at 07:30:16AM +0000, Joerg Pulz wrote:
>
>>  the system is still running without panic, but i found the following log
>>  entries from last night:
>>
>>  May 24 23:28:57 charon kernel: pf_route: m0->m_len < sizeof(struct ip)
>>  May 24 23:28:57 charon kernel: pf_route: m0->m_len < sizeof(struct ip)
>>
>>  Do you think that this may be related to the panics?
>>  I've found this error message two times in contrib/pf/net/pf.c.
>>  I can't say which of them or both have printed the message.
>
> Yes, this could possibly explain it.
>
> All pfil consumers assume that the IP header is one continuous memory
> region in the mbuf, without verifying this or correcting it (with
> m_pullup() or such) if wrong.
>
> pf: pf_check_in() in sys/contrib/pf/net/pf_ioctl.c
> 	h = mtod(*m, struct ip *);
> 	access h->ip_len
>
> ipfw: ipfw_check_hook() in sys/netinet/ipfw/ip_fw_pfil.c
> 	SET_NET_IPLEN(mtod(*m0, struct ip *));
>
> ipfilter: fr_check_wrapper() in sys/contrib/ipfilter/netinet/ip_fil_freebsd.c
> 	struct ip *ip = mtod(*mp, struct ip *);
> 	access ip->ip_hl
>
> Hence, the caller of pfil_run_hooks() must ensure this before the call.
> ip_input() does, but there are operations that might violate the
> condition again.
>
> If the IP header is not continuous in the first mbuf, doing
>
> 	struct ip *ip = mtod(m, struct ip *);
> 	ip->ip_len = ntohs(ip->ip_len);
>
> will read (and write) unrelated memory.
>
> If, later, something does call m_pullup(), ip_len will get 'replaced'
> again. This could lead to some byte order swaps getting undone.
>
> I'm not sure what the proper action is here, i.e. should we be surprised
> that an mbuf with such a small m_len is found, and track down how it
> was produced, or should the pfil code simply expect this?
>
> I'd probably add a sanity check to pfil_run_hooks(), like
>
> --- sys/net/pfil.c      23 Sep 2011 00:51:37 -0000      1.19.2.1
> +++ sys/net/pfil.c      25 May 2012 09:10:27 -0000
> @@ -46,6 +46,8 @@
>
> #include <net/if.h>
> #include <net/pfil.h>
> +#include <netinet/in.h>
> +#include <netinet/ip.h>
>
> static struct mtx pfil_global_lock;
>
> @@ -74,15 +76,21 @@
>        struct mbuf *m = *mp;
>        int rv = 0;
>
> +       if (m->m_pkthdr.len < sizeof(struct ip) ||
> +           m->m_len < sizeof(struct ip))
> +               panic("pfil_run_hooks: m->m_pkthdr.len %d, m->m_len %d",
> +                   (int)m->m_pkthdr.len, (int)m->m_len);
>        PFIL_RLOCK(ph, &rmpt);
>        KASSERT(ph->ph_nhooks >= 0, ("Pfil hook count dropped < 0"));
>        for (pfh = pfil_hook_get(dir, ph); pfh != NULL;
>             pfh = TAILQ_NEXT(pfh, pfil_link)) {
>                if (pfh->pfil_func != NULL) {
> +                       ASSERT_HOST_BYTE_ORDER(m);
>                        rv = (*pfh->pfil_func)(pfh->pfil_arg, &m, ifp, dir,
>                            inp);
>                        if (rv != 0 || m == NULL)
>                                break;
> +                       ASSERT_HOST_BYTE_ORDER(m);
>                }
>        }
>        PFIL_RUNLOCK(ph, &rmpt);
>
> Then when it will panic (instead of just the pf_route() message), the
> stack trace could help.
>
> This might require several iterations, adding such checks all around the
> place. The cause might be some mbuf operation done in ipsec, for some
> edge case, explaining why it occurs so rarely...
>
> If I could easily reproduce it locally, I'd probably do it, but it's
> your machine that crashes all the time, so it's your call :)

Daniel,

thanks for your explaining.

As i said, i will do everything to track this down to the real bottom to 
get it fixed forever.

Your proposed changes are in and the system is running with the rebuilt 
kernel. Now it's time to wait and see what happens.

Kind regards
Joerg

- -- 
The beginning is the most important part of the work.
 				-Plato
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.18 (FreeBSD)

iD8DBQFPv2wvSPOsGF+KA+MRAp1DAKCp2KZdPKBdsR5PUbK3ixqXqFdi9ACfbfvd
vez+bq9X5lMjbdysCXy+stU=
=tvtF
-----END PGP SIGNATURE-----



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?alpine.BSF.2.00.1205251319280.89783>