Bug #903

BPF indexed byte load can sign extend

Added by guy almost 7 years ago. Updated over 6 years ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:-
Target version:-

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;

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:

*** 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;

History

#1 Updated by sepherosa almost 7 years ago

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

Best Regards,
sephe

Also available in: Atom PDF