Re: [PATCH] hostap_plx: fix CIS verification
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 [EMAIL PROTECTED] 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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] hostap_plx: fix CIS verification
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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] hostap_plx: fix CIS verification
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 MalinenPGP id EFC895FA - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] 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 [EMAIL PROTECTED]
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
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] hostap_plx: fix CIS verification
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 MalinenPGP id EFC895FA - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] hostap_plx: fix CIS verification
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 [EMAIL PROTECTED] 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 MalinenPGP id EFC895FA - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
