Bug 12484

>	would not capture such packets. Debugging turned up a problem
>	in this case:
>
>	    case BPF_LD|BPF_B|BPF_IND:
>		    k = X + pc->k;
>		    if (pc->k >= buflen || X >= buflen - k) {
>	
>	The conditional can be rewritten with "k" expanded:
>
>		    if (pc->k >= buflen || X >= buflen - (X + pc->k)) {
>
>	which is the same as:
>
>		    if (pc->k >= buflen || X + X + pc->k >= buflen) {
>	
>	and is clearly wrong.

Oops.  This should have been:

		if (pc->k >= buflen || X >= buflen - p->k) {

which is the same modulo possible truncation mod 2^32 as each of the
following:

		if (pc->k >= buflen || X + p->k >= buflen) {
		if (pc->k >= buflen || k >= buflen) {
		if (k >= buflen) {

The latter is the conditinal in rev.1.11.

>	Other cases where "k" is initialized to "X + pc->k" appear to
>	either be broken or at least contain unnecessary checks.

The other cases seem to be correct.  The extra checks relative to 1.11
are to detect truncation for weird values of X and/or pc->k, e.g., X =
4, pc->k = (u_int32_t)-5 gives X + pc->k = 3 if u_int32_t is u_int.
I don't know if this much overflow checking is justified.

>	The appended context changes are against 1.13 (it looks like
>	3.2-RELEASE shipped with 1.12).
>
>RCS file: RCS/bpf_filter.c,v
>retrieving revision 1.3
>diff -c -r1.3 bpf_filter.c
>*** /tmp/,RCSt1z29845	Thu Jul  1 19:43:31 1999
>- --- bpf_filter.c	Thu Jul  1 19:42:40 1999
>***************
>*** 217,223 ****
>  
>  		case BPF_LD|BPF_W|BPF_ABS:
>  			k = pc->k;
>! 			if (k > buflen || sizeof(int32_t) > buflen - k) {
>  #ifdef KERNEL
>  				int merr;
>  
>- --- 217,223 ----
>  
>  		case BPF_LD|BPF_W|BPF_ABS:
>  			k = pc->k;
>! 			if (sizeof(int32_t) > buflen - k) {
>  #ifdef KERNEL
>  				int merr;
>  

If you reduce the amount of overflow checking back to 1.11 levels, then
change the checks back to the ones in 1.11. e.g.,

 			if (k + sizeof(int32_t) > buflen) {

The check:

			if (sizeof(int32_t) > buflen - k) {

has fatal overflow problems if k > buflen and u_int32_t is u_int.

Bruce

Comment 2 Mike Barcroft 2001-07-21 00:11:34 UTC

State Changed
From-To: open->feedback


Does this problem still occur in newer versions of FreeBSD, 
such as 4.3-RELEASE?

Comment 3 Mike Barcroft 2001-07-21 18:18:45 UTC

Adding this to the Audit-Trail.

On Sat, Jul 21, 2001 at 10:40:39PM +1000, Bruce Evans wrote:
> I kept the following private mail about this:
> 
> > From bde@zeta.org.au Wed Aug 16 03:34:22 2000 +1000
> > Date: Wed, 16 Aug 2000 03:34:20 +1000 (EST)
> > From: Bruce Evans <bde@zeta.org.au>
> > X-Sender: bde@besplex.bde.org
> > To: Johan Karlsson <k@numeri.campus.luth.se>
> > Subject: Re: PR 12484 and your commit rev 1.14 of src/sys/net/bpf_filter.c
> > In-Reply-To: <200008150757.JAA13997@numeri.campus.luth.se>
> > Message-ID: <Pine.BSF.4.21.0008160316500.2104-100000@besplex.bde.org>
> > MIME-Version: 1.0
> > Content-Type: TEXT/PLAIN; charset=US-ASCII
> > Status: O
> > X-Status: 
> > X-Keywords:                  
> > X-UID: 859
> > 
> > On Tue, 15 Aug 2000, Johan Karlsson wrote:
> > 
> > > Hi Bruce
> > > It seems you commited a fix for PR 12484 in rev 1.14 of 
> > > src/sys/net/bpf_filter.c.
> > > 
> > > Can the PR be closed or should the 
> > > "inconsistent and probably unnecessarily pessimal" checks
> > > be fixed first?
> > 
> > I don't know bpf well enough to be sure of more than the formal
> > correctness of the fix.  The people on the Cc line of the PR
> > should know :-).  Craig Leres didn't seem to have any objections.
> > 
> > The efficiency issues were more interesting on old (slow) hardware.
> > 
> > PR 16644 is also about this.  Its author is also at LBL, but seems
> > to be as confused as usual.
> > 
> > Bruce
> 
> Bruce
>

Comment 4 Mike Barcroft 2001-07-21 18:29:25 UTC

State Changed
From-To: feedback->closed


I'm satisfied the originator's problem has been solved.