hostap_plx: fix CIS verification

Hello, Jouni!

On Fri, 2006-10-20 at 18:19 -0700, Jouni Malinen wrote: 
> On Fri, Oct 20, 2006 at 06:20:15PM -0400, Pavel Roskin
wrote:
> 
> > The record length for numerical manufacturer ID
should be at least 4
> > bytes (two 16-bit words).  The code required 5
bytes, which would break
> > for most (if not all) cards.  Reported by
ph35smfree.fr
> 
> >  		case CISTPL_MANFID:
> > -			if (cis[pos + 1] < 5)
> > +			if (cis[pos + 1] < 4)
> 
> Hmm.. Interesting. I think this was changed from 4 to 5
due to a
> potential buffer overflow as reported by Coverity
tools.. In addition, I
> think that I spent long time trying to understand why
it could be a
> buffer overflow and since it was changed, likely
finally figured out an
> example case.. Alas, I don't remember what exactly this
was anymore.

Coverity has no means to interpret CIS.  However, it may
understand
kmalloc, which allocates CIS_MAX_LEN for the CIS copy.

The value of cis[pos + 1] has no bearing on the validity of
the access
to cis[pos + 5] from the point of view of a checking tool.

> It looks like the comparison of the length field to be
<5 was incorrect,
> but in order to avoid re-introducing any potential
buffer overflows,
> that condition could be extended to verify that pos is
small enough..

pos is already checked in the beginning of the loop to be
small enough,
but the check is not strong enough.  The next tuple starts
at (pos +
cis[pos + 1] + 2), and we want that to be at most
CIS_MAX_LEN.

That's something Coverity could have found.

So, the right fix would be:

diff --git a/drivers/net/wireless/hostap/hostap_plx.c
b/drivers/net/wireless/hostap/hostap_plx.c
index b5b72db..bc81b13 100644
--- a/drivers/net/wireless/hostap/hostap_plx.c
+++ b/drivers/net/wireless/hostap/hostap_plx.c
 -364,7
+364,7  #define CIS_MAX_LEN 256
 
 	pos = 0;
 	while (pos < CIS_MAX_LEN - 1 && cis[pos] !=
CISTPL_END) {
-		if (pos + cis[pos + 1] >= CIS_MAX_LEN)
+		if (pos + 2 + cis[pos + 1] > CIS_MAX_LEN)
 			goto cis_error;
 
 		switch (cis[pos]) {

I'm rewriting this with "<" because it's easier
to read.  Next tuple at
exactly CIS_MEX_LEN is fine - we just stop precessing at
that point.

I'm going to combine this with my previous fix and resend.

-- 
Regards,
Pavel Roskin

_______________________________________________
HostAP mailing list
HostAPshmoo.com
http:/
/lists.shmoo.com/mailman/listinfo/hostap