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