Subject: Re: crashes in ipfilter on i386
To: Greg Troxel <gdt@ir.bbn.com>
From: Darren Reed <darrenr@netbsd.org>
List: tech-net
Date: 09/10/2007 23:24:09
Greg Troxel wrote:
> ...
> Are you sure that you use this code?
>
> > if (frpr_pullup(fin, ICMP6ERR_MINPKTLEN) == -1)
> > return;
> [...]
> > icmp6 = fin->fin_dp;
> > ip6 = (ip6_t *)((char *)icmp6 + ICMPERR_ICMPHLEN);
> > if (IP6_NEQ(&fin->fin_fi.fi_dst,
> > &ip6->ip6_src))
> > fin->fin_flx |= FI_BAD;
>
> I am asking, because there was a bug exactly in this place
> (a stale version of the icmp6 pointer was used) and the crash
> was exactly where you have shown it in your previous mail.
>
> However, this is fixed in the code snippet above.
>
> The frpr_pullup ensures that enough data is in the buffer for the
> IP6_NEQ operation and the icmp6 (and thus ip6) pointers are recomputed
> in case the buffer was moved.
>
> I am sure that I was running the code with the frpr_pullup.
>
>
> Here's the diff in my source tree, producing a kernel that doesn't
> crash. Note there is a lot of s/INLINE// noise because I found it hard
> to follow the asm code, but the real change is dropping the IP6_NEQ
> test. Obviously my change causes a functionality regression - it was
> intended to isolate the bug, not proposed as a fix.
>
> I still suspect failure to pull up enough bytes, but I can't point to
> the wrong line of code.
>
> Any clues?
>
I'd like you to try it without the #if 0 but with this change in ip_fil.h:
! #define I60(x) (((i6addr_t *)(x))->i6[0])
! #define I61(x) (((i6addr_t *)(x))->i6[1])
! #define I62(x) (((i6addr_t *)(x))->i6[2])
! #define I63(x) (((i6addr_t *)(x))->i6[3])
! #define HI60(x) ntohl(((i6addr_t *)(x))->i6[0])
! #define HI61(x) ntohl(((i6addr_t *)(x))->i6[1])
! #define HI62(x) ntohl(((i6addr_t *)(x))->i6[2])
! #define HI63(x) ntohl(((i6addr_t *)(x))->i6[3])
#define IP6_EQ(a,b) ((I63(a) == I63(b)) && (I62(a) ==
I62(b)) && \
(I61(a) == I61(b)) && (I60(a) == I60(b)))
--- 156,169 ----
#define iplookupptr vptr[0]
#define iplookupfunc lptr[1]
! #define I60(x) (((u_32_t *)(x))[0])
! #define I61(x) (((u_32_t *)(x))[1])
! #define I62(x) (((u_32_t *)(x))[2])
! #define I63(x) (((u_32_t *)(x))[3])
! #define HI60(x) ntohl(((u_32_t *)(x))[0])
! #define HI61(x) ntohl(((u_32_t *)(x))[1])
! #define HI62(x) ntohl(((u_32_t *)(x))[2])
! #define HI63(x) ntohl(((u_32_t *)(x))[3])
Darren