NetBSD Problem Report #32928

From pcah8322@artax.karlin.mff.cuni.cz  Sat Feb 25 12:18:09 2006
Return-Path: <pcah8322@artax.karlin.mff.cuni.cz>
Received: from mail.casablanca.cz (mail.casablanca.cz [82.208.31.182])
	by narn.netbsd.org (Postfix) with ESMTP id 32CB063B871
	for <gnats-bugs@gnats.NetBSD.org>; Sat, 25 Feb 2006 12:18:09 +0000 (UTC)
Message-Id: <200602251217.k1PCHtb8000952@beta.martani.n2.repy.czf>
Date: Sat, 25 Feb 2006 13:17:55 +0100 (CET)
From: Pavel Cahyna <pcah8322@artax.karlin.mff.cuni.cz>
Reply-To: pavel.cahyna@st.mff.cuni.cz
To: gnats-bugs@netbsd.org
Subject: bpf filter can fail to extract a 32-bit quantity
X-Send-Pr-Version: 3.95

>Number:         32928
>Category:       kern
>Synopsis:       bpf filter can fail to extract a 32-bit quantity
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sat Feb 25 12:20:00 +0000 2006
>Last-Modified:  Sat Feb 25 14:35:01 +0000 2006
>Originator:     Pavel Cahyna
>Release:        4.3BSD NET/2 - NetBSD 3.99.15
>Organization:
>Environment:
System: NetBSD beta 3.0_RC5 NetBSD 3.0_RC5 (EV56) #3: Mon Dec 12 20:28:20 CET 2005 pavel@beta:/usr/src/sys/arch/alpha/compile/EV56 alpha
Architecture: alpha
Machine: alpha
>Description:
In net/bpf_filter.c there is a function m_xword() which extracts a
32-bit integer from a mbuf chain at a given offset k.

First, it determines where the integer starts:
	MINDEX(len, m, k);
m is now the first mbuf, len is its length and k is updated to the
offset of the data in the mbuf. Then we test if the whole integer is
in the mbuf:
	if (len >= k + 4) {
		*err = 0;
		return EXTRACT_LONG(cp);
	}
if it is not, we go to the next mbuf:
	m0 = m->m_next;
	if (m0 == 0 || m0->m_len + len - k < 4)
		goto bad;

but the "goto bad" if "m0->m_len + len - k < 4" is wrong - the integer
could legally continue in a third mbuf. Of course removing this check
is not enough - some code must be added to handle this situation.
>How-To-Repeat:
Code inspection.

This will be difficult to trigger (a 1- or 2- byte mbuf in the middle
of a mbuf chain is unlikely), but with functions like bpf_mtap2()
which prepend a short mbuf it could be possible.

>Fix:
I'm working on it. I propose to call m_xhalf() twice and concatenate
the results.

>Audit-Trail:
From: Pavel Cahyna <pavel.cahyna@st.mff.cuni.cz>
To: Rui Paulo <rpaulo@fnop.net>
Cc: gnats-bugs@netbsd.org
Subject: Re: kern/32928: bpf filter can fail to extract a 32-bit quantity
Date: Sat, 25 Feb 2006 14:42:50 +0100

 On Sat, Feb 25, 2006 at 12:51:33PM +0000, Rui Paulo wrote:
 > --- bpf_filter.c.~1.29.~	2006-02-10 20:08:13.000000000 +0000
 > +++ bpf_filter.c	2006-02-25 12:51:07.000000000 +0000
 > @@ -98,9 +98,13 @@ m_xword(struct mbuf *m, uint32_t k, int 
 >  		*err = 0;
 >  		return EXTRACT_LONG(cp);
 >  	}
 > -	m0 = m->m_next;
 > -	if (m0 == 0 || m0->m_len + len - k < 4)
 > -		goto bad;
 > +
 > +	for (m0 = m->m_next; ; m0 = m0->next) {
 > +		if (m0 == 0)
 > +			goto bad;
 > +		if (m0->m_len + len - k >= 4)
 > +			break;
 > +	}

 Sorry, I don't see how this is supposed to work. This would skip the short
 mbuf(s) and read different data than it is supposed to.

 BTW I'm planning to reorganize this code a bit... I just wanted to know if
 this is an actual bug and if calling m_xhalf twice would be OK.

From: Pavel Cahyna <pavel.cahyna@st.mff.cuni.cz>
To: Rui Paulo <rpaulo@fnop.net>
Cc: gnats-bugs@netbsd.org
Subject: Re: kern/32928: bpf filter can fail to extract a 32-bit quantity
Date: Sat, 25 Feb 2006 15:05:25 +0100

 On Sat, Feb 25, 2006 at 01:52:38PM +0000, Rui Paulo wrote:
 > >  BTW I'm planning to reorganize this code a bit... I just wanted to know if
 > >  this is an actual bug and if calling m_xhalf twice would be OK.
 > 
 > Alright. IIUC, m_xhalf will never be called twice with the same mbuf,
 > but I can be completely wrong and you'll probably correct me :-)

 I'm proposing this, so m_xhalf would actually be called twice with the
 same mbuf (of course, error handling is unfinished):

 Index: bpf_filter.c
 ===================================================================
 RCS file: /home/pavel/cvs/src/sys/net/bpf_filter.c,v
 retrieving revision 1.29
 diff -u -c -r1.29 bpf_filter.c
 cvs diff: conflicting specifications of output style
 --- bpf_filter.c	7 Feb 2006 20:10:48 -0000	1.29
 +++ bpf_filter.c	25 Feb 2006 14:01:27 -0000
 @@ -97,27 +97,10 @@
  	if (len >= k + 4) {
  		*err = 0;
  		return EXTRACT_LONG(cp);
 +	} else {
 +		return (m_xhalf(m, k, err) << 16) | m_xhalf(m, k+2, err);
  	}
 -	m0 = m->m_next;
 -	if (m0 == 0 || m0->m_len + len - k < 4)
 -		goto bad;
 -	*err = 0;
 -	np = mtod(m0, u_char *);
 -	switch (len - k) {
 -
 -	case 1:
 -		return (cp[0] << 24) | (np[0] << 16) | (np[1] << 8) |
 		np[2];
 -
 -	case 2:
 -		return (cp[0] << 24) | (cp[1] << 16) | (np[0] << 8) |
 		np[1];

 -	default:
 -		return (cp[0] << 24) | (cp[1] << 16) | (cp[2] << 8) |
 		np[0];
 -	}
 -    bad:
 -	*err = 1;
 -
 -	return 0;
  }

  static int

From: David Laight <david@l8s.co.uk>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: kern/32928: bpf filter can fail to extract a 32-bit quantity
Date: Sat, 25 Feb 2006 14:29:10 +0000

 On Sat, Feb 25, 2006 at 02:10:03PM +0000, Pavel Cahyna wrote:
 >  +	} else {
 >  +		return (m_xhalf(m, k, err) << 16) | m_xhalf(m, k+2, err);

 I suspect you really don't want to be doing that unless the data
 actually crosses 2 mbuf boundaries.
 In which case the correct thing to do is fix the underlying routine.

 	David

 -- 
 David Laight: david@l8s.co.uk

From: Pavel Cahyna <pcah8322@artax.karlin.mff.cuni.cz>
To: gnats-bugs@netbsd.org
Cc: kern-bug-people@netbsd.org, gnats-admin@netbsd.org,
	netbsd-bugs@netbsd.org
Subject: Re: kern/32928: bpf filter can fail to extract a 32-bit quantity
Date: Sat, 25 Feb 2006 15:32:07 +0100

 >  >  +	} else {
 >  >  +		return (m_xhalf(m, k, err) << 16) | m_xhalf(m, k+2, err);
 >  
 >  I suspect you really don't want to be doing that unless the data
 >  actually crosses 2 mbuf boundaries.

 Why not? Because of performance, or is such code incorrect?

 Pavel Cahyna

Home	PR Database Search