[PATCH RFC] cfg80211: Add ies_allocated check before copying ies

[PATCH RFC] cfg80211: Add ies_allocated check before copying ies

[Michael Buesch]

I don't fully understand the code, but let's imagine the following situation:

- cfg80211_bss_update was called and the ie was stored to allocated space.
- The information_elements pointer is changed to the allocated space.
- cfg80211_bss_update is called again, but now the ie fits into the
  space after the "found" structure.
- But the information_elements pointer still points to the allocated space.
  So it may overrun the buffer and crash.

Is this scenario possible?
If yes, please consider the following patch.

Signed-off-by: Michael Buesch <mb@bu3sch.de>

---
 net/wireless/scan.c |    5 +++++
 1 file changed, 5 insertions(+)

--- wireless-testing.orig/net/wireless/scan.c
+++ wireless-testing/net/wireless/scan.c
@@ -378,6 +378,11 @@ cfg80211_bss_update(struct cfg80211_regi
 			size_t ielen = res->pub.len_information_elements;
 
 			if (ksize(found) >= used + ielen) {
+				if (found->ies_allocated) {
+					kfree(found->pub.information_elements);
+					found->ies_allocated = false;
+					found->pub.information_elements = (u8 *)found + used;
+				}
 				memcpy(found->pub.information_elements,
 				       res->pub.information_elements, ielen);
 				found->pub.len_information_elements = ielen;

-- 
Greetings, Michael.

Re: [PATCH RFC] cfg80211: Add ies_allocated check before copying ies

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1191 bytes --]

On Sat, 2009-04-25 at 22:36 +0200, Michael Buesch wrote:
> I don't fully understand the code, but let's imagine the following situation:
> 
> - cfg80211_bss_update was called and the ie was stored to allocated space.
> - The information_elements pointer is changed to the allocated space.
> - cfg80211_bss_update is called again, but now the ie fits into the
>   space after the "found" structure.
> - But the information_elements pointer still points to the allocated space.
>   So it may overrun the buffer and crash.
> 
> Is this scenario possible?
> If yes, please consider the following patch.

Yeah, looks like a bug, good catch. I don't think the fix is correct
though -- you lose the data in this case.

I think it should simply be:

>  			size_t ielen = res->pub.len_information_elements;
>  
> -			if (ksize(found) >= used + ielen) {
> +			if (!found->ies_allocated && ksize(found) >= used + ielen) {
>  				memcpy(found->pub.information_elements,
>  				       res->pub.information_elements, ielen);
>  				found->pub.len_information_elements = ielen;
> 

so that the else branch gets a chance to reallocate if necessary, would
you agree?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH RFC] cfg80211: Add ies_allocated check before copying ies

[Michael Buesch]

On Sunday 26 April 2009 10:32:32 Johannes Berg wrote:
> On Sat, 2009-04-25 at 22:36 +0200, Michael Buesch wrote:
> > I don't fully understand the code, but let's imagine the following situation:
> > 
> > - cfg80211_bss_update was called and the ie was stored to allocated space.
> > - The information_elements pointer is changed to the allocated space.
> > - cfg80211_bss_update is called again, but now the ie fits into the
> >   space after the "found" structure.
> > - But the information_elements pointer still points to the allocated space.
> >   So it may overrun the buffer and crash.
> > 
> > Is this scenario possible?
> > If yes, please consider the following patch.
> 
> Yeah, looks like a bug, good catch. I don't think the fix is correct
> though -- you lose the data in this case.
> 
> I think it should simply be:
> 
> >  			size_t ielen = res->pub.len_information_elements;
> >  
> > -			if (ksize(found) >= used + ielen) {
> > +			if (!found->ies_allocated && ksize(found) >= used + ielen) {
> >  				memcpy(found->pub.information_elements,
> >  				       res->pub.information_elements, ielen);
> >  				found->pub.len_information_elements = ielen;
> > 
> 
> so that the else branch gets a chance to reallocate if necessary, would
> you agree?

Yeah I first also considered this option, but I thought that the code likes to
prefer putting the stuff into the "found" tail, if it fits.
But yes, your fix is perfectly fine. If it was allocated once, it will always be
allocated (and grown) from then on.
It won't shrink the buffer anymore for future ies that are smaller, but I guess that's OK.

-- 
Greetings, Michael.

Re: [PATCH RFC] cfg80211: Add ies_allocated check before copying ies

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1234 bytes --]

On Sun, 2009-04-26 at 10:40 +0200, Michael Buesch wrote:

> > I think it should simply be:
> > 
> > >  			size_t ielen = res->pub.len_information_elements;
> > >  
> > > -			if (ksize(found) >= used + ielen) {
> > > +			if (!found->ies_allocated && ksize(found) >= used + ielen) {
> > >  				memcpy(found->pub.information_elements,
> > >  				       res->pub.information_elements, ielen);
> > >  				found->pub.len_information_elements = ielen;
> > > 
> > 
> > so that the else branch gets a chance to reallocate if necessary, would
> > you agree?
> 
> Yeah I first also considered this option, but I thought that the code likes to
> prefer putting the stuff into the "found" tail, if it fits.
> But yes, your fix is perfectly fine. If it was allocated once, it will always be
> allocated (and grown) from then on.
> It won't shrink the buffer anymore for future ies that are smaller, but I guess that's OK.

Ah, yes, but then the correct fix would be to kfree the allocated IEs
and use the old buffer again, not discard the data :) I don't think it's
worth bothering, the entire structures are usually freed fairly quickly
anyway. Do you want to redo the patch or would you prefer if I sent it?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]