[PATCH 5/9] drivers/net/wireless: Correct code taking the size of a pointer

[PATCH 5/9] drivers/net/wireless: Correct code taking the size of a pointer

[Julia Lawall]

From: Julia Lawall <julia@diku.dk>

sizeof(iv16) and sizeof(iv32) are the sizes of pointers.  Change them to
the size of the copied data.

A simplified version of the semantic patch that finds this problem is as
follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression *x;
expression f;
type T;
@@

*f(...,(T)x,...)
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

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

diff --git a/drivers/net/wireless/rt2x00/rt2800lib.c b/drivers/net/wireless/rt2x00/rt2800lib.c
index eb1e1d0..1e74732 100644
--- a/drivers/net/wireless/rt2x00/rt2800lib.c
+++ b/drivers/net/wireless/rt2x00/rt2800lib.c
@@ -2140,8 +2140,8 @@ static void rt2800_get_tkip_seq(struct ieee80211_hw *hw, u8 hw_key_idx,
 	rt2800_register_multiread(rt2x00dev, offset,
 				      &iveiv_entry, sizeof(iveiv_entry));
 
-	memcpy(&iveiv_entry.iv[0], iv16, sizeof(iv16));
-	memcpy(&iveiv_entry.iv[4], iv32, sizeof(iv32));
+	memcpy(&iveiv_entry.iv[0], iv16, sizeof(*iv16));
+	memcpy(&iveiv_entry.iv[4], iv32, sizeof(*iv32));
 }
 
 static int rt2800_set_rts_threshold(struct ieee80211_hw *hw, u32 value)

Re: [PATCH 5/9] drivers/net/wireless: Correct code taking the size of a pointer

[Andreas Schwab]

Julia Lawall <julia@diku.dk> writes:

> diff --git a/drivers/net/wireless/rt2x00/rt2800lib.c b/drivers/net/wireless/rt2x00/rt2800lib.c
> index eb1e1d0..1e74732 100644
> --- a/drivers/net/wireless/rt2x00/rt2800lib.c
> +++ b/drivers/net/wireless/rt2x00/rt2800lib.c
> @@ -2140,8 +2140,8 @@ static void rt2800_get_tkip_seq(struct ieee80211_hw *hw, u8 hw_key_idx,
>  	rt2800_register_multiread(rt2x00dev, offset,
>  				      &iveiv_entry, sizeof(iveiv_entry));
>  
> -	memcpy(&iveiv_entry.iv[0], iv16, sizeof(iv16));
> -	memcpy(&iveiv_entry.iv[4], iv32, sizeof(iv32));
> +	memcpy(&iveiv_entry.iv[0], iv16, sizeof(*iv16));
> +	memcpy(&iveiv_entry.iv[4], iv32, sizeof(*iv32));

That still looks pretty bogus, the memcpy calls are overwriting the
values written by the previous rt2800_register_multiread call.  Most
likely the first two arguments need to be swapped.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH 5/9] drivers/net/wireless: Correct code taking the size of a pointer

[Julia Lawall]

On Sun, 13 Dec 2009, Andreas Schwab wrote:

> Julia Lawall <julia@diku.dk> writes:
> 
> > diff --git a/drivers/net/wireless/rt2x00/rt2800lib.c b/drivers/net/wireless/rt2x00/rt2800lib.c
> > index eb1e1d0..1e74732 100644
> > --- a/drivers/net/wireless/rt2x00/rt2800lib.c
> > +++ b/drivers/net/wireless/rt2x00/rt2800lib.c
> > @@ -2140,8 +2140,8 @@ static void rt2800_get_tkip_seq(struct ieee80211_hw *hw, u8 hw_key_idx,
> >  	rt2800_register_multiread(rt2x00dev, offset,
> >  				      &iveiv_entry, sizeof(iveiv_entry));
> >  
> > -	memcpy(&iveiv_entry.iv[0], iv16, sizeof(iv16));
> > -	memcpy(&iveiv_entry.iv[4], iv32, sizeof(iv32));
> > +	memcpy(&iveiv_entry.iv[0], iv16, sizeof(*iv16));
> > +	memcpy(&iveiv_entry.iv[4], iv32, sizeof(*iv32));
> 
> That still looks pretty bogus, the memcpy calls are overwriting the
> values written by the previous rt2800_register_multiread call.  Most
> likely the first two arguments need to be swapped.

OK, I will look at it and try again.

Thanks,
julia

Re: [PATCH 5/9] drivers/net/wireless: Correct code taking the size of a pointer

[Julia Lawall]

From: Julia Lawall <julia@diku.dk>

sizeof(iv16) and sizeof(iv32) are the sizes of pointers.  Change them to
the size of the copied data.

Furthermore, iveiv_entry is a local structure that has just been
initialized and is not visible outside this function.  Thus, there would
seem to be no point to copy data into it.  The order of the arguments is
thus changed to copy the data into the parameters, which are provided as
pointers, suggesting in this case that they should be used to return values.

A simplified version of the semantic patch that finds the first problem is as
follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression *x;
expression f;
type T;
@@

*f(...,(T)x,...)
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

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

diff --git a/drivers/net/wireless/rt2x00/rt2800lib.c b/drivers/net/wireless/rt2x00/rt2800lib.c
index eb1e1d0..f52b82e 100644
--- a/drivers/net/wireless/rt2x00/rt2800lib.c
+++ b/drivers/net/wireless/rt2x00/rt2800lib.c
@@ -2140,8 +2140,8 @@ static void rt2800_get_tkip_seq(struct ieee80211_hw *hw, u8 hw_key_idx,
 	rt2800_register_multiread(rt2x00dev, offset,
 				      &iveiv_entry, sizeof(iveiv_entry));
 
-	memcpy(&iveiv_entry.iv[0], iv16, sizeof(iv16));
-	memcpy(&iveiv_entry.iv[4], iv32, sizeof(iv32));
+	memcpy(iv16, &iveiv_entry.iv[0], sizeof(*iv16));
+	memcpy(iv32, &iveiv_entry.iv[4], sizeof(*iv32));
 }
 
 static int rt2800_set_rts_threshold(struct ieee80211_hw *hw, u32 value)

Re: [PATCH 5/9] drivers/net/wireless: Correct code taking the size of a pointer

[Julia Lawall]

I have submitted the suggested patch, but I am not sure whether it is 
correct.  Will the memcpy for iv16 be picking up the right two bytes?

julia


On Sun, 13 Dec 2009, Andreas Schwab wrote:

> Julia Lawall <julia@diku.dk> writes:
> 
> > diff --git a/drivers/net/wireless/rt2x00/rt2800lib.c b/drivers/net/wireless/rt2x00/rt2800lib.c
> > index eb1e1d0..1e74732 100644
> > --- a/drivers/net/wireless/rt2x00/rt2800lib.c
> > +++ b/drivers/net/wireless/rt2x00/rt2800lib.c
> > @@ -2140,8 +2140,8 @@ static void rt2800_get_tkip_seq(struct ieee80211_hw *hw, u8 hw_key_idx,
> >  	rt2800_register_multiread(rt2x00dev, offset,
> >  				      &iveiv_entry, sizeof(iveiv_entry));
> >  
> > -	memcpy(&iveiv_entry.iv[0], iv16, sizeof(iv16));
> > -	memcpy(&iveiv_entry.iv[4], iv32, sizeof(iv32));
> > +	memcpy(&iveiv_entry.iv[0], iv16, sizeof(*iv16));
> > +	memcpy(&iveiv_entry.iv[4], iv32, sizeof(*iv32));
> 
> That still looks pretty bogus, the memcpy calls are overwriting the
> values written by the previous rt2800_register_multiread call.  Most
> likely the first two arguments need to be swapped.
> 
> Andreas.
> 
> -- 
> Andreas Schwab, schwab@linux-m68k.org
> GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
> "And now for something completely different."
> 

Re: [PATCH 5/9] drivers/net/wireless: Correct code taking the size of a pointer

[Gertjan van Wingerde]

On 12/13/09 17:07, Julia Lawall wrote:
> From: Julia Lawall <julia@diku.dk>
> 
> sizeof(iv16) and sizeof(iv32) are the sizes of pointers.  Change them to
> the size of the copied data.
> 
> Furthermore, iveiv_entry is a local structure that has just been
> initialized and is not visible outside this function.  Thus, there would
> seem to be no point to copy data into it.  The order of the arguments is
> thus changed to copy the data into the parameters, which are provided as
> pointers, suggesting in this case that they should be used to return values.
> 
> A simplified version of the semantic patch that finds the first problem is as
> follows: (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @@
> expression *x;
> expression f;
> type T;
> @@
> 
> *f(...,(T)x,...)
> // </smpl>
> 
> Signed-off-by: Julia Lawall <julia@diku.dk>

This is certainly better than the original code.

Acked-by: Gertjan van Wingerde <gwingerde@gmail.com>

> 
> ---
>  drivers/net/wireless/rt2x00/rt2800lib.c        |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/rt2x00/rt2800lib.c b/drivers/net/wireless/rt2x00/rt2800lib.c
> index eb1e1d0..f52b82e 100644
> --- a/drivers/net/wireless/rt2x00/rt2800lib.c
> +++ b/drivers/net/wireless/rt2x00/rt2800lib.c
> @@ -2140,8 +2140,8 @@ static void rt2800_get_tkip_seq(struct ieee80211_hw *hw, u8 hw_key_idx,
>  	rt2800_register_multiread(rt2x00dev, offset,
>  				      &iveiv_entry, sizeof(iveiv_entry));
>  
> -	memcpy(&iveiv_entry.iv[0], iv16, sizeof(iv16));
> -	memcpy(&iveiv_entry.iv[4], iv32, sizeof(iv32));
> +	memcpy(iv16, &iveiv_entry.iv[0], sizeof(*iv16));
> +	memcpy(iv32, &iveiv_entry.iv[4], sizeof(*iv32));
>  }
>  
>  static int rt2800_set_rts_threshold(struct ieee80211_hw *hw, u32 value)
> 

Re: [PATCH 5/9] drivers/net/wireless: Correct code taking the size of a pointer

[Ivo van Doorn]

On Monday 14 December 2009, Gertjan van Wingerde wrote:
> On 12/13/09 17:07, Julia Lawall wrote:
> > From: Julia Lawall <julia@diku.dk>
> > 
> > sizeof(iv16) and sizeof(iv32) are the sizes of pointers.  Change them to
> > the size of the copied data.
> > 
> > Furthermore, iveiv_entry is a local structure that has just been
> > initialized and is not visible outside this function.  Thus, there would
> > seem to be no point to copy data into it.  The order of the arguments is
> > thus changed to copy the data into the parameters, which are provided as
> > pointers, suggesting in this case that they should be used to return values.
> > 
> > A simplified version of the semantic patch that finds the first problem is as
> > follows: (http://coccinelle.lip6.fr/)
> > 
> > // <smpl>
> > @@
> > expression *x;
> > expression f;
> > type T;
> > @@
> > 
> > *f(...,(T)x,...)
> > // </smpl>
> > 
> > Signed-off-by: Julia Lawall <julia@diku.dk>
> 
> This is certainly better than the original code.
> 
> Acked-by: Gertjan van Wingerde <gwingerde@gmail.com>

Acked-by: Ivo van Doorn <IvDoorn@gmail.com>

> > 
> > ---
> >  drivers/net/wireless/rt2x00/rt2800lib.c        |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/rt2x00/rt2800lib.c b/drivers/net/wireless/rt2x00/rt2800lib.c
> > index eb1e1d0..f52b82e 100644
> > --- a/drivers/net/wireless/rt2x00/rt2800lib.c
> > +++ b/drivers/net/wireless/rt2x00/rt2800lib.c
> > @@ -2140,8 +2140,8 @@ static void rt2800_get_tkip_seq(struct ieee80211_hw *hw, u8 hw_key_idx,
> >  	rt2800_register_multiread(rt2x00dev, offset,
> >  				      &iveiv_entry, sizeof(iveiv_entry));
> >  
> > -	memcpy(&iveiv_entry.iv[0], iv16, sizeof(iv16));
> > -	memcpy(&iveiv_entry.iv[4], iv32, sizeof(iv32));
> > +	memcpy(iv16, &iveiv_entry.iv[0], sizeof(*iv16));
> > +	memcpy(iv32, &iveiv_entry.iv[4], sizeof(*iv32));
> >  }
> >  
> >  static int rt2800_set_rts_threshold(struct ieee80211_hw *hw, u32 value)
> > 
> 
>