[PATCH] hostap_plx: fix CIS verification

[PATCH] hostap_plx: fix CIS verification

[Pavel Roskin]

hostap_plx: fix CIS verification

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 ph35sm@free.fr

Signed-off-by: Pavel Roskin <proski@gnu.org>
---
This is an small fix with significant impact.
hostap_plx is currently broken, and this patch fixes it.
Please forward this patch to 2.6.x.y kernels.

 drivers/net/wireless/hostap/hostap_plx.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/hostap/hostap_plx.c b/drivers/net/wireless/hostap/hostap_plx.c
index 6dfa041..b5b72db 100644
--- a/drivers/net/wireless/hostap/hostap_plx.c
+++ b/drivers/net/wireless/hostap/hostap_plx.c
@@ -391,7 +391,7 @@ #define CIS_MAX_LEN 256
 			break;
 
 		case CISTPL_MANFID:
-			if (cis[pos + 1] < 5)
+			if (cis[pos + 1] < 4)
 				goto cis_error;
 			manfid1 = cis[pos + 2] + (cis[pos + 3] << 8);
 			manfid2 = cis[pos + 4] + (cis[pos + 5] << 8);

-- 
Regards,
Pavel Roskin

Re: [PATCH] hostap_plx: fix CIS verification

[Jouni Malinen]

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 ph35sm@free.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.

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..
Something like (cis[pos + 1] < 4 && pos + 5 < CIS_MAX_LEN) could be a
better fix here. I don't have easy access to PLX cards anymore, so this
is untested and I'm too lazy to copy this function into a separate
program to run it against CIS dumps.

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCH] hostap_plx: fix CIS verification

[John W. Linville]

On Fri, Oct 20, 2006 at 06:19:43PM -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 ph35sm@free.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.
> 
> 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..
> Something like (cis[pos + 1] < 4 && pos + 5 < CIS_MAX_LEN) could be a
> better fix here. I don't have easy access to PLX cards anymore, so this
> is untested and I'm too lazy to copy this function into a separate
> program to run it against CIS dumps.

Pavel,

Will you be refactoring this patch?  Or do you disagree with Jouni's
assessment?

Thanks,

John

Re: [PATCH] hostap_plx: fix CIS verification

[Pavel Roskin]

On Tue, 2006-10-24 at 20:37 -0400, John W. Linville wrote:
> Will you be refactoring this patch?  Or do you disagree with Jouni's
> assessment?

OK, give me an hour to produce a better patch.  My patch has an
advantage of being simple and of fixing exactly one thing, but if Jouni
feels more comfortable with an additional check, I'll add it.

I don't have Coverity to check, and I think the results of Coverity were
misinterpreted.  It doesn't know anything about CIS structure.  Anyway,
let me just check Jouni's suggestion on a real PLX card.

-- 
Regards,
Pavel Roskin

Re: [PATCH] hostap_plx: fix CIS verification

[Jouni Malinen]

On Tue, Oct 24, 2006 at 08:48:09PM -0400, Pavel Roskin wrote:

> I don't have Coverity to check, and I think the results of Coverity were
> misinterpreted.  It doesn't know anything about CIS structure.  Anyway,
> let me just check Jouni's suggestion on a real PLX card.

It doesn't need to know anything about the CIS structure. The driver
must not be allowed to run over a buffer even if given an invalid CIS
data.
 
-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCH] hostap_plx: fix CIS verification

[Pavel Roskin]

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 ph35sm@free.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

Re: [PATCH] hostap_plx: fix CIS verification

[Jouni Malinen]

On Tue, Oct 24, 2006 at 10:12:24PM -0400, Pavel Roskin wrote:

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

Indeed.

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

Agreed and I went through the report at some point and I think I found
it to be valid..

> So, the right fix would be:
> -		if (pos + cis[pos + 1] >= CIS_MAX_LEN)
> +		if (pos + 2 + cis[pos + 1] > CIS_MAX_LEN)
>  			goto cis_error;

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

Great, thanks!

-- 
Jouni Malinen                                            PGP id EFC895FA

[PATCH FIXED] hostap_plx: fix CIS verification

[Pavel Roskin]

hostap_plx: fix two related off-by-one errors in CIS parser

From: Pavel Roskin <proski@gnu.org>

The length of the manfid CIS should be at least 4, and it's normally 4.
It's incorrect to require it to be at least 5.  This breaks support for
most (if not all) cards.

The right place to ensure that we don't access beyond the CIS buffer is
to strengthen another check.  Make sure that the next tuple begins at
least at the CIS buffer end (in which case we stop processing) or
before that.

Reported by ph35sm@free.fr

Signed-off-by: Pavel Roskin <proski@gnu.org>
---
I'd like to remind that it's 2.6.x.y material.

 drivers/net/wireless/hostap/hostap_plx.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/hostap/hostap_plx.c b/drivers/net/wireless/hostap/hostap_plx.c
index 6dfa041..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]) {
@@ -391,7 +391,7 @@ #define CIS_MAX_LEN 256
 			break;
 
 		case CISTPL_MANFID:
-			if (cis[pos + 1] < 5)
+			if (cis[pos + 1] < 4)
 				goto cis_error;
 			manfid1 = cis[pos + 2] + (cis[pos + 3] << 8);
 			manfid2 = cis[pos + 4] + (cis[pos + 5] << 8);

-- 
Regards,
Pavel Roskin