re: ath6kl: pass only unicast frames for aggregation

re: ath6kl: pass only unicast frames for aggregation

[Dan Carpenter]

Hi Kalle,

Smatch complains that 5694f962964 "ath6kl: pass only unicast frames
for aggregation"

drivers/net/wireless/ath/ath6kl/txrx.c
  1241                          if (conn && ar->intra_bss) {
  1242                                  skb1 = skb;
  1243                                  skb = NULL;
                                        ^^^^^^^^^^^
  1244                          } else if (conn && !ar->intra_bss) {
  1245                                  dev_kfree_skb(skb);
  1246                                  skb = NULL;
                                        ^^^^^^^^^^^
  1247                          }
  1248                  }
  1249                  if (skb1)
  1250                          ath6kl_data_tx(skb1, ar->net_dev);
  1251          }
  1252  
  1253          datap = (struct ethhdr *) skb->data;
                                          ^^^^^^^^^
skb can be NULL.

  1254  

regards,
dan carpenter

Re: ath6kl: pass only unicast frames for aggregation

[Kalle Valo]

Hi Dan,

On 10/05/2011 08:59 AM, Dan Carpenter wrote:
> Smatch complains that 5694f962964 "ath6kl: pass only unicast frames
> for aggregation"
> 
> drivers/net/wireless/ath/ath6kl/txrx.c
>   1241                          if (conn && ar->intra_bss) {
>   1242                                  skb1 = skb;
>   1243                                  skb = NULL;
>                                         ^^^^^^^^^^^
>   1244                          } else if (conn && !ar->intra_bss) {
>   1245                                  dev_kfree_skb(skb);
>   1246                                  skb = NULL;
>                                         ^^^^^^^^^^^
>   1247                          }
>   1248                  }
>   1249                  if (skb1)
>   1250                          ath6kl_data_tx(skb1, ar->net_dev);
>   1251          }
>   1252  
>   1253          datap = (struct ethhdr *) skb->data;
>                                           ^^^^^^^^^
> skb can be NULL.

Good catch, thanks! I should run smatch more, it's a really nice tool.

I think a fix like this would be appropriate. Jouni, what do you think?

(I have just copypasted the patch to thunderbird, I'm sure it's white
space damaged. Sorry about that.)

Kalle

--- a/drivers/net/wireless/ath/ath6kl/txrx.c
+++ b/drivers/net/wireless/ath/ath6kl/txrx.c
@@ -1247,6 +1247,10 @@ void ath6kl_rx(struct htc_target *target, struct
htc_packet *packet)
                }
                if (skb1)
                        ath6kl_data_tx(skb1, ar->net_dev);
+
+               if (skb == NULL)
+                       /* nothing to deliver up the stack */
+                       return;
        }

        datap = (struct ethhdr *) skb->data;

Re: ath6kl: pass only unicast frames for aggregation

[Jouni Malinen]

On Wed, Oct 05, 2011 at 01:09:53PM +0300, Kalle Valo wrote:
> Good catch, thanks! I should run smatch more, it's a really nice tool.

This could have actually been found even before
5694f962964c5162f6b49ddb5d517180bd7d1d98 with more thorough static
analysis since an A-MSDU sent to ath6kl AP would have hit the NULL
pointer dereference in aggr_slice_amsdu().. Anyway, this new commit does
indeed seem to make this much more likely to hit the issue (any data
frame between two associated STAs).

> I think a fix like this would be appropriate. Jouni, what do you think?

> --- a/drivers/net/wireless/ath/ath6kl/txrx.c
> +++ b/drivers/net/wireless/ath/ath6kl/txrx.c
> @@ -1247,6 +1247,10 @@ void ath6kl_rx(struct htc_target *target, struct
> htc_packet *packet)
>                 }
>                 if (skb1)
>                         ath6kl_data_tx(skb1, ar->net_dev);
> +
> +               if (skb == NULL)
> +                       /* nothing to deliver up the stack */
> +                       return;
>         }
> 
>         datap = (struct ethhdr *) skb->data;


This looks like the correct behavior here. However, I would recommend
using braces around any multi-line conditional statement even if it
really is a comment and a single statement that would not, in theory,
require this in C language. Leaving those out here seems to be just
asking for problems should someone add something before the "return;"
line and not notice to add braces at that point. The same comment would
actually apply for the commit 5694f962964c5162f6b49ddb5d517180bd7d1d98,
too. If you want to avoid the extra line an braces, moving the comment
to the end of the return line would work for me.

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: ath6kl: pass only unicast frames for aggregation

[Kalle Valo]

On 10/05/2011 02:57 PM, Jouni Malinen wrote:
> On Wed, Oct 05, 2011 at 01:09:53PM +0300, Kalle Valo wrote:
>> Good catch, thanks! I should run smatch more, it's a really nice tool.
> 
> This could have actually been found even before
> 5694f962964c5162f6b49ddb5d517180bd7d1d98 with more thorough static
> analysis since an A-MSDU sent to ath6kl AP would have hit the NULL
> pointer dereference in aggr_slice_amsdu().. Anyway, this new commit does
> indeed seem to make this much more likely to hit the issue (any data
> frame between two associated STAs).

Good point, I'll mention in the patch how severe this actually is.

>> I think a fix like this would be appropriate. Jouni, what do you think?
> 
>> --- a/drivers/net/wireless/ath/ath6kl/txrx.c
>> +++ b/drivers/net/wireless/ath/ath6kl/txrx.c
>> @@ -1247,6 +1247,10 @@ void ath6kl_rx(struct htc_target *target, struct
>> htc_packet *packet)
>>                 }
>>                 if (skb1)
>>                         ath6kl_data_tx(skb1, ar->net_dev);
>> +
>> +               if (skb == NULL)
>> +                       /* nothing to deliver up the stack */
>> +                       return;
>>         }
>>
>>         datap = (struct ethhdr *) skb->data;
> 
> 
> This looks like the correct behavior here. However, I would recommend
> using braces around any multi-line conditional statement even if it
> really is a comment and a single statement that would not, in theory,
> require this in C language. Leaving those out here seems to be just
> asking for problems should someone add something before the "return;"
> line and not notice to add braces at that point. The same comment would
> actually apply for the commit 5694f962964c5162f6b49ddb5d517180bd7d1d98,
> too. If you want to avoid the extra line an braces, moving the comment
> to the end of the return line would work for me.

I have used to not using braces even there's a comment like here. But
you have a point and I'll change my style.

Thanks for checking this.

Kalle