kern/89748: BPF indexed byte load can sign extend

[Guy Harris]

>Number:         89748
>Category:       kern
>Synopsis:       BPF indexed byte load can sign extend
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Wed Nov 30 08:50:02 GMT 2005
>Closed-Date:
>Last-Modified:
>Originator:     Guy Harris
>Release:        
>Organization:
>Environment:
>Description:
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;
                int len;

                if (buflen != 0)
                    return 0;
                m = (struct mbuf *)p;
                MINDEX(len, m, k);
                A = mtod(m, u_char *)[k];
                continue;
#else
                return 0;
#endif
            }
            A = p[k];
            continue;

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 (k >= buflen) {
#ifdef _KERNEL
                struct mbuf *m;
                int len;

                if (buflen != 0)
                    return 0;
                m = (struct mbuf *)p;
                MINDEX(len, m, k);
                A = mtod(m, char *)[k];
                continue;
#else
                return 0;
#endif
            }
            A = p[k];
            continue;

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 NetBSD, it's "u_char" in both "mtod()" calls, fixing that.

>How-To-Repeat:
              
>Fix:
Index: bpf_filter.c
===================================================================
RCS file: /cvs/src/sys/net/bpf_filter.c,v
retrieving revision 1.15
diff -c -r1.15 bpf_filter.c
*** bpf_filter.c	28 Sep 2005 20:53:56 -0000	1.15
--- bpf_filter.c	30 Nov 2005 07:58:29 -0000
***************
*** 301,307 ****
  					return 0;
  				m = (struct mbuf *)p;
  				MINDEX(len, m, k);
! 				A = mtod(m, char *)[k];
  				continue;
  #else
  				return 0;
--- 301,307 ----
  					return 0;
  				m = (struct mbuf *)p;
  				MINDEX(len, m, k);
! 				A = mtod(m, u_char *)[k];
  				continue;
  #else
  				return 0;

>Release-Note:
>Audit-Trail:
>Unformatted: