[patch] staging: rtl8723au: incorrect use of ether_addr_copy()

[patch] staging: rtl8723au: incorrect use of ether_addr_copy()

[Dan Carpenter]

The return from myid() isn't aligned correctly for ether_addr_copy().

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/staging/rtl8723au/core/rtw_mlme_ext.c b/drivers/staging/rtl8723au/core/rtw_mlme_ext.c
index 3eb77de..c8f7890 100644
--- a/drivers/staging/rtl8723au/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8723au/core/rtw_mlme_ext.c
@@ -2402,7 +2402,7 @@ void issue_beacon23a(struct rtw_adapter *padapter, int timeout_ms)
 	mgmt->seq_ctrl = 0;
 
 	ether_addr_copy(mgmt->da, bc_addr);
-	ether_addr_copy(mgmt->sa, myid(&padapter->eeprompriv));
+	memcpy(mgmt->sa, myid(&padapter->eeprompriv), ETH_ALEN);
 	ether_addr_copy(mgmt->bssid, get_my_bssid23a(cur_network));
 
 	/* timestamp will be inserted by hardware */
@@ -2556,7 +2556,7 @@ static void issue_probersp(struct rtw_adapter *padapter, unsigned char *da,
 		cpu_to_le16(IEEE80211_FTYPE_MGMT | IEEE80211_STYPE_PROBE_RESP);
 
 	ether_addr_copy(mgmt->da, da);
-	ether_addr_copy(mgmt->sa, mac);
+	memcpy(mgmt->sa, mac, ETH_ALEN);
 	ether_addr_copy(mgmt->bssid, bssid);
 
 	mgmt->seq_ctrl = cpu_to_le16(IEEE80211_SN_TO_SEQ(pmlmeext->mgnt_seq));
@@ -2718,7 +2718,7 @@ static int _issue_probereq(struct rtw_adapter *padapter,
 		ether_addr_copy(pwlanhdr->addr3, bc_addr);
 	}
 
-	ether_addr_copy(pwlanhdr->addr2, mac);
+	memcpy(pwlanhdr->addr2, mac, ETH_ALEN);
 
 	pwlanhdr->seq_ctrl =
 		cpu_to_le16(IEEE80211_SN_TO_SEQ(pmlmeext->mgnt_seq));
@@ -2863,8 +2863,8 @@ static void issue_auth(struct rtw_adapter *padapter, struct sta_info *psta,
 #ifdef CONFIG_8723AU_AP_MODE
 		unsigned short val16;
 		ether_addr_copy(mgmt->da, psta->hwaddr);
-		ether_addr_copy(mgmt->sa, myid(&padapter->eeprompriv));
-		ether_addr_copy(mgmt->bssid, myid(&padapter->eeprompriv));
+		memcpy(mgmt->sa, myid(&padapter->eeprompriv), ETH_ALEN);
+		memcpy(mgmt->bssid, myid(&padapter->eeprompriv), ETH_ALEN);
 
 		/*  setting auth algo number */
 		val16 = (u16)psta->authalg;
@@ -2895,7 +2895,7 @@ static void issue_auth(struct rtw_adapter *padapter, struct sta_info *psta,
 		struct ieee80211_mgmt *iv_mgmt;
 
 		ether_addr_copy(mgmt->da, get_my_bssid23a(&pmlmeinfo->network));
-		ether_addr_copy(mgmt->sa, myid(&padapter->eeprompriv));
+		memcpy(mgmt->sa, myid(&padapter->eeprompriv), ETH_ALEN);
 		ether_addr_copy(mgmt->bssid,
 				get_my_bssid23a(&pmlmeinfo->network));
 
@@ -3006,7 +3006,7 @@ static void issue_assocrsp(struct rtw_adapter *padapter, unsigned short status,
 	mgmt->frame_control = cpu_to_le16(IEEE80211_FTYPE_MGMT | pkt_type);
 
 	ether_addr_copy(mgmt->da, pstat->hwaddr);
-	ether_addr_copy(mgmt->sa, myid(&padapter->eeprompriv));
+	memcpy(mgmt->sa, myid(&padapter->eeprompriv), ETH_ALEN);
 	ether_addr_copy(mgmt->bssid, get_my_bssid23a(&pmlmeinfo->network));
 
 	mgmt->seq_ctrl = cpu_to_le16(IEEE80211_SN_TO_SEQ(pmlmeext->mgnt_seq));
@@ -3129,7 +3129,7 @@ static void issue_assocreq(struct rtw_adapter *padapter)
 		cpu_to_le16(IEEE80211_FTYPE_MGMT | IEEE80211_STYPE_ASSOC_REQ);
 
 	ether_addr_copy(mgmt->da, get_my_bssid23a(&pmlmeinfo->network));
-	ether_addr_copy(mgmt->sa, myid(&padapter->eeprompriv));
+	memcpy(mgmt->sa, myid(&padapter->eeprompriv), ETH_ALEN);
 	ether_addr_copy(mgmt->bssid, get_my_bssid23a(&pmlmeinfo->network));
 
 	mgmt->seq_ctrl = cpu_to_le16(IEEE80211_SN_TO_SEQ(pmlmeext->mgnt_seq));
@@ -3400,7 +3400,7 @@ static int _issue_nulldata23a(struct rtw_adapter *padapter, unsigned char *da,
 		pwlanhdr->frame_control |= cpu_to_le16(IEEE80211_FCTL_PM);
 
 	ether_addr_copy(pwlanhdr->addr1, da);
-	ether_addr_copy(pwlanhdr->addr2, myid(&padapter->eeprompriv));
+	memcpy(pwlanhdr->addr2, myid(&padapter->eeprompriv), ETH_ALEN);
 	ether_addr_copy(pwlanhdr->addr3, get_my_bssid23a(&pmlmeinfo->network));
 
 	pwlanhdr->seq_ctrl =
@@ -3528,7 +3528,7 @@ static int _issue_qos_nulldata23a(struct rtw_adapter *padapter,
 		pwlanhdr->qos_ctrl |= cpu_to_le16(IEEE80211_QOS_CTL_EOSP);
 
 	ether_addr_copy(pwlanhdr->addr1, da);
-	ether_addr_copy(pwlanhdr->addr2, myid(&padapter->eeprompriv));
+	memcpy(pwlanhdr->addr2, myid(&padapter->eeprompriv), ETH_ALEN);
 	ether_addr_copy(pwlanhdr->addr3, get_my_bssid23a(&pmlmeinfo->network));
 
 	pwlanhdr->seq_ctrl =
@@ -3633,7 +3633,7 @@ static int _issue_deauth(struct rtw_adapter *padapter, unsigned char *da,
 		cpu_to_le16(IEEE80211_FTYPE_MGMT | IEEE80211_STYPE_DEAUTH);
 
 	ether_addr_copy(mgmt->da, da);
-	ether_addr_copy(mgmt->sa, myid(&padapter->eeprompriv));
+	memcpy(mgmt->sa, myid(&padapter->eeprompriv), ETH_ALEN);
 	ether_addr_copy(mgmt->bssid, get_my_bssid23a(&pmlmeinfo->network));
 
 	mgmt->seq_ctrl = cpu_to_le16(IEEE80211_SN_TO_SEQ(pmlmeext->mgnt_seq));
@@ -3737,7 +3737,7 @@ void issue_action_spct_ch_switch23a(struct rtw_adapter *padapter,
 		cpu_to_le16(IEEE80211_FTYPE_MGMT | IEEE80211_STYPE_ACTION);
 
 	ether_addr_copy(mgmt->da, ra); /* RA */
-	ether_addr_copy(mgmt->sa, myid(&padapter->eeprompriv)); /* TA */
+	memcpy(mgmt->sa, myid(&padapter->eeprompriv), ETH_ALEN); /* TA */
 	ether_addr_copy(mgmt->bssid, ra); /* DA = RA */
 
 	mgmt->seq_ctrl = cpu_to_le16(IEEE80211_SN_TO_SEQ(pmlmeext->mgnt_seq));
@@ -3799,7 +3799,7 @@ void issue_action_BA23a(struct rtw_adapter *padapter,
 		cpu_to_le16(IEEE80211_FTYPE_MGMT | IEEE80211_STYPE_ACTION);
 
 	ether_addr_copy(mgmt->da, raddr);
-	ether_addr_copy(mgmt->sa, myid(&padapter->eeprompriv));
+	memcpy(mgmt->sa, myid(&padapter->eeprompriv), ETH_ALEN);
 	ether_addr_copy(mgmt->bssid, get_my_bssid23a(&pmlmeinfo->network));
 
 	mgmt->seq_ctrl = cpu_to_le16(IEEE80211_SN_TO_SEQ(pmlmeext->mgnt_seq));
diff --git a/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c b/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
index 271c33d..976ec2c 100644
--- a/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
+++ b/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
@@ -219,7 +219,7 @@ ConstructBeacon(struct rtw_adapter *padapter, u8 *pframe, u32 *pLength)
 		cpu_to_le16(IEEE80211_FTYPE_MGMT | IEEE80211_STYPE_BEACON);
 
 	ether_addr_copy(mgmt->da, bc_addr);
-	ether_addr_copy(mgmt->sa, myid(&padapter->eeprompriv));
+	memcpy(mgmt->sa, myid(&padapter->eeprompriv), ETH_ALEN);
 	ether_addr_copy(mgmt->bssid, get_my_bssid23a(cur_network));
 
 	/* A Beacon frame shouldn't have fragment bits set */
diff --git a/drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c b/drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c
index bd6953a..cb81e64 100644
--- a/drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c
+++ b/drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c
@@ -2414,7 +2414,7 @@ void rtw_cfg80211_indicate_sta_disassoc(struct rtw_adapter *padapter,
 	mgmt.frame_control =
 		cpu_to_le16(IEEE80211_FTYPE_MGMT | IEEE80211_STYPE_DEAUTH);
 
-	ether_addr_copy(mgmt.da, myid(&padapter->eeprompriv));
+	memcpy(mgmt.da, myid(&padapter->eeprompriv), ETH_ALEN);
 	ether_addr_copy(mgmt.sa, da);
 	ether_addr_copy(mgmt.bssid, get_my_bssid23a(&pmlmeinfo->network));
 

Re: [patch] staging: rtl8723au: incorrect use of ether_addr_copy()

[Joe Perches]

On Wed, 2014-10-08 at 13:40 +0300, Dan Carpenter wrote:
> The return from myid() isn't aligned correctly for ether_addr_copy().

Hey Dan.

Actual evidence showing ether_addr_copy conversions
may not always be wise.

How did you find them?

Is there a new alignment capability in smatch?

Re: [patch] staging: rtl8723au: incorrect use of ether_addr_copy()

[Dan Carpenter]

On Wed, Oct 08, 2014 at 05:26:11AM -0700, Joe Perches wrote:
> On Wed, 2014-10-08 at 13:40 +0300, Dan Carpenter wrote:
> > The return from myid() isn't aligned correctly for ether_addr_copy().
> 
> Hey Dan.
> 
> Actual evidence showing ether_addr_copy conversions
> may not always be wise.
> 
> How did you find them?

I was just trying to see how common these kinds of bugs are.  It didn't
take long to find, but my impression is that they are rare and I got
lucky.  These kinds of bugs are tricky to find and we don't have any
tools for it.

regards,
dan carpenter

Re: [patch] staging: rtl8723au: incorrect use of ether_addr_copy()

[Julia Lawall]

On Wed, 8 Oct 2014, Dan Carpenter wrote:

> On Wed, Oct 08, 2014 at 05:26:11AM -0700, Joe Perches wrote:
> > On Wed, 2014-10-08 at 13:40 +0300, Dan Carpenter wrote:
> > > The return from myid() isn't aligned correctly for ether_addr_copy().
> >
> > Hey Dan.
> >
> > Actual evidence showing ether_addr_copy conversions
> > may not always be wise.
> >
> > How did you find them?
>
> I was just trying to see how common these kinds of bugs are.  It didn't
> take long to find, but my impression is that they are rare and I got
> lucky.  These kinds of bugs are tricky to find and we don't have any
> tools for it.

Couldn't you just use your favorite matching tool, collect the file names,
compile them, run pahole, and process the output in some way?  It doesn't
give a complete analysis (you don't find all problems), but if you find a
problem it is a real problem.

julia

Re: [patch] staging: rtl8723au: incorrect use of ether_addr_copy()

[Dan Carpenter]

On Wed, Oct 08, 2014 at 02:50:50PM +0200, Julia Lawall wrote:
> Couldn't you just use your favorite matching tool, collect the file names,
> compile them, run pahole, and process the output in some way?  It doesn't
> give a complete analysis (you don't find all problems), but if you find a
> problem it is a real problem.

Well...  You would be better off using Smatch than pahole.  And
obviously I did try something like this, but it's fairly tricky.

regards,
dan carpenter

Re: [patch] staging: rtl8723au: incorrect use of ether_addr_copy()

[Jes Sorensen]

Dan Carpenter <dan.carpenter@oracle.com> writes:
> The return from myid() isn't aligned correctly for ether_addr_copy().
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Sorry, this makes no sense, just fix it properly!

drivers/staging/rtl8723au/include/rtw_eeprom.h:

struct eeprom_priv {
        u8              bautoload_fail_flag;
        u8              bloadfile_fail_flag;
        u8              bloadmac_fail_flag;
        /* u8           bempty; */
        /* u8           sys_config; */
        u8              mac_addr[6];    /* PermanentAddress */
        /* u8           config0; */

Move mac_addr[6] to the top of the struct and be done with it.

NACK

Jes

> diff --git a/drivers/staging/rtl8723au/core/rtw_mlme_ext.c b/drivers/staging/rtl8723au/core/rtw_mlme_ext.c
> index 3eb77de..c8f7890 100644
> --- a/drivers/staging/rtl8723au/core/rtw_mlme_ext.c
> +++ b/drivers/staging/rtl8723au/core/rtw_mlme_ext.c
> @@ -2402,7 +2402,7 @@ void issue_beacon23a(struct rtw_adapter *padapter, int timeout_ms)
>  	mgmt->seq_ctrl = 0;
>  
>  	ether_addr_copy(mgmt->da, bc_addr);
> -	ether_addr_copy(mgmt->sa, myid(&padapter->eeprompriv));
> +	memcpy(mgmt->sa, myid(&padapter->eeprompriv), ETH_ALEN);
>  	ether_addr_copy(mgmt->bssid, get_my_bssid23a(cur_network));
>  
>  	/* timestamp will be inserted by hardware */
> @@ -2556,7 +2556,7 @@ static void issue_probersp(struct rtw_adapter *padapter, unsigned char *da,
>  		cpu_to_le16(IEEE80211_FTYPE_MGMT | IEEE80211_STYPE_PROBE_RESP);
>  
>  	ether_addr_copy(mgmt->da, da);
> -	ether_addr_copy(mgmt->sa, mac);
> +	memcpy(mgmt->sa, mac, ETH_ALEN);
>  	ether_addr_copy(mgmt->bssid, bssid);
>  
>  	mgmt->seq_ctrl = cpu_to_le16(IEEE80211_SN_TO_SEQ(pmlmeext->mgnt_seq));
> @@ -2718,7 +2718,7 @@ static int _issue_probereq(struct rtw_adapter *padapter,
>  		ether_addr_copy(pwlanhdr->addr3, bc_addr);
>  	}
>  
> -	ether_addr_copy(pwlanhdr->addr2, mac);
> +	memcpy(pwlanhdr->addr2, mac, ETH_ALEN);
>  
>  	pwlanhdr->seq_ctrl =
>  		cpu_to_le16(IEEE80211_SN_TO_SEQ(pmlmeext->mgnt_seq));
> @@ -2863,8 +2863,8 @@ static void issue_auth(struct rtw_adapter *padapter, struct sta_info *psta,
>  #ifdef CONFIG_8723AU_AP_MODE
>  		unsigned short val16;
>  		ether_addr_copy(mgmt->da, psta->hwaddr);
> -		ether_addr_copy(mgmt->sa, myid(&padapter->eeprompriv));
> -		ether_addr_copy(mgmt->bssid, myid(&padapter->eeprompriv));
> +		memcpy(mgmt->sa, myid(&padapter->eeprompriv), ETH_ALEN);
> +		memcpy(mgmt->bssid, myid(&padapter->eeprompriv), ETH_ALEN);
>  
>  		/*  setting auth algo number */
>  		val16 = (u16)psta->authalg;
> @@ -2895,7 +2895,7 @@ static void issue_auth(struct rtw_adapter *padapter, struct sta_info *psta,
>  		struct ieee80211_mgmt *iv_mgmt;
>  
>  		ether_addr_copy(mgmt->da, get_my_bssid23a(&pmlmeinfo->network));
> -		ether_addr_copy(mgmt->sa, myid(&padapter->eeprompriv));
> +		memcpy(mgmt->sa, myid(&padapter->eeprompriv), ETH_ALEN);
>  		ether_addr_copy(mgmt->bssid,
>  				get_my_bssid23a(&pmlmeinfo->network));
>  
> @@ -3006,7 +3006,7 @@ static void issue_assocrsp(struct rtw_adapter *padapter, unsigned short status,
>  	mgmt->frame_control = cpu_to_le16(IEEE80211_FTYPE_MGMT | pkt_type);
>  
>  	ether_addr_copy(mgmt->da, pstat->hwaddr);
> -	ether_addr_copy(mgmt->sa, myid(&padapter->eeprompriv));
> +	memcpy(mgmt->sa, myid(&padapter->eeprompriv), ETH_ALEN);
>  	ether_addr_copy(mgmt->bssid, get_my_bssid23a(&pmlmeinfo->network));
>  
>  	mgmt->seq_ctrl = cpu_to_le16(IEEE80211_SN_TO_SEQ(pmlmeext->mgnt_seq));
> @@ -3129,7 +3129,7 @@ static void issue_assocreq(struct rtw_adapter *padapter)
>  		cpu_to_le16(IEEE80211_FTYPE_MGMT | IEEE80211_STYPE_ASSOC_REQ);
>  
>  	ether_addr_copy(mgmt->da, get_my_bssid23a(&pmlmeinfo->network));
> -	ether_addr_copy(mgmt->sa, myid(&padapter->eeprompriv));
> +	memcpy(mgmt->sa, myid(&padapter->eeprompriv), ETH_ALEN);
>  	ether_addr_copy(mgmt->bssid, get_my_bssid23a(&pmlmeinfo->network));
>  
>  	mgmt->seq_ctrl = cpu_to_le16(IEEE80211_SN_TO_SEQ(pmlmeext->mgnt_seq));
> @@ -3400,7 +3400,7 @@ static int _issue_nulldata23a(struct rtw_adapter *padapter, unsigned char *da,
>  		pwlanhdr->frame_control |= cpu_to_le16(IEEE80211_FCTL_PM);
>  
>  	ether_addr_copy(pwlanhdr->addr1, da);
> -	ether_addr_copy(pwlanhdr->addr2, myid(&padapter->eeprompriv));
> +	memcpy(pwlanhdr->addr2, myid(&padapter->eeprompriv), ETH_ALEN);
>  	ether_addr_copy(pwlanhdr->addr3, get_my_bssid23a(&pmlmeinfo->network));
>  
>  	pwlanhdr->seq_ctrl =
> @@ -3528,7 +3528,7 @@ static int _issue_qos_nulldata23a(struct rtw_adapter *padapter,
>  		pwlanhdr->qos_ctrl |= cpu_to_le16(IEEE80211_QOS_CTL_EOSP);
>  
>  	ether_addr_copy(pwlanhdr->addr1, da);
> -	ether_addr_copy(pwlanhdr->addr2, myid(&padapter->eeprompriv));
> +	memcpy(pwlanhdr->addr2, myid(&padapter->eeprompriv), ETH_ALEN);
>  	ether_addr_copy(pwlanhdr->addr3, get_my_bssid23a(&pmlmeinfo->network));
>  
>  	pwlanhdr->seq_ctrl =
> @@ -3633,7 +3633,7 @@ static int _issue_deauth(struct rtw_adapter *padapter, unsigned char *da,
>  		cpu_to_le16(IEEE80211_FTYPE_MGMT | IEEE80211_STYPE_DEAUTH);
>  
>  	ether_addr_copy(mgmt->da, da);
> -	ether_addr_copy(mgmt->sa, myid(&padapter->eeprompriv));
> +	memcpy(mgmt->sa, myid(&padapter->eeprompriv), ETH_ALEN);
>  	ether_addr_copy(mgmt->bssid, get_my_bssid23a(&pmlmeinfo->network));
>  
>  	mgmt->seq_ctrl = cpu_to_le16(IEEE80211_SN_TO_SEQ(pmlmeext->mgnt_seq));
> @@ -3737,7 +3737,7 @@ void issue_action_spct_ch_switch23a(struct rtw_adapter *padapter,
>  		cpu_to_le16(IEEE80211_FTYPE_MGMT | IEEE80211_STYPE_ACTION);
>  
>  	ether_addr_copy(mgmt->da, ra); /* RA */
> -	ether_addr_copy(mgmt->sa, myid(&padapter->eeprompriv)); /* TA */
> +	memcpy(mgmt->sa, myid(&padapter->eeprompriv), ETH_ALEN); /* TA */
>  	ether_addr_copy(mgmt->bssid, ra); /* DA = RA */
>  
>  	mgmt->seq_ctrl = cpu_to_le16(IEEE80211_SN_TO_SEQ(pmlmeext->mgnt_seq));
> @@ -3799,7 +3799,7 @@ void issue_action_BA23a(struct rtw_adapter *padapter,
>  		cpu_to_le16(IEEE80211_FTYPE_MGMT | IEEE80211_STYPE_ACTION);
>  
>  	ether_addr_copy(mgmt->da, raddr);
> -	ether_addr_copy(mgmt->sa, myid(&padapter->eeprompriv));
> +	memcpy(mgmt->sa, myid(&padapter->eeprompriv), ETH_ALEN);
>  	ether_addr_copy(mgmt->bssid, get_my_bssid23a(&pmlmeinfo->network));
>  
>  	mgmt->seq_ctrl = cpu_to_le16(IEEE80211_SN_TO_SEQ(pmlmeext->mgnt_seq));
> diff --git a/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c b/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
> index 271c33d..976ec2c 100644
> --- a/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
> +++ b/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
> @@ -219,7 +219,7 @@ ConstructBeacon(struct rtw_adapter *padapter, u8 *pframe, u32 *pLength)
>  		cpu_to_le16(IEEE80211_FTYPE_MGMT | IEEE80211_STYPE_BEACON);
>  
>  	ether_addr_copy(mgmt->da, bc_addr);
> -	ether_addr_copy(mgmt->sa, myid(&padapter->eeprompriv));
> +	memcpy(mgmt->sa, myid(&padapter->eeprompriv), ETH_ALEN);
>  	ether_addr_copy(mgmt->bssid, get_my_bssid23a(cur_network));
>  
>  	/* A Beacon frame shouldn't have fragment bits set */
> diff --git a/drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c b/drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c
> index bd6953a..cb81e64 100644
> --- a/drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c
> +++ b/drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c
> @@ -2414,7 +2414,7 @@ void rtw_cfg80211_indicate_sta_disassoc(struct rtw_adapter *padapter,
>  	mgmt.frame_control =
>  		cpu_to_le16(IEEE80211_FTYPE_MGMT | IEEE80211_STYPE_DEAUTH);
>  
> -	ether_addr_copy(mgmt.da, myid(&padapter->eeprompriv));
> +	memcpy(mgmt.da, myid(&padapter->eeprompriv), ETH_ALEN);
>  	ether_addr_copy(mgmt.sa, da);
>  	ether_addr_copy(mgmt.bssid, get_my_bssid23a(&pmlmeinfo->network));
>  

Re: [patch] staging: rtl8723au: incorrect use of ether_addr_copy()

[Joe Perches]

On Wed, 2014-10-08 at 15:46 +0300, Dan Carpenter wrote:
> On Wed, Oct 08, 2014 at 05:26:11AM -0700, Joe Perches wrote:
> > On Wed, 2014-10-08 at 13:40 +0300, Dan Carpenter wrote:
> > > The return from myid() isn't aligned correctly for ether_addr_copy().
> > 
> > Hey Dan.
> > 
> > Actual evidence showing ether_addr_copy conversions
> > may not always be wise.
> > 
> > How did you find them?
> 
> I was just trying to see how common these kinds of bugs are.  It didn't
> take long to find, but my impression is that they are rare and I got
> lucky.  These kinds of bugs are tricky to find and we don't have any
> tools for it.

As far as I know, that's true too.

Jes, was the mac_addr field in this struct
ever __aligned(2)?

struct eeprom_priv {
       u8              bautoload_fail_flag;
       u8              bloadfile_fail_flag;
       u8              bloadmac_fail_flag;
       /* u8           bempty; */
       /* u8           sys_config; */
       u8              mac_addr[6];    /* PermanentAddress */
...
}

As far as I can tell from git history, it was
that way at the first check-in.

Dan, did you also look for any other alignment
defects in uses of any is_<foo>_ether_addr calls?

Re: [patch] staging: rtl8723au: incorrect use of ether_addr_copy()

[Dan Carpenter]

On Wed, Oct 08, 2014 at 03:59:33PM +0200, Jes Sorensen wrote:
> Dan Carpenter <dan.carpenter@oracle.com> writes:
> > The return from myid() isn't aligned correctly for ether_addr_copy().
> >
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> Sorry, this makes no sense, just fix it properly!
> 
> drivers/staging/rtl8723au/include/rtw_eeprom.h:
> 
> struct eeprom_priv {
>         u8              bautoload_fail_flag;
>         u8              bloadfile_fail_flag;
>         u8              bloadmac_fail_flag;
>         /* u8           bempty; */
>         /* u8           sys_config; */
>         u8              mac_addr[6];    /* PermanentAddress */
>         /* u8           config0; */
> 
> Move mac_addr[6] to the top of the struct and be done with it.
> 
> NACK

Oops.  I thought it was something from the hardware.  Actually can you
fix it and give me a reported-by tag?

regards,
dan carpenter

Re: [patch] staging: rtl8723au: incorrect use of ether_addr_copy()

[Jes Sorensen]

Dan Carpenter <dan.carpenter@oracle.com> writes:
> On Wed, Oct 08, 2014 at 03:59:33PM +0200, Jes Sorensen wrote:
>> Dan Carpenter <dan.carpenter@oracle.com> writes:
>> > The return from myid() isn't aligned correctly for ether_addr_copy().
>> >
>> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>> 
>> Sorry, this makes no sense, just fix it properly!
>> 
>> drivers/staging/rtl8723au/include/rtw_eeprom.h:
>> 
>> struct eeprom_priv {
>>         u8              bautoload_fail_flag;
>>         u8              bloadfile_fail_flag;
>>         u8              bloadmac_fail_flag;
>>         /* u8           bempty; */
>>         /* u8           sys_config; */
>>         u8              mac_addr[6];    /* PermanentAddress */
>>         /* u8           config0; */
>> 
>> Move mac_addr[6] to the top of the struct and be done with it.
>> 
>> NACK
>
> Oops.  I thought it was something from the hardware.  Actually can you
> fix it and give me a reported-by tag?

That stuff is just copied into memory from the eeprom, so we can pretty
much do with it as we like.

I'll put it on my list.

Jes

Re: [patch] staging: rtl8723au: incorrect use of ether_addr_copy()

[Jes Sorensen]

Joe Perches <joe@perches.com> writes:
> On Wed, 2014-10-08 at 15:46 +0300, Dan Carpenter wrote:
>> On Wed, Oct 08, 2014 at 05:26:11AM -0700, Joe Perches wrote:
>> > On Wed, 2014-10-08 at 13:40 +0300, Dan Carpenter wrote:
>> > > The return from myid() isn't aligned correctly for ether_addr_copy().
>> > 
>> > Hey Dan.
>> > 
>> > Actual evidence showing ether_addr_copy conversions
>> > may not always be wise.
>> > 
>> > How did you find them?
>> 
>> I was just trying to see how common these kinds of bugs are.  It didn't
>> take long to find, but my impression is that they are rare and I got
>> lucky.  These kinds of bugs are tricky to find and we don't have any
>> tools for it.
>
> As far as I know, that's true too.
>
> Jes, was the mac_addr field in this struct
> ever __aligned(2)?
>
> struct eeprom_priv {
>        u8              bautoload_fail_flag;
>        u8              bloadfile_fail_flag;
>        u8              bloadmac_fail_flag;
>        /* u8           bempty; */
>        /* u8           sys_config; */
>        u8              mac_addr[6];    /* PermanentAddress */
> ...
> }
>
> As far as I can tell from git history, it was
> that way at the first check-in.

I may have removed other entries that were unused, and that caused it to
become mis-aligned. I can't say for sure - the fix is straight forward
though.

Jes

Re: [patch] staging: rtl8723au: incorrect use of ether_addr_copy()

[Joe Perches]

On Wed, 2014-10-08 at 16:33 +0200, Jes Sorensen wrote:
> Joe Perches <joe@perches.com> writes:
> > On Wed, 2014-10-08 at 15:46 +0300, Dan Carpenter wrote:
> >> On Wed, Oct 08, 2014 at 05:26:11AM -0700, Joe Perches wrote:
> >> > On Wed, 2014-10-08 at 13:40 +0300, Dan Carpenter wrote:
> >> > > The return from myid() isn't aligned correctly for ether_addr_copy().
> >> > 
> >> > Hey Dan.
> >> > 
> >> > Actual evidence showing ether_addr_copy conversions
> >> > may not always be wise.
> >> > 
> >> > How did you find them?
> >> 
> >> I was just trying to see how common these kinds of bugs are.  It didn't
> >> take long to find, but my impression is that they are rare and I got
> >> lucky.  These kinds of bugs are tricky to find and we don't have any
> >> tools for it.
> >
> > As far as I know, that's true too.
> >
> > Jes, was the mac_addr field in this struct
> > ever __aligned(2)?
> >
> > struct eeprom_priv {
> >        u8              bautoload_fail_flag;
> >        u8              bloadfile_fail_flag;
> >        u8              bloadmac_fail_flag;
> >        /* u8           bempty; */
> >        /* u8           sys_config; */
> >        u8              mac_addr[6];    /* PermanentAddress */
> > ...
> > }
> >
> > As far as I can tell from git history, it was
> > that way at the first check-in.
> 
> I may have removed other entries that were unused, and that caused it to
> become mis-aligned. I can't say for sure - the fix is straight forward
> though.

One option is to add __aligned(2) to the mac_addr field
and make no other change.

Re: [patch] staging: rtl8723au: incorrect use of ether_addr_copy()

[Jes Sorensen]

Joe Perches <joe@perches.com> writes:
> On Wed, 2014-10-08 at 16:33 +0200, Jes Sorensen wrote:
>> Joe Perches <joe@perches.com> writes:
>> > On Wed, 2014-10-08 at 15:46 +0300, Dan Carpenter wrote:
>> >> On Wed, Oct 08, 2014 at 05:26:11AM -0700, Joe Perches wrote:
>> >> > On Wed, 2014-10-08 at 13:40 +0300, Dan Carpenter wrote:
>> >> > > The return from myid() isn't aligned correctly for ether_addr_copy().
>> >> > 
>> >> > Hey Dan.
>> >> > 
>> >> > Actual evidence showing ether_addr_copy conversions
>> >> > may not always be wise.
>> >> > 
>> >> > How did you find them?
>> >> 
>> >> I was just trying to see how common these kinds of bugs are.  It didn't
>> >> take long to find, but my impression is that they are rare and I got
>> >> lucky.  These kinds of bugs are tricky to find and we don't have any
>> >> tools for it.
>> >
>> > As far as I know, that's true too.
>> >
>> > Jes, was the mac_addr field in this struct
>> > ever __aligned(2)?
>> >
>> > struct eeprom_priv {
>> >        u8              bautoload_fail_flag;
>> >        u8              bloadfile_fail_flag;
>> >        u8              bloadmac_fail_flag;
>> >        /* u8           bempty; */
>> >        /* u8           sys_config; */
>> >        u8              mac_addr[6];    /* PermanentAddress */
>> > ...
>> > }
>> >
>> > As far as I can tell from git history, it was
>> > that way at the first check-in.
>> 
>> I may have removed other entries that were unused, and that caused it to
>> become mis-aligned. I can't say for sure - the fix is straight forward
>> though.
>
> One option is to add __aligned(2) to the mac_addr field
> and make no other change.

As I said in another mail, just move it to the front of the struct and
be done with it. No point in wasting alignment bytes if we don't have
to.

Jes