Re: [PATCH] cfg80211: Fix array-bounds warning in fragment copy

2017-03-27 Thread Matthias Kaehlcke 
El Mon, Mar 27, 2017 at 12:47:59PM +0200 Johannes Berg ha dit:

> On Fri, 2017-03-24 at 18:06 -0700, Matthias Kaehlcke wrote:
> > __ieee80211_amsdu_copy_frag intentionally initializes a pointer to
> > array[-1] to increment it later to valid values. clang rightfully
> > generates an array-bounds warning on the initialization statement.
> > Work around this by initializing the pointer to array[0] and
> > decrementing it later, which allows to leave the rest of the
> > algorithm untouched.
> > 
> > Signed-off-by: Matthias Kaehlcke 
> > ---
> >  net/wireless/util.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/wireless/util.c b/net/wireless/util.c
> > index 68e5f2ecee1a..d3d459e4a070 100644
> > --- a/net/wireless/util.c
> > +++ b/net/wireless/util.c
> > @@ -659,7 +659,7 @@ __ieee80211_amsdu_copy_frag(struct sk_buff *skb,
> > struct sk_buff *frame,
> >     int offset, int len)
> >  {
> >     struct skb_shared_info *sh = skb_shinfo(skb);
> > -   const skb_frag_t *frag = >frags[-1];
> > +   const skb_frag_t *frag = >frags[0];
> >     struct page *frag_page;
> >     void *frag_ptr;
> >     int frag_len, frag_size;
> > @@ -669,6 +669,7 @@ __ieee80211_amsdu_copy_frag(struct sk_buff *skb,
> > struct sk_buff *frame,
> >     frag_page = virt_to_head_page(skb->head);
> >     frag_ptr = skb->data;
> >     frag_size = head_size;
> > +   frag--;
> 
> Isn't it just a question of time until the compiler will see through
> this trick and warn about it?

Maybe.

Actually it seems the algorithm can be easily adapted to increment the
pointer after consumption, which is clearer anyway. I will give this a
shot. I'm not sure how to exercise the code path for testing and would
appreciate help on this end.

Matthias

Re: [PATCH] cfg80211: Fix array-bounds warning in fragment copy

2017-03-27 Thread Johannes Berg 

> > > - const skb_frag_t *frag = >frags[-1];
> > > + const skb_frag_t *frag = >frags[0];
[...]
> > > + frag--;
> > 
> > Isn't it just a question of time until the compiler will see
> > through this trick and warn about it?
> 
> Frag is incremented again before being accessed, so there is nothing
> for the compiler to see through here.

But by that argument the existing code was already fine. The compiler
flagged it nonetheless, perhaps because it couldn't prove it was
incremented unconditionally/in all branches?

johannes

Re: [PATCH] cfg80211: Fix array-bounds warning in fragment copy

2017-03-27 Thread Felix Fietkau 
On 2017-03-27 12:47, Johannes Berg wrote:
> On Fri, 2017-03-24 at 18:06 -0700, Matthias Kaehlcke wrote:
>> __ieee80211_amsdu_copy_frag intentionally initializes a pointer to
>> array[-1] to increment it later to valid values. clang rightfully
>> generates an array-bounds warning on the initialization statement.
>> Work around this by initializing the pointer to array[0] and
>> decrementing it later, which allows to leave the rest of the
>> algorithm untouched.
>> 
>> Signed-off-by: Matthias Kaehlcke 
>> ---
>>  net/wireless/util.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/net/wireless/util.c b/net/wireless/util.c
>> index 68e5f2ecee1a..d3d459e4a070 100644
>> --- a/net/wireless/util.c
>> +++ b/net/wireless/util.c
>> @@ -659,7 +659,7 @@ __ieee80211_amsdu_copy_frag(struct sk_buff *skb,
>> struct sk_buff *frame,
>>  int offset, int len)
>>  {
>>  struct skb_shared_info *sh = skb_shinfo(skb);
>> -const skb_frag_t *frag = >frags[-1];
>> +const skb_frag_t *frag = >frags[0];
>>  struct page *frag_page;
>>  void *frag_ptr;
>>  int frag_len, frag_size;
>> @@ -669,6 +669,7 @@ __ieee80211_amsdu_copy_frag(struct sk_buff *skb,
>> struct sk_buff *frame,
>>  frag_page = virt_to_head_page(skb->head);
>>  frag_ptr = skb->data;
>>  frag_size = head_size;
>> +frag--;
> 
> Isn't it just a question of time until the compiler will see through
> this trick and warn about it?
Frag is incremented again before being accessed, so there is nothing for
the compiler to see through here.

Acked-by: Felix Fietkau 

- Felix

Re: [PATCH] cfg80211: Fix array-bounds warning in fragment copy

2017-03-27 Thread Johannes Berg 
On Fri, 2017-03-24 at 18:06 -0700, Matthias Kaehlcke wrote:
> __ieee80211_amsdu_copy_frag intentionally initializes a pointer to
> array[-1] to increment it later to valid values. clang rightfully
> generates an array-bounds warning on the initialization statement.
> Work around this by initializing the pointer to array[0] and
> decrementing it later, which allows to leave the rest of the
> algorithm untouched.
> 
> Signed-off-by: Matthias Kaehlcke 
> ---
>  net/wireless/util.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/wireless/util.c b/net/wireless/util.c
> index 68e5f2ecee1a..d3d459e4a070 100644
> --- a/net/wireless/util.c
> +++ b/net/wireless/util.c
> @@ -659,7 +659,7 @@ __ieee80211_amsdu_copy_frag(struct sk_buff *skb,
> struct sk_buff *frame,
>   int offset, int len)
>  {
>   struct skb_shared_info *sh = skb_shinfo(skb);
> - const skb_frag_t *frag = >frags[-1];
> + const skb_frag_t *frag = >frags[0];
>   struct page *frag_page;
>   void *frag_ptr;
>   int frag_len, frag_size;
> @@ -669,6 +669,7 @@ __ieee80211_amsdu_copy_frag(struct sk_buff *skb,
> struct sk_buff *frame,
>   frag_page = virt_to_head_page(skb->head);
>   frag_ptr = skb->data;
>   frag_size = head_size;
> + frag--;

Isn't it just a question of time until the compiler will see through
this trick and warn about it?

johannes

[PATCH] cfg80211: Fix array-bounds warning in fragment copy

2017-03-24 Thread Matthias Kaehlcke 
__ieee80211_amsdu_copy_frag intentionally initializes a pointer to
array[-1] to increment it later to valid values. clang rightfully
generates an array-bounds warning on the initialization statement.
Work around this by initializing the pointer to array[0] and
decrementing it later, which allows to leave the rest of the
algorithm untouched.

Signed-off-by: Matthias Kaehlcke 
---
 net/wireless/util.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/wireless/util.c b/net/wireless/util.c
index 68e5f2ecee1a..d3d459e4a070 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -659,7 +659,7 @@ __ieee80211_amsdu_copy_frag(struct sk_buff *skb, struct 
sk_buff *frame,
int offset, int len)
 {
struct skb_shared_info *sh = skb_shinfo(skb);
-   const skb_frag_t *frag = >frags[-1];
+   const skb_frag_t *frag = >frags[0];
struct page *frag_page;
void *frag_ptr;
int frag_len, frag_size;
@@ -669,6 +669,7 @@ __ieee80211_amsdu_copy_frag(struct sk_buff *skb, struct 
sk_buff *frame,
frag_page = virt_to_head_page(skb->head);
frag_ptr = skb->data;
frag_size = head_size;
+   frag--;
 
while (offset >= frag_size) {
offset -= frag_size;
-- 
2.12.1.578.ge9c3154ca4-goog