| Message ID | 1468884277-18606-4-git-send-email-f.fainelli@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
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. 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. 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Arend Van Spriel <arend.vanspriel@broadcom.com> writes: > 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. > 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. Maybe add a comment explaining that?
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 >>
On 19-7-2016 18:42, Florian Fainelli wrote: > 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. Well, that would be an 802.11 standard update and given that it would break legacy devices I think it is unlikely that reordering would happen in this case. >> 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. Agree. I am also fine with two separate memcpy() statements if that gives enough clarification. Regards, Arend >> >> 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 >>> > > -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
struct ieee80211_rts::ra is only ETH_ALEN wide, yet we attempt to copy 2 * ETH_ALEN, which will potentially overrun the destination buffer. 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(-)