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

[Johannes Berg <johannes@sipsolutions.net>]

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