[Patch 1/1] wext: fix 32/64 bit alignment issue for 'point' type

[Patch 1/1] wext: fix 32/64 bit alignment issue for 'point' type

[Gerrit Renker]

wireless: fix alignment problem

With the current definition, IW_EV_POINT_PK_LEN is 12 on 64-bit and 8 on 32-bit systems,
due to the way struct iw_event is packed (24 byte on 64-bit systems instead of 20 byte
on 32-bit systems). Furthermore, the iwe_stream_add_point() in include/net/iw_handler.h
also uses IW_EV_LCP_PK_LEN as header length. 

The current definition appears to be a typo (PK_LEN instead of LEN); it causes 
misalignment on 64 bit systems.

Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
 include/linux/wireless.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
	
--- a/include/linux/wireless.h
+++ b/include/linux/wireless.h
@@ -1157,6 +1157,6 @@ struct __compat_iw_event {
 #define IW_EV_PARAM_PK_LEN	(IW_EV_LCP_PK_LEN + sizeof(struct iw_param))
 #define IW_EV_ADDR_PK_LEN	(IW_EV_LCP_PK_LEN + sizeof(struct sockaddr))
 #define IW_EV_QUAL_PK_LEN	(IW_EV_LCP_PK_LEN + sizeof(struct iw_quality))
-#define IW_EV_POINT_PK_LEN	(IW_EV_LCP_LEN + 4)
+#define IW_EV_POINT_PK_LEN	(IW_EV_LCP_PK_LEN + 4)
 
 #endif	/* _LINUX_WIRELESS_H */

Re: [Patch 1/1] wext: fix 32/64 bit alignment issue for 'point' type

[Johannes Berg]

On Thu, 2010-09-30 at 12:25 +0200, Gerrit Renker wrote:
> wireless: fix alignment problem
> 
> With the current definition, IW_EV_POINT_PK_LEN is 12 on 64-bit and 8 on 32-bit systems,
> due to the way struct iw_event is packed (24 byte on 64-bit systems instead of 20 byte
> on 32-bit systems). Furthermore, the iwe_stream_add_point() in include/net/iw_handler.h
> also uses IW_EV_LCP_PK_LEN as header length. 
> 
> The current definition appears to be a typo (PK_LEN instead of LEN); it causes 
> misalignment on 64 bit systems.

So, correct me if I'm wrong, the only effect this change has is changing
the second memcpy() in iwe_stream_add_point() to not copy an extra 4
bytes that will be overwritten right away by the next memcpy()
(presumably, unless the data is < 4, in which case the memcpy might
actually be out of bounds).

Where's the misalignment issue?

johannes

Re: [Patch 1/1] wext: fix 32/64 bit alignment issue for 'point' type

[Johannes Berg]

On Thu, 2010-09-30 at 16:38 +0200, Johannes Berg wrote:
> On Thu, 2010-09-30 at 12:25 +0200, Gerrit Renker wrote:
> > wireless: fix alignment problem
> > 
> > With the current definition, IW_EV_POINT_PK_LEN is 12 on 64-bit and 8 on 32-bit systems,
> > due to the way struct iw_event is packed (24 byte on 64-bit systems instead of 20 byte
> > on 32-bit systems). Furthermore, the iwe_stream_add_point() in include/net/iw_handler.h
> > also uses IW_EV_LCP_PK_LEN as header length. 
> > 
> > The current definition appears to be a typo (PK_LEN instead of LEN); it causes 
> > misalignment on 64 bit systems.
> 
> So, correct me if I'm wrong, the only effect this change has is changing
> the second memcpy() in iwe_stream_add_point() to not copy an extra 4
> bytes that will be overwritten right away by the next memcpy()
> (presumably, unless the data is < 4, in which case the memcpy might
> actually be out of bounds).

Interestingly -- yay for wext! -- wireless-tools source code actually
uses the define you propose:

#define IW_EV_POINT_PK_LEN	(IW_EV_LCP_PK_LEN + 4)

which probably means that it was fixed there but the fix never
propagated to the kernel ...

johannes

Re: [Patch 1/1] wext: fix 32/64 bit alignment issue for 'point' type

[gerrit]

> On Thu, 2010-09-30 at 12:25 +0200, Gerrit Renker wrote:
>> wireless: fix alignment problem
>>
>> With the current definition, IW_EV_POINT_PK_LEN is 12 on 64-bit and 8 on
>> 32-bit systems, due to the way struct iw_event is packed (24 byte on
>> 64-bit systems instead of 20 byte on 32-bit systems). Furthermore, the
iwe_stream_add_point() in
>> include/net/iw_handler.h
>> also uses IW_EV_LCP_PK_LEN as header length.
>>
>> The current definition appears to be a typo (PK_LEN instead of LEN); it
>> causes misalignment on 64 bit systems.
>
> So, correct me if I'm wrong, the only effect this change has is changing
> the second memcpy() in iwe_stream_add_point() to not copy an extra 4
> bytes that will be overwritten right away by the next memcpy()
> (presumably, unless the data is < 4, in which case the memcpy might
> actually be out of bounds).
>
Thank you for pointing this out, it seems the change is not as trivial
as I thought.

I noticed this problem in userspace where the 4 byte difference caused
misalignment of point types (such as essid) on a 64 bit system.

I wonder whether the following thinking is right:
 * the first memcpy copies 4 bytes = IW_EV_LCP_PK_LEN starting at stream
 * the second memcpy should start where the first left off, i.e.
     memcpy(stream +  IW_EV_LCP_PK_LEN,
            ((char *) &iwe->u) + IW_EV_POINT_OFF,
            IW_EV_POINT_PK_LEN - IW_EV_LCP_PK_LEN);
   where IW_EV_POINT_PK_LEN = IW_EV_LCP_PK_LEN = 8
 * if this were true, then
      int lcp_len   = iwe_stream_lcp_len(info);
   could also go.

But I have not tested any of this. Unless it is clear, please ignore for
the moment, I will test next week and get back.

Re: [Patch 1/1] wext: fix 32/64 bit alignment issue for 'point' type

[Johannes Berg]

On Fri, 2010-10-01 at 08:22 +0100, gerrit@erg.abdn.ac.uk wrote:

> >> The current definition appears to be a typo (PK_LEN instead of LEN); it
> >> causes misalignment on 64 bit systems.
> >
> > So, correct me if I'm wrong, the only effect this change has is changing
> > the second memcpy() in iwe_stream_add_point() to not copy an extra 4
> > bytes that will be overwritten right away by the next memcpy()
> > (presumably, unless the data is < 4, in which case the memcpy might
> > actually be out of bounds).

> Thank you for pointing this out, it seems the change is not as trivial
> as I thought.

Well, I think the change is correct, given that without it we seem to
copy too much in that memcpy() (which is unlikely to matter though)

> I noticed this problem in userspace where the 4 byte difference caused
> misalignment of point types (such as essid) on a 64 bit system.

Ah, so userspace reading this was the issue -- yes, I can see that, and
I checked the "canonical userspace" (wireless-tools), and it essentially
contains this patch.

> I wonder whether the following thinking is right:
>  * the first memcpy copies 4 bytes = IW_EV_LCP_PK_LEN starting at stream
>  * the second memcpy should start where the first left off, i.e.
>      memcpy(stream +  IW_EV_LCP_PK_LEN,
>             ((char *) &iwe->u) + IW_EV_POINT_OFF,
>             IW_EV_POINT_PK_LEN - IW_EV_LCP_PK_LEN);
>    where IW_EV_POINT_PK_LEN = IW_EV_LCP_PK_LEN = 8
>  * if this were true, then
>       int lcp_len   = iwe_stream_lcp_len(info);
>    could also go.
> 
> But I have not tested any of this. Unless it is clear, please ignore for
> the moment, I will test next week and get back.

I think it's a bit more complicated than that, but then this is wext, so
it can't be easy to understand ;)

The format should be
 - iw_event.len (2 bytes)
 - iw_event.cmd (2 bytes)
 - [potentially padding, depending on alignment of union iwreq_data
    in struct iw_event -- lcp_len accounts for this]
 - iw_point.length (2 bytes)
 - iw_point.flags (2 bytes)
 - iw_point data ("extra")

As far as I can tell, the bug that you found means that we copy 4 or 8
too many bytes when we copy in iw_point.{length,flags}, but typically it
won't matter all that much as we neither declare it valid nor will the
destination buffer usually be overrun by it.

This is the only effect on the kernel since this is the only user of
this define that you want to change. In userspace, there are more users,
and they might well run into issues due to the bad version of the
constant, which is most likely why Jean corrected it in
wireless-tools ...

In any case, I consider all this software archaeology, and not really
worth the effort unless something breaks ... nobody can properly review
this code anyway and keep their sanity :-)

johannes

Re: [Patch 1/1] wext: fix 32/64 bit alignment issue for 'point' type

[Gerrit Renker]

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

| Ah, so userspace reading this was the issue -- yes, I can see that, and
| I checked the "canonical userspace" (wireless-tools), and it essentially
| contains this patch.
| 
The change should really be confined only to userspace, please see below.

There are three different alignments for three different cases, which are
caught by iwlib in userspace. Users of wireless.h can be encouraged to use the
latest iwlib, which catches the alignment issues transparently (I have been
given this advice myself).

In the drawings below I have traced with an asterisk where in userspace the data is
read out - as far as I can see this is correct. The only worrying case is number (3).

The cases are:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1) 32 bit kernel
~~~~~~~~~~~~~~~~
 point_len = IW_EV_POINT_LEN = 8
 iwe->len  = event_len = point_len + u.data.length = 8 + u.data.length
 lcp_len   = IW_EV_LCP_LEN   = 4
 2nd memcpy: IW_EV_POINT_PK_LEN - IW_EV_LCP_PK_LEN = 8 - 4 = 4


 IW_EV_LCP_PK_LEN
 <-------------->                *
 +-------+-------+-------+-------+------------ ...-+
 | len   | cmd   |length | flags | extra       ... |
 +-------+-------+-------+-------+------------ ...-+
    2       2       2       2
 
     lcp_len
 <-------------->
 <--1st memcpy--><- 2nd memcpy -><- 3rd memcpy ... >
 
 <--------- point_len ---------->


~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2) 64-bit kernel
~~~~~~~~~~~~~~~~
 point_len = IW_EV_POINT_LEN = 16
 iwe->len  = event_len = point_len + u.data.length = 16 + u.data.length
 lcp_len   = IW_EV_LCP_LEN   = 8
 2nd memcpy: IW_EV_POINT_PK_LEN - IW_EV_LCP_PK_LEN = 12 - 4 = 8

 IW_EV_LCP_PK_LEN
 <-------------->                *                               *
 +-------+-------+---------------+-------+-------+---------------+------------ ...-+
 | len   | cmd   |   (empty)     |length | flags |  (empty)      | extra       ... |
 +-------+-------+---------------+-------+-------+---------------+------------ ...-+
    2       2           4             2      2          4 
 
      lcp_len
 <------------------------------>
 <------- 1st memcpy -----------><--------- 2nd memcpy ---------><- 3rd memcpy ... >
 
 <--------- point_len ------------------------------------------>


~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
3) 64-bit compat
~~~~~~~~~~~~~~~~
 point_len = IW_EV_COMPAT_POINT_LEN = 8
 iwe->len  = event_len = point_len + u.data.length = 8 + u.data.length
 lcp_len   = IW_EV_COMPAT_LCP_LEN   = 4
 2nd memcpy: IW_EV_POINT_PK_LEN - IW_EV_LCP_PK_LEN = 12 - 4 = 8

 IW_EV_LCP_PK_LEN
 <-------------->                *
 +-------+-------+-------+-------+---------------+------- ...-+
 | len   | cmd   |length | flags |  (empty) -> extra      ... |
 +-------+-------+-------+-------+---------------+------- ...-+
    2       2       2       2          4
 
     lcp_len
 <-------------->                <-!! OVERLAP !!>
 <--1st memcpy--><------- 2nd memcpy ----------->
                                 <---- 3rd memcpy ------- ... >
 <--------- point_len ---------->

Here it is as you point out: the second and third memcpy()'s overlap by 4 bytes,
which would be a problem if the length == 0 and no extra data were to be copied.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I attach a patch to fix the last case -- could you please have a look.

[-- Attachment #2: wext.diff --]
[-- Type: text/x-diff, Size: 1153 bytes --]

wext: avoid overlapping memcpy in compat-64 mode

The second memcpy in iwe_stream_add_point() copies:
 * 4 bytes in 32-bit mode (sufficient for length/flags);
 * 8 bytes in 64-bit mode (only first 4 bytes used);
 * 8 bytes in 64-bit compat (overlaps with the third mempcy).

To avoid problems with the third memcpy, this patch reduces the copy length
to always 4 bytes = sizeof(iw_point.length) + sizeof(iw_point.flags)/

Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
 include/net/iw_handler.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- a/include/net/iw_handler.h
+++ b/include/net/iw_handler.h
@@ -548,9 +548,9 @@ iwe_stream_add_point(struct iw_request_info *info, char *stream, char *ends,
 	if(likely((stream + event_len) < ends)) {
 		iwe->len = event_len;
 		memcpy(stream, (char *) iwe, IW_EV_LCP_PK_LEN);
+		/* copy length and flags of iw_point */
 		memcpy(stream + lcp_len,
-		       ((char *) &iwe->u) + IW_EV_POINT_OFF,
-		       IW_EV_POINT_PK_LEN - IW_EV_LCP_PK_LEN);
+		       ((char *) &iwe->u) + IW_EV_POINT_OFF, 4);
 		memcpy(stream + point_len, extra, iwe->u.data.length);
 		stream += event_len;
 	}

Re: [Patch 1/1] wext: fix 32/64 bit alignment issue for 'point' type

[Johannes Berg]

On Mon, 2010-10-11 at 07:14 +0200, Gerrit Renker wrote:

> In the drawings below I have traced with an asterisk where in userspace the data is
> read out - as far as I can see this is correct. The only worrying case is number (3).

> 3) 64-bit compat
> ~~~~~~~~~~~~~~~~
>  point_len = IW_EV_COMPAT_POINT_LEN = 8
>  iwe->len  = event_len = point_len + u.data.length = 8 + u.data.length
>  lcp_len   = IW_EV_COMPAT_LCP_LEN   = 4
>  2nd memcpy: IW_EV_POINT_PK_LEN - IW_EV_LCP_PK_LEN = 12 - 4 = 8
> 
>  IW_EV_LCP_PK_LEN
>  <-------------->                *
>  +-------+-------+-------+-------+---------------+------- ...-+
>  | len   | cmd   |length | flags |  (empty) -> extra      ... |
>  +-------+-------+-------+-------+---------------+------- ...-+
>     2       2       2       2          4
>  
>      lcp_len
>  <-------------->                <-!! OVERLAP !!>
>  <--1st memcpy--><------- 2nd memcpy ----------->
>                                  <---- 3rd memcpy ------- ... >
>  <--------- point_len ---------->
> 
> Here it is as you point out: the second and third memcpy()'s overlap by 4 bytes,
> which would be a problem if the length == 0 and no extra data were to be copied.

This, and the other two cases as well, looks correct.


> --- a/include/net/iw_handler.h
> +++ b/include/net/iw_handler.h
> @@ -548,9 +548,9 @@ iwe_stream_add_point(struct iw_request_info *info, char *stream, char *ends,
>         if(likely((stream + event_len) < ends)) {
>                 iwe->len = event_len;
>                 memcpy(stream, (char *) iwe, IW_EV_LCP_PK_LEN);
> +               /* copy length and flags of iw_point */
>                 memcpy(stream + lcp_len,
> -                      ((char *) &iwe->u) + IW_EV_POINT_OFF,
> -                      IW_EV_POINT_PK_LEN - IW_EV_LCP_PK_LEN);
> +                      ((char *) &iwe->u) + IW_EV_POINT_OFF, 4);
>                 memcpy(stream + point_len, extra, iwe->u.data.length);
>                 stream += event_len;
>         }

However, I don't understand this patch. Your original patch would have
fixed this just as well, since after it

IW_EV_POINT_PK_LEN - IW_EV_LCP_PK_LEN

would be

(IW_EV_LCP_PK_LEN + 4) - IW_EV_LCP_PK_LEN == 4

and at the same time fixes the userspace problem, if anyone really wants
to use wext in new applications that don't use iwlib still...

johannes

Re: [Patch 1/1] wext: fix 32/64 bit alignment issue for 'point' type

[gerrit]

> However, I don't understand this patch. Your original patch would have
> fixed this just as well, since after it
>
> IW_EV_POINT_PK_LEN - IW_EV_LCP_PK_LEN
>
> would be
>
> (IW_EV_LCP_PK_LEN + 4) - IW_EV_LCP_PK_LEN == 4
>
> and at the same time fixes the userspace problem, if anyone really wants
> to use wext in new applications that don't use iwlib still...
>
Good catch, this is much better (I got lost in all the other constants).
I will check if I have not missed anything and resubmit if there are no
side effects.

Thanks a lot for the review and suggestion, this looks good.

Re: [Patch 1/1] wext: fix 32/64 bit alignment issue for 'point' type

[Johannes Berg]

On Mon, 2010-10-11 at 12:56 +0100, gerrit@erg.abdn.ac.uk wrote:
> > However, I don't understand this patch. Your original patch would have
> > fixed this just as well, since after it
> >
> > IW_EV_POINT_PK_LEN - IW_EV_LCP_PK_LEN
> >
> > would be
> >
> > (IW_EV_LCP_PK_LEN + 4) - IW_EV_LCP_PK_LEN == 4
> >
> > and at the same time fixes the userspace problem, if anyone really wants
> > to use wext in new applications that don't use iwlib still...
> >
> Good catch, this is much better (I got lost in all the other constants).
> I will check if I have not missed anything and resubmit if there are no
> side effects.

Alright, thanks. I'm pretty sure you can't have missed anything, since
git grep for the constant shows only this single use, and iwlib ships a
header file that already contains your correction :-)

I'm not sure if there's a real or perceived length limit on git commit
logs, but it might be nice to archive your discussion of the problem
(today's, I mean) in the git log.

Anyway, thanks for investigating this!

johannes

[Patch v2 1/1] wext: fix alignment problem in serializing 'struct iw_point'

[Gerrit Renker]

wext: fix alignment problem in serializing 'struct iw_point'

This fixes a typo in the definition of the serialized length of struct iw_point:
 a) wireless.h is exported to userspace, the typo causes IW_EV_POINT_PK_LEN
    to be 12 on 64-bit, and 8 on 32-bit systems (causing misalignment);
 b) in compat-64 mode iwe_stream_add_point() memcpys overlap (see below).

The second case in  in compat-64 mode looks like (variable names are as in
include/net/iw_handler.h:iwe_stream_add_point()):

 point_len = IW_EV_COMPAT_POINT_LEN = 8
 lcp_len   = IW_EV_COMPAT_LCP_LEN   = 4
 2nd memcpy: IW_EV_POINT_PK_LEN - IW_EV_LCP_PK_LEN = 12 - 4 = 8

 IW_EV_LCP_PK_LEN
 <-------------->                *---> 'extra' data area
 +-------+-------+-------+-------+---------------+------- ...-+
 | len   | cmd   |length | flags |  (empty) -> extra      ... |
 +-------+-------+-------+-------+---------------+------- ...-+
    2       2       2       2          4
 
     lcp_len
 <-------------->                <-!! OVERLAP !!>
 <--1st memcpy--><------- 2nd memcpy ----------->
                                 <---- 3rd memcpy ------- ... >
 <--------- point_len ---------->

This case could cause overrun whenever iw_point.length < 4. 
The other two cases are -
 * 32-bit systems: IW_EV_POINT_PK_LEN - IW_EV_LCP_PK_LEN =  8 - 4 = 4,
   the second memcpy copies exactly the 4 required bytes;
 * 64-bit systems: IW_EV_POINT_PK_LEN - IW_EV_LCP_PK_LEN = 12 - 4 = 8,
   the second memcpy copies a superfluous (but non overlapping) 4 bytes.

The patch changes IW_EV_POINT_PK_LEN to be 8, so that in all 3 cases always only
the requested iw_point.{length,flags} (both __u16) are copied, avoiding overrrun
(compat-64) and superfluous copy (64-bit). In addition, the userspace header is
sanitized (in agreement with version 30 of the wireless tools).

Many thanks to Johannes Berg for help and review with this patch.
    
Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
 include/linux/wireless.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
	
--- a/include/linux/wireless.h
+++ b/include/linux/wireless.h
@@ -1157,6 +1157,6 @@ struct __compat_iw_event {
 #define IW_EV_PARAM_PK_LEN	(IW_EV_LCP_PK_LEN + sizeof(struct iw_param))
 #define IW_EV_ADDR_PK_LEN	(IW_EV_LCP_PK_LEN + sizeof(struct sockaddr))
 #define IW_EV_QUAL_PK_LEN	(IW_EV_LCP_PK_LEN + sizeof(struct iw_quality))
-#define IW_EV_POINT_PK_LEN	(IW_EV_LCP_LEN + 4)
+#define IW_EV_POINT_PK_LEN	(IW_EV_LCP_PK_LEN + 4)
 
 #endif	/* _LINUX_WIRELESS_H */