Re: [PATCH] cfg80211: Fix array-bounds warning in fragment copy
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 = &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
Re: [PATCH] cfg80211: Fix array-bounds warning in fragment copy
> > > - 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
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 = &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 - Felix
Re: [PATCH] cfg80211: Fix array-bounds warning in fragment copy
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 = &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
[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 --- 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