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