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

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

[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 <mka@chromium.org>
---
 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 = &sh->frags[-1];
+	const skb_frag_t *frag = &sh->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

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

[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 <mka@chromium.org>
> ---
>  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 = &sh->frags[-1];
> +	const skb_frag_t *frag = &sh->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

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

[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 <mka@chromium.org>
>> ---
>>  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 = &sh->frags[-1];
>> +	const skb_frag_t *frag = &sh->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 <nbd@nbd.name>

- Felix

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

[Johannes Berg]

> > > -	const skb_frag_t *frag = &sh->frags[-1];
> > > +	const skb_frag_t *frag = &sh->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

[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 <mka@chromium.org>
> > ---
> >  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 = &sh->frags[-1];
> > +	const skb_frag_t *frag = &sh->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