[PATCH V2 1/2] wl12xx: consider encryption when configuring auto arp template

[PATCH V2 1/2] wl12xx: consider encryption when configuring auto arp template

[Eliad Peller]

When configuring the arp response template, and encryption is enabled,
we should add some space and set the protected flag bit in the fc.

In order to track the encryption type, set wl->encryption_type when
setting an encryption key, and reconfigure the arp response.
Clear this field on wl1271_join, as keys have to be re-configured
anyway after a join command.

Signed-off-by: Eliad Peller <eliad@wizery.com>
---
v1 -> v2:
	don't configure template in ap mode

 drivers/net/wireless/wl12xx/cmd.c          |   65 ++++++++++++++++++++-------
 drivers/net/wireless/wl12xx/main.c         |   19 ++++++++-
 drivers/net/wireless/wl12xx/wl12xx.h       |    4 ++
 drivers/net/wireless/wl12xx/wl12xx_80211.h |    2 +-
 4 files changed, 71 insertions(+), 19 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/cmd.c b/drivers/net/wireless/wl12xx/cmd.c
index 2468044..65bcecb 100644
--- a/drivers/net/wireless/wl12xx/cmd.c
+++ b/drivers/net/wireless/wl12xx/cmd.c
@@ -742,28 +742,30 @@ out:
 
 int wl1271_cmd_build_arp_rsp(struct wl1271 *wl, __be32 ip_addr)
 {
-	int ret;
-	struct wl12xx_arp_rsp_template tmpl;
+	int ret, extra = 0;
+	u16 fc;
+	struct sk_buff *skb;
+	struct wl12xx_arp_rsp_template *tmpl;
 	struct ieee80211_hdr_3addr *hdr;
 	struct arphdr *arp_hdr;
 
-	memset(&tmpl, 0, sizeof(tmpl));
+	skb = dev_alloc_skb(sizeof(*hdr) + sizeof(*tmpl) + 8);
+	if (!skb) {
+		wl1271_error("failed to allocate buffer for arp rsp template");
+		return -ENOMEM;
+	}
 
-	/* mac80211 header */
-	hdr = &tmpl.hdr;
-	hdr->frame_control = cpu_to_le16(IEEE80211_FTYPE_DATA |
-					 IEEE80211_STYPE_DATA |
-					 IEEE80211_FCTL_TODS);
-	memcpy(hdr->addr1, wl->vif->bss_conf.bssid, ETH_ALEN);
-	memcpy(hdr->addr2, wl->vif->addr, ETH_ALEN);
-	memset(hdr->addr3, 0xff, ETH_ALEN);
+	skb_reserve(skb, sizeof(*hdr) + 8);
+
+	tmpl = (struct wl12xx_arp_rsp_template *)skb_put(skb, sizeof(*tmpl));
+	memset(tmpl, 0, sizeof(tmpl));
 
 	/* llc layer */
-	memcpy(tmpl.llc_hdr, rfc1042_header, sizeof(rfc1042_header));
-	tmpl.llc_type = cpu_to_be16(ETH_P_ARP);
+	memcpy(tmpl->llc_hdr, rfc1042_header, sizeof(rfc1042_header));
+	tmpl->llc_type = cpu_to_be16(ETH_P_ARP);
 
 	/* arp header */
-	arp_hdr = &tmpl.arp_hdr;
+	arp_hdr = &tmpl->arp_hdr;
 	arp_hdr->ar_hrd = cpu_to_be16(ARPHRD_ETHER);
 	arp_hdr->ar_pro = cpu_to_be16(ETH_P_IP);
 	arp_hdr->ar_hln = ETH_ALEN;
@@ -771,12 +773,41 @@ int wl1271_cmd_build_arp_rsp(struct wl1271 *wl, __be32 ip_addr)
 	arp_hdr->ar_op = cpu_to_be16(ARPOP_REPLY);
 
 	/* arp payload */
-	memcpy(tmpl.sender_hw, wl->vif->addr, ETH_ALEN);
-	tmpl.sender_ip = ip_addr;
+	memcpy(tmpl->sender_hw, wl->vif->addr, ETH_ALEN);
+	tmpl->sender_ip = ip_addr;
+
+	/* encryption space */
+	switch (wl->encryption_type) {
+	case KEY_TKIP:
+		extra = WL1271_TKIP_IV_SPACE;
+		break;
+	case KEY_AES:
+		extra = 8;
+		break;
+	}
+
+	if (extra) {
+		u8 *space = skb_push(skb, extra);
+		memset(space, 0, extra);
+	}
+
+	/* mac80211 header */
+	hdr = (struct ieee80211_hdr_3addr *)skb_push(skb, sizeof(*hdr));
+	memset(hdr, 0, sizeof(hdr));
+	fc = IEEE80211_FTYPE_DATA | IEEE80211_STYPE_DATA | IEEE80211_FCTL_TODS;
+	if (wl->encryption_type != KEY_NONE)
+		fc |= IEEE80211_FCTL_PROTECTED;
+
+	hdr->frame_control = cpu_to_le16(fc);
+	memcpy(hdr->addr1, wl->vif->bss_conf.bssid, ETH_ALEN);
+	memcpy(hdr->addr2, wl->vif->addr, ETH_ALEN);
+	memset(hdr->addr3, 0xff, ETH_ALEN);
 
 	ret = wl1271_cmd_template_set(wl, CMD_TEMPL_ARP_RSP,
-				      &tmpl, sizeof(tmpl), 0,
+				      skb->data, skb->len, 0,
 				      wl->basic_rate);
+	dev_kfree_skb(skb);
+
 
 	return ret;
 }
diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
index 7126506..2ec15bf 100644
--- a/drivers/net/wireless/wl12xx/main.c
+++ b/drivers/net/wireless/wl12xx/main.c
@@ -1623,6 +1623,9 @@ static int wl1271_join(struct wl1271 *wl, bool set_assoc)
 	if (test_bit(WL1271_FLAG_STA_ASSOCIATED, &wl->flags))
 		wl1271_info("JOIN while associated.");
 
+	/* clear encryption type */
+	wl->encryption_type = KEY_NONE;
+
 	if (set_assoc)
 		set_bit(WL1271_FLAG_STA_ASSOCIATED, &wl->flags);
 
@@ -2196,6 +2199,17 @@ static int wl1271_op_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
 			wl1271_error("Could not add or replace key");
 			goto out_sleep;
 		}
+
+		/* reconfiguring arp response (data packet) */
+		if (wl->bss_type == BSS_TYPE_STA_BSS &&
+		    wl->encryption_type != key_type) {
+			wl->encryption_type = key_type;
+			ret = wl1271_cmd_build_arp_rsp(wl, wl->ip_addr);
+			if (ret < 0) {
+				wl1271_warning("build arp rsp failed: %d", ret);
+				goto out_sleep;
+			}
+		}
 		break;
 
 	case DISABLE_KEY:
@@ -2734,6 +2748,7 @@ static void wl1271_bss_info_changed_sta(struct wl1271 *wl,
 
 		if (bss_conf->arp_addr_cnt == 1 &&
 		    bss_conf->arp_filter_enabled) {
+			wl->ip_addr = addr;
 			/*
 			 * The template should have been configured only upon
 			 * association. however, it seems that the correct ip
@@ -2749,8 +2764,10 @@ static void wl1271_bss_info_changed_sta(struct wl1271 *wl,
 			ret = wl1271_acx_arp_ip_filter(wl,
 				ACX_ARP_FILTER_ARP_FILTERING,
 				addr);
-		} else
+		} else {
+			wl->ip_addr = 0;
 			ret = wl1271_acx_arp_ip_filter(wl, 0, addr);
+		}
 
 		if (ret < 0)
 			goto out;
diff --git a/drivers/net/wireless/wl12xx/wl12xx.h b/drivers/net/wireless/wl12xx/wl12xx.h
index fb2b79f..b36a406 100644
--- a/drivers/net/wireless/wl12xx/wl12xx.h
+++ b/drivers/net/wireless/wl12xx/wl12xx.h
@@ -556,6 +556,10 @@ struct wl1271 {
 	/* bands supported by this instance of wl12xx */
 	struct ieee80211_supported_band bands[IEEE80211_NUM_BANDS];
 
+	/* save the current encryption type for auto-arp config*/
+	u8 encryption_type;
+	__be32 ip_addr;
+
 	/* RX BA constraint value */
 	bool ba_support;
 	u8 ba_rx_bitmap;
diff --git a/drivers/net/wireless/wl12xx/wl12xx_80211.h b/drivers/net/wireless/wl12xx/wl12xx_80211.h
index 18fe542..71e29aa 100644
--- a/drivers/net/wireless/wl12xx/wl12xx_80211.h
+++ b/drivers/net/wireless/wl12xx/wl12xx_80211.h
@@ -134,7 +134,7 @@ struct wl12xx_qos_null_data_template {
 } __packed;
 
 struct wl12xx_arp_rsp_template {
-	struct ieee80211_hdr_3addr hdr;
+	/* not including ieee80211 header */
 
 	u8 llc_hdr[sizeof(rfc1042_header)];
 	__be16 llc_type;
-- 
1.7.0.4

[PATCH V2 2/2] wl12xx: Revert "wl12xx: disable auto-arp"

[Eliad Peller]

This reverts commit e5e2f24b3eec67a7a35d43654a997f98ca21aff2.

The encryption consideration on auto-arp configuration seems
to resolve the crashes that were occured when auto-arp was enabled,
so we can re-enable it now.

Signed-off-by: Eliad Peller <eliad@wizery.com>
---
 drivers/net/wireless/wl12xx/main.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
index 2ec15bf..7ebee3d 100644
--- a/drivers/net/wireless/wl12xx/main.c
+++ b/drivers/net/wireless/wl12xx/main.c
@@ -2762,7 +2762,8 @@ static void wl1271_bss_info_changed_sta(struct wl1271 *wl,
 			}
 
 			ret = wl1271_acx_arp_ip_filter(wl,
-				ACX_ARP_FILTER_ARP_FILTERING,
+				(ACX_ARP_FILTER_ARP_FILTERING |
+				 ACX_ARP_FILTER_AUTO_ARP),
 				addr);
 		} else {
 			wl->ip_addr = 0;
-- 
1.7.0.4

Re: [PATCH V2 1/2] wl12xx: consider encryption when configuring auto arp template

[Luciano Coelho]

Hi Eliad,

On Thu, 2011-04-14 at 12:47 +0300, Eliad Peller wrote:
> When configuring the arp response template, and encryption is enabled,
> we should add some space and set the protected flag bit in the fc.
> 
> In order to track the encryption type, set wl->encryption_type when
> setting an encryption key, and reconfigure the arp response.
> Clear this field on wl1271_join, as keys have to be re-configured
> anyway after a join command.
> 
> Signed-off-by: Eliad Peller <eliad@wizery.com>
> ---

A couple of style comments.


> diff --git a/drivers/net/wireless/wl12xx/cmd.c b/drivers/net/wireless/wl12xx/cmd.c
> index 2468044..65bcecb 100644
> --- a/drivers/net/wireless/wl12xx/cmd.c
> +++ b/drivers/net/wireless/wl12xx/cmd.c
> @@ -742,28 +742,30 @@ out:
>  
>  int wl1271_cmd_build_arp_rsp(struct wl1271 *wl, __be32 ip_addr)
>  {
> -	int ret;
> -	struct wl12xx_arp_rsp_template tmpl;
> +	int ret, extra = 0;
> +	u16 fc;
> +	struct sk_buff *skb;
> +	struct wl12xx_arp_rsp_template *tmpl;
>  	struct ieee80211_hdr_3addr *hdr;
>  	struct arphdr *arp_hdr;
>  
> -	memset(&tmpl, 0, sizeof(tmpl));
> +	skb = dev_alloc_skb(sizeof(*hdr) + sizeof(*tmpl) + 8);

[...]

> +	skb_reserve(skb, sizeof(*hdr) + 8);

What's 8? Maybe it would be better to make it clear with something like
WL1271_EXTRA_SPACE_MAX?


> @@ -771,12 +773,41 @@ int wl1271_cmd_build_arp_rsp(struct wl1271 *wl, __be32 ip_addr)
>  	arp_hdr->ar_op = cpu_to_be16(ARPOP_REPLY);
>  
>  	/* arp payload */
> -	memcpy(tmpl.sender_hw, wl->vif->addr, ETH_ALEN);
> -	tmpl.sender_ip = ip_addr;
> +	memcpy(tmpl->sender_hw, wl->vif->addr, ETH_ALEN);
> +	tmpl->sender_ip = ip_addr;
> +
> +	/* encryption space */
> +	switch (wl->encryption_type) {
> +	case KEY_TKIP:
> +		extra = WL1271_TKIP_IV_SPACE;
> +		break;
> +	case KEY_AES:
> +		extra = 8;
> +		break;
> +	}

It would be nicer to have WL1271_EXTRA_SPACE_AES here instead.  And
maybe rename the WL1271_TKIP_IV_SPACE to WL1271_EXTRA_SPACE_TKIP, to
make it consistent and clearer?

Maybe we should also complete the switch and explicitly set extra to
zero for KEY_NONE, KEY_WEP, KEY_GEM and default?


> @@ -2734,6 +2748,7 @@ static void wl1271_bss_info_changed_sta(struct wl1271 *wl,
>  
>  		if (bss_conf->arp_addr_cnt == 1 &&
>  		    bss_conf->arp_filter_enabled) {
> +			wl->ip_addr = addr;
>  			/*
>  			 * The template should have been configured only upon
>  			 * association. however, it seems that the correct ip
> @@ -2749,8 +2764,10 @@ static void wl1271_bss_info_changed_sta(struct wl1271 *wl,
>  			ret = wl1271_acx_arp_ip_filter(wl,
>  				ACX_ARP_FILTER_ARP_FILTERING,
>  				addr);
> -		} else
> +		} else {
> +			wl->ip_addr = 0;
>  			ret = wl1271_acx_arp_ip_filter(wl, 0, addr);
> +		}

This is not important at all, but maybe it's a bit cleaner: you could
remove the addr argument in wl1271_acx_arp_ip_filter() and always use
wl->ip_addr instead? You could then also get rid of the local addr here.


-- 
Cheers,
Luca.

Re: [PATCH V2 1/2] wl12xx: consider encryption when configuring auto arp template

[Eliad Peller]

hi Luca,

On Tue, Apr 26, 2011 at 9:53 AM, Luciano Coelho <coelho@ti.com> wrote:
> On Thu, 2011-04-14 at 12:47 +0300, Eliad Peller wrote:
>> When configuring the arp response template, and encryption is enabled,
>> we should add some space and set the protected flag bit in the fc.
>>
>> In order to track the encryption type, set wl->encryption_type when
>> setting an encryption key, and reconfigure the arp response.
>> Clear this field on wl1271_join, as keys have to be re-configured
>> anyway after a join command.
>>
>> Signed-off-by: Eliad Peller <eliad@wizery.com>
>> ---
>
> A couple of style comments.
> [...]
>
>> +     skb_reserve(skb, sizeof(*hdr) + 8);
>
> What's 8? Maybe it would be better to make it clear with something like
> WL1271_EXTRA_SPACE_MAX?
>
sure.

>
>> @@ -771,12 +773,41 @@ int wl1271_cmd_build_arp_rsp(struct wl1271 *wl, __be32 ip_addr)
>>       arp_hdr->ar_op = cpu_to_be16(ARPOP_REPLY);
>>
>>       /* arp payload */
>> -     memcpy(tmpl.sender_hw, wl->vif->addr, ETH_ALEN);
>> -     tmpl.sender_ip = ip_addr;
>> +     memcpy(tmpl->sender_hw, wl->vif->addr, ETH_ALEN);
>> +     tmpl->sender_ip = ip_addr;
>> +
>> +     /* encryption space */
>> +     switch (wl->encryption_type) {
>> +     case KEY_TKIP:
>> +             extra = WL1271_TKIP_IV_SPACE;
>> +             break;
>> +     case KEY_AES:
>> +             extra = 8;
>> +             break;
>> +     }
>
> It would be nicer to have WL1271_EXTRA_SPACE_AES here instead.  And
> maybe rename the WL1271_TKIP_IV_SPACE to WL1271_EXTRA_SPACE_TKIP, to
> make it consistent and clearer?
>
WL1271_TKIP_IV_SPACE is already being used in tx.c.
It's a bit tricky, as while we use the WL1271_TKIP_IV_SPACE (or
WL1271_EXTRA_SPACE_TKIP) in our tx path, we won't use
WL1271_EXTRA_SPACE_AES in the tx path, because we set the
IEEE80211_KEY_FLAG_GENERATE_IV for the key.
anyway, i'll just add it :)

> Maybe we should also complete the switch and explicitly set extra to
> zero for KEY_NONE, KEY_WEP, KEY_GEM and default?
>
>
>> @@ -2734,6 +2748,7 @@ static void wl1271_bss_info_changed_sta(struct wl1271 *wl,
>>
>>               if (bss_conf->arp_addr_cnt == 1 &&
>>                   bss_conf->arp_filter_enabled) {
>> +                     wl->ip_addr = addr;
>>                       /*
>>                        * The template should have been configured only upon
>>                        * association. however, it seems that the correct ip
>> @@ -2749,8 +2764,10 @@ static void wl1271_bss_info_changed_sta(struct wl1271 *wl,
>>                       ret = wl1271_acx_arp_ip_filter(wl,
>>                               ACX_ARP_FILTER_ARP_FILTERING,
>>                               addr);
>> -             } else
>> +             } else {
>> +                     wl->ip_addr = 0;
>>                       ret = wl1271_acx_arp_ip_filter(wl, 0, addr);
>> +             }
>
> This is not important at all, but maybe it's a bit cleaner: you could
> remove the addr argument in wl1271_acx_arp_ip_filter() and always use
> wl->ip_addr instead? You could then also get rid of the local addr here.
>
in fact, since we are going toward multi-role fw, i do try to avoid
using the global wl struct as much as possible.
anyway, i didn't change this function, so i don't think it's the right
patch to change it.
(we should probably change a lot of functions after we'll have a
per-interface data)

thanks for the review,
Eliad.

Re: [PATCH V2 1/2] wl12xx: consider encryption when configuring auto arp template

[Luciano Coelho]

On Tue, 2011-04-26 at 11:39 +0300, Eliad Peller wrote:
> >> @@ -2749,8 +2764,10 @@ static void
> wl1271_bss_info_changed_sta(struct wl1271 *wl,
> >>                       ret = wl1271_acx_arp_ip_filter(wl,
> >>                               ACX_ARP_FILTER_ARP_FILTERING,
> >>                               addr);
> >> -             } else
> >> +             } else {
> >> +                     wl->ip_addr = 0;
> >>                       ret = wl1271_acx_arp_ip_filter(wl, 0, addr);
> >> +             }
> >
> > This is not important at all, but maybe it's a bit cleaner: you
> could
> > remove the addr argument in wl1271_acx_arp_ip_filter() and always
> use
> > wl->ip_addr instead? You could then also get rid of the local addr
> here.
> >
> in fact, since we are going toward multi-role fw, i do try to avoid
> using the global wl struct as much as possible.
> anyway, i didn't change this function, so i don't think it's the right
> patch to change it.
> (we should probably change a lot of functions after we'll have a
> per-interface data) 

Agreed. :)

-- 
Cheers,
Luca.