Subject: Re: mtod abuse?
To: tech-net@netbsd.org, David Young <dyoung@pobox.com>
From: Pavel Cahyna <pcah8322@artax.karlin.mff.cuni.cz>
List: tech-net
Date: 08/20/2004 08:36:06
--cWoXeonUoKmBZSoM
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

> On Sat, Aug 07, 2004 at 06:04:28PM +0200, Pavel Cahyna wrote:
> > Hello,
> > 
> > is it correct to call mtod() without calling m_pullup() before dereferencing
> > the pointer obtained? I would think that it isn't. Such calls do occur in
> > wi.c, function wi_start(), however. e.g.
> > 
> > --- cut here ---
> > 			IF_DEQUEUE(&ic->ic_pwrsaveq, m0);
> >                         wh = mtod(m0, struct ieee80211_frame *);
> > 			llc = (struct llc *) (wh + 1);
> > 			m_copydata(m0, 4, ETHER_ADDR_LEN * 2,
> > 			    (caddr_t)&frmhdr.wi_ehdr);
> > --->			frmhdr.wi_ehdr.ether_type = llc->llc_snap.ether_type;
> > --- cut here ---
> 
> This looks like a bug to me.  I am looking at ieee80211_encap, which
> inserts the LLC header, and I do not think the 802.11 header and LLC
> header will ordinarily be stored consecutively.
> 
> An m_pullup is appropriate.  However, I do not see how the LLC header
> can straddle two mbufs.  So I recommend that you use m_pulldown instead
> of m_pullup.

Is the attached patch a correct fix for this?

Bye	Pavel

--cWoXeonUoKmBZSoM
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="wi-llc.diff"

Index: wi.c
===================================================================
RCS file: /home/pavel/cvs/src/sys/dev/ic/wi.c,v
retrieving revision 1.186
diff -u -r1.186 wi.c
--- wi.c	10 Aug 2004 00:57:20 -0000	1.186
+++ wi.c	20 Aug 2004 06:34:56 -0000
@@ -1039,7 +1039,8 @@
 			break;
 		else if (!IF_IS_EMPTY(&ic->ic_pwrsaveq)) {
 			struct llc *llc;
-
+			struct mbuf *n;
+			int llc_off;
 			/* 
 			 * Should these packets be processed after the
 			 * regular packets or before?  Since they are being
@@ -1051,7 +1052,14 @@
 
 			IF_DEQUEUE(&ic->ic_pwrsaveq, m0);
                         wh = mtod(m0, struct ieee80211_frame *);
-			llc = (struct llc *) (wh + 1);
+			n = m_pulldown(m0, sizeof (struct ieee80211_frame),
+				       sizeof (struct llc), &llc_off);
+			if (n == NULL) {
+				ifp->if_oerrors++;
+				continue;
+			}
+			
+			llc = (struct llc *) mtod(n, caddr_t) + llc_off;
 			m_copydata(m0, 4, ETHER_ADDR_LEN * 2,
 			    (caddr_t)&frmhdr.wi_ehdr);
 			frmhdr.wi_ehdr.ether_type = llc->llc_snap.ether_type;

--cWoXeonUoKmBZSoM--