Re: [PATCH 3/4] brcmsmac: Fix invalid memcpy() size in brcms_c_d11hdrs_mac80211

[Florian Fainelli <f.fainelli@gmail.com>]

On 07/19/2016 03:38 AM, Arend Van Spriel wrote:
> On 19-7-2016 1:24, Florian Fainelli wrote:
>> struct ieee80211_rts::ra is only ETH_ALEN wide, yet we attempt to copy 2
>> * ETH_ALEN, which will potentially overrun the destination buffer.
> 
> NACK - this is intentional. Have to admit it is a bit of trickery.

This seems fragile, if there is any kind of re-ordering of fields within
that structure your trick breaks apart.

> struct ieee80211_rts is mapped over struct d11txh which is sent to
> hardware. The struct is used for both RTS and CTS. Transmitting CTS will
> only fill 802.11 addr2 in struct ieee80211_rts::ra. Transmitting RTS
> fills 802.11 addr1 in ra and 802.11 addr2 in ta using single memcpy().
> Not very clear, but your change is not the way to go here.

Fair enough, as Kalle suggested, this deserves a comment explaining why,
I would not be surprised other people would trip over that.

> 
> Regards,
> Arend
> 
>> Reported-by: coverity (CID 145657)
>> Fixes: 5b435de0d7868 ("net: wireless: add brcm80211 drivers")
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c
>> index c2a938b59044..59813a3666eb 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c
>> @@ -6671,7 +6671,7 @@ brcms_c_d11hdrs_mac80211(struct brcms_c_info *wlc, struct ieee80211_hw *hw,
>>  			rts->frame_control = cpu_to_le16(IEEE80211_FTYPE_CTL |
>>  							 IEEE80211_STYPE_RTS);
>>  
>> -			memcpy(&rts->ra, &h->addr1, 2 * ETH_ALEN);
>> +			memcpy(&rts->ra, &h->addr1, ETH_ALEN);
>>  		}
>>  
>>  		/* mainrate
>>


-- 
Florian