Re: BPF indexed byte load can sign extend

["Sepherosa Ziehau" <sepherosa@xxxxxxxxx>]

On Jan 2, 2008 10:56 AM, Guy Harris <guy@alum.mit.edu> wrote:
> The code to handle BPF non-indexed loads in bpf_filter() in bpf_filter.c is:
>
>                 case BPF_LD|BPF_B|BPF_ABS:
>                         k = pc->k;
>                         if (k > buflen) {
> #ifdef _KERNEL
>                                 struct mbuf *m;
>
>                                 if (buflen != 0)
>                                         return 0;
>                                 m = (struct mbuf *)p;
>                                 MINDEX(m, k);
>                                 A = mtod(m, u_char *)[k];
>                                 continue;
> #else
>                                 return 0;
>                         }
>
>
> which will load the byte at the offset in the instruction into the
> accumulator, zero-extending it.
>
> The code to handle indexed loads is
>
>                 case BPF_LD|BPF_B|BPF_IND:
>                         k = X + pc->k;
>                         if (pc->k >= buflen || X >= buflen - pc->k) {
> #ifdef _KERNEL
>                                 struct mbuf *m;
>
>                                 if (buflen != 0)
>                                         return 0;
>                                 m = (struct mbuf *)p;
>                                 MINDEX(m, k);
>                                 A = mtod(m, char *)[k];
>                                 continue;
> #else
>                                 return 0;
>                         }
>
>
> which will load the byte at the offset that's the sum of the offset in
> the instruction and the contents of the index register; if buflen is 0,
> as is the case when this is called from bpf_mtap(), it will sign-extend
> it on most if not all platforms, as the mbuf data pointer is cast to
> "char *" rather than "u_char *". Otherwise, as is the case when this is
> called from bpf_tap(), it'll zero-extend it, as "p" is a "u_char *".
>
> Presumably those semantics are not intended, as the same instruction
> behaves differently depending on how the tapping is being done, and on
> whether the load is indexed or not. (In libpcap's user-mode filter, it's
> never sign-extended.)
>
> In current NetBSD, OpenBSD, and FreeBSD, it's "u_char" in both "mtod()"
> calls, fixing that.
>
> Here's a patch:

Committed.  Thanks!
BTW, please create a unified patch next time :)

Best Regards,
sephe

>
> *** bpf_filter.c        Tue Jan  1 18:46:19 2008
> --- bpf_filter.c.UNSIGNED       Tue Jan  1 18:44:03 2008
> ***************
> *** 326,332 ****
>                                         return 0;
>                                 m = (struct mbuf *)p;
>                                 MINDEX(m, k);
> !                               A = mtod(m, char *)[k];
>                                 continue;
>    #else
>                                 return 0;
> --- 326,332 ----
>                                         return 0;
>                                 m = (struct mbuf *)p;
>                                 MINDEX(m, k);
> !                               A = mtod(m, u_char *)[k];
>                                 continue;
>    #else
>                                 return 0;
>



-- 
Live Free or Die