Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1609053 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] cfg80211: Fix array-bounds warning in fragment copy Matthias Kaehlcke <mka@chromium.org> - 2017-03-25 02:10 +0100
    Re: [PATCH] cfg80211: Fix array-bounds warning in fragment copy Johannes Berg <johannes@sipsolutions.net> - 2017-03-27 12:50 +0200
      Re: [PATCH] cfg80211: Fix array-bounds warning in fragment copy Felix Fietkau <nbd@nbd.name> - 2017-03-27 13:40 +0200
        Re: [PATCH] cfg80211: Fix array-bounds warning in fragment copy Johannes Berg <johannes@sipsolutions.net> - 2017-03-27 13:40 +0200
      Re: [PATCH] cfg80211: Fix array-bounds warning in fragment copy Matthias Kaehlcke <mka@chromium.org> - 2017-03-27 21:50 +0200

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

__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

[toc] | [next] | [standalone]

#1609730

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

[toc] | [prev] | [next] | [standalone]

#1609744

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

[toc] | [prev] | [next] | [standalone]

#1609749

> > > -	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

[toc] | [prev] | [next] | [standalone]

#1610093

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web