On 14/06/16(Tue) 20:10, Florian Obser wrote:
> On Tue, Jun 14, 2016 at 06:26:00PM +0200, Martin Pieuchot wrote:
> > On 14/06/16(Tue) 15:18, Florian Obser wrote:
> > > Hi,
> > > I'm seeing this panic on my v6 gateway running in a vm (don't ask):
> > > It has a v6 tunnel via HE on gif0.
> > >
> > > I hope I copied all relevant information, if not, my appologies, I'm
> > > in a hurry currently, please just ask for more.
> > >
> > > I will probably investigate more when I'm home :)
> > >
> > > panic: trap type 6, code=0, pc=ffffffff812fe70f
> > > Starting stack trace...
> > > panic() at panic+0x10b
> > > trap() at trap+0x7b8
> > > --- trap (number 6) ---
> > > ip6_output_ipsec_lookup() at ip6_output_ipsec_lookup+0x6f
> > > ip6_output() at ip6_output+0x21c
> > > esp_output_cb() at esp_output_cb+0x135
> > > taskq_thread() at taskq_thread+0x6c
> > > end trace frame: 0x0, count: 251
> > > End of stack trace.
> > > syncing disks... done
> >
> > This seems to be an invalid `'tdbi'' dereference in
> > ip6_output_ipsec_lookup():
> >
> > 2890: tdbi = (struct tdb_ident *)(mtag + 1);
> > 2891: HERE -> if (tdbi->spi == tdb->tdb_spi &&
> > 2892: tdbi->proto == tdb->tdb_sproto &&
> > ...
> >
> > Markus, Mike any idea how this could happen?
>
> I tracked it down to ref 1.89 of ip6_forward.c / ref 1.205 ip6_output.c:
> "factor out ipsec into ip6_output_ipsec_{lookup,send}(); ok mpi@, naddy@"
>
> The problem is that we are not exiting the "loop detection" for loop
> when tdb is set to NULL. We enter again and dereference tdb -> boom.
>
> The following diff makes ip6_output_ipsec_lookup() similar to
> ip_output_ipsec_lookup().
> It's easier to see what the diff is doing by applying and doing diff -b.
> OK?
ok mpi@
> p.s. I also note that the v4 and v6 version are really similiar, we
> can probably merge them. Wonder if it's worth it or if it's best to
> keep v4 and v6 seperate...
For the moment we're trying to reduce the size of "#ifdef IPSEC" chunks
inside the IP paths. But reducing differences between v4 and v6 by
reusing code is a good thing.
> diff --git ip6_output.c ip6_output.c
> index 64eea86..3adaa7d 100644
> --- ip6_output.c
> +++ ip6_output.c
> @@ -2882,21 +2882,21 @@ ip6_output_ipsec_lookup(struct mbuf *m, int *error,
> struct inpcb *inp)
> tdb = ipsp_spd_lookup(m, AF_INET6, sizeof(struct ip6_hdr),
> error, IPSP_DIRECTION_OUT, NULL, inp, 0);
>
> - if (tdb != NULL) {
> - /* Loop detection */
> - for (mtag = m_tag_first(m); mtag != NULL;
> - mtag = m_tag_next(m, mtag)) {
> - if (mtag->m_tag_id != PACKET_TAG_IPSEC_OUT_DONE)
> - continue;
> - tdbi = (struct tdb_ident *)(mtag + 1);
> - if (tdbi->spi == tdb->tdb_spi &&
> - tdbi->proto == tdb->tdb_sproto &&
> - tdbi->rdomain == tdb->tdb_rdomain &&
> - !bcmp(&tdbi->dst, &tdb->tdb_dst,
> - sizeof(union sockaddr_union)))
> - tdb = NULL;
> + if (tdb == NULL)
> + return NULL;
> + /* Loop detection */
> + for (mtag = m_tag_first(m); mtag != NULL; mtag = m_tag_next(m, mtag)) {
> + if (mtag->m_tag_id != PACKET_TAG_IPSEC_OUT_DONE)
> + continue;
> + tdbi = (struct tdb_ident *)(mtag + 1);
> + if (tdbi->spi == tdb->tdb_spi &&
> + tdbi->proto == tdb->tdb_sproto &&
> + tdbi->rdomain == tdb->tdb_rdomain &&
> + !memcmp(&tdbi->dst, &tdb->tdb_dst,
> + sizeof(union sockaddr_union))) {
> + /* no IPsec needed */
> + return NULL;
> }
> - /* We need to do IPsec */
> }
> return tdb;
> }
>
>
> --
> I'm not entirely sure you are real.
>
