[PATCH 0/4] brcm80211: Misc coverity fixes

[PATCH 0/4] brcm80211: Misc coverity fixes

[Florian Fainelli]

Hi,

This patch series addresses several coverity issues, they all seemed relevant
to me.

There is also a ton of warnings in Coverity caused by brcmf_fil_iovar_int_get()
and friends because of the initial access:

__le32 data_le = cpu_to_le32(*data) which can utilize unitialized memory. I am
not sure if we actually care about any kind of initial, value, but if we don't,
then the fix should be fairly obvious.

Thanks!


Florian Fainelli (4):
  brcmfmac: Fix glob_skb leak in brcmf_sdiod_recv_chain
  brcmsmac: Free packet if dma_mapping_error() fails in dma_rxfill
  brcmsmac: Fix invalid memcpy() size in brcms_c_d11hdrs_mac80211
  brcmsmac: Initialize power in brcms_c_stf_ss_algo_channel_get()

 drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 4 +++-
 drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c    | 4 +++-
 drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c   | 2 +-
 drivers/net/wireless/broadcom/brcm80211/brcmsmac/stf.c    | 2 +-
 4 files changed, 8 insertions(+), 4 deletions(-)

-- 
2.7.4

[PATCH 1/4] brcmfmac: Fix glob_skb leak in brcmf_sdiod_recv_chain

[Florian Fainelli]

In case brcmf_sdiod_recv_chain() cannot complete a succeful call to
brcmf_sdiod_buffrw, we would be leaking glom_skb and not free it as we
should, fix this.

Reported-by: coverity (CID 1164856)
Fixes: a413e39a38573 ("brcmfmac: fix brcmf_sdcard_recv_chain() for host without sg support")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
index c4b89d27e2e8..f549c25608d6 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
@@ -726,8 +726,10 @@ int brcmf_sdiod_recv_chain(struct brcmf_sdio_dev *sdiodev,
 			return -ENOMEM;
 		err = brcmf_sdiod_buffrw(sdiodev, SDIO_FUNC_2, false, addr,
 					 glom_skb);
-		if (err)
+		if (err) {
+			brcmu_pkt_buf_free_skb(glom_skb);
 			goto done;
+		}
 
 		skb_queue_walk(pktq, skb) {
 			memcpy(skb->data, glom_skb->data, skb->len);
-- 
2.7.4

[PATCH 2/4] brcmsmac: Free packet if dma_mapping_error() fails in dma_rxfill

[Florian Fainelli]

In case dma_mapping_error() returns an error in dma_rxfill, we would be
leaking a packet that we allocated with brcmu_pkt_buf_get_skb().

Reported-by: coverity (CID 1081819)
Fixes: 67d0cf50bd32 ("brcmsmac: Fix WARNING caused by lack of calls to dma_mapping_error()")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c
index 796f5f9d5d5a..b7df576bb84d 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c
@@ -1079,8 +1079,10 @@ bool dma_rxfill(struct dma_pub *pub)
 
 		pa = dma_map_single(di->dmadev, p->data, di->rxbufsize,
 				    DMA_FROM_DEVICE);
-		if (dma_mapping_error(di->dmadev, pa))
+		if (dma_mapping_error(di->dmadev, pa)) {
+			brcmu_pkt_buf_free_skb(p);
 			return false;
+		}
 
 		/* save the free packet pointer */
 		di->rxp[rxout] = p;
-- 
2.7.4

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

[Florian Fainelli]

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(-)

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
-- 
2.7.4

[PATCH 4/4] brcmsmac: Initialize power in brcms_c_stf_ss_algo_channel_get()

[Florian Fainelli]

wlc_phy_txpower_get_current() does a logical OR of power->flags, which
presumes that power.flags was initiliazed earlier by the caller,
unfortunately, this is not the case, so make sure we zero out the struct
tx_power before calling into wlc_phy_txpower_get_current().

Reported-by: coverity (CID 146011)
Fixes: 5b435de0d7868 ("net: wireless: add brcm80211 drivers")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmsmac/stf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/stf.c b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/stf.c
index dd9162722495..0ab865de1491 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/stf.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/stf.c
@@ -87,7 +87,7 @@ void
 brcms_c_stf_ss_algo_channel_get(struct brcms_c_info *wlc, u16 *ss_algo_channel,
 			    u16 chanspec)
 {
-	struct tx_power power;
+	struct tx_power power = { };
 	u8 siso_mcs_id, cdd_mcs_id, stbc_mcs_id;
 
 	/* Clear previous settings */
-- 
2.7.4

Re: [PATCH 0/4] brcm80211: Misc coverity fixes

[Arend Van Spriel]

+ Bob

On 19-7-2016 1:24, Florian Fainelli wrote:
> Hi,
> 
> This patch series addresses several coverity issues, they all seemed relevant
> to me.

Hi Florian,

Been a while so nice to see coverity fixes popping up. Actually
something that I have on my todo list to add our brcm80211 to coverity
within Broadcom. So being curious as to whether this comes from a public
coverity server like scan.coverity.com. Maybe bit redundant to setup
internally if there is a good coverity analysis publicly available.

> There is also a ton of warnings in Coverity caused by brcmf_fil_iovar_int_get()
> and friends because of the initial access:
> 
> __le32 data_le = cpu_to_le32(*data) which can utilize unitialized memory. I am
> not sure if we actually care about any kind of initial, value, but if we don't,
> then the fix should be fairly obvious.

If we are talking only about "get" variant than we mostly don't care.
Some getters support filter variables to be passed towards firmware. I
have not looked at the analysis to give any judgement here.

Regards,
Arend

> Thanks!
> 
> 
> Florian Fainelli (4):
>   brcmfmac: Fix glob_skb leak in brcmf_sdiod_recv_chain
>   brcmsmac: Free packet if dma_mapping_error() fails in dma_rxfill
>   brcmsmac: Fix invalid memcpy() size in brcms_c_d11hdrs_mac80211
>   brcmsmac: Initialize power in brcms_c_stf_ss_algo_channel_get()
> 
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 4 +++-
>  drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c    | 4 +++-
>  drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c   | 2 +-
>  drivers/net/wireless/broadcom/brcm80211/brcmsmac/stf.c    | 2 +-
>  4 files changed, 8 insertions(+), 4 deletions(-)
> 

Re: [PATCH 2/4] brcmsmac: Free packet if dma_mapping_error() fails in dma_rxfill

[Arend Van Spriel]

On 19-7-2016 1:24, Florian Fainelli wrote:
> In case dma_mapping_error() returns an error in dma_rxfill, we would be
> leaking a packet that we allocated with brcmu_pkt_buf_get_skb().
> 
> Reported-by: coverity (CID 1081819)
> Fixes: 67d0cf50bd32 ("brcmsmac: Fix WARNING caused by lack of calls to dma_mapping_error()")

Acked-by: Arend van Spriel <arend@broadcom.com>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c
> index 796f5f9d5d5a..b7df576bb84d 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c
> @@ -1079,8 +1079,10 @@ bool dma_rxfill(struct dma_pub *pub)
>  
>  		pa = dma_map_single(di->dmadev, p->data, di->rxbufsize,
>  				    DMA_FROM_DEVICE);
> -		if (dma_mapping_error(di->dmadev, pa))
> +		if (dma_mapping_error(di->dmadev, pa)) {
> +			brcmu_pkt_buf_free_skb(p);
>  			return false;
> +		}
>  
>  		/* save the free packet pointer */
>  		di->rxp[rxout] = p;
> 

Re: [PATCH 1/4] brcmfmac: Fix glob_skb leak in brcmf_sdiod_recv_chain

[Arend Van Spriel]

On 19-7-2016 1:24, Florian Fainelli wrote:
> In case brcmf_sdiod_recv_chain() cannot complete a succeful call to
> brcmf_sdiod_buffrw, we would be leaking glom_skb and not free it as we
> should, fix this.
> 
> Reported-by: coverity (CID 1164856)
> Fixes: a413e39a38573 ("brcmfmac: fix brcmf_sdcard_recv_chain() for host without sg support")

Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> index c4b89d27e2e8..f549c25608d6 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> @@ -726,8 +726,10 @@ int brcmf_sdiod_recv_chain(struct brcmf_sdio_dev *sdiodev,
>  			return -ENOMEM;
>  		err = brcmf_sdiod_buffrw(sdiodev, SDIO_FUNC_2, false, addr,
>  					 glom_skb);
> -		if (err)
> +		if (err) {
> +			brcmu_pkt_buf_free_skb(glom_skb);
>  			goto done;
> +		}
>  
>  		skb_queue_walk(pktq, skb) {
>  			memcpy(skb->data, glom_skb->data, skb->len);
> 

Re: [PATCH 4/4] brcmsmac: Initialize power in brcms_c_stf_ss_algo_channel_get()

[Arend Van Spriel]

On 19-7-2016 1:24, Florian Fainelli wrote:
> wlc_phy_txpower_get_current() does a logical OR of power->flags, which
> presumes that power.flags was initiliazed earlier by the caller,
> unfortunately, this is not the case, so make sure we zero out the struct
> tx_power before calling into wlc_phy_txpower_get_current().
> 
> Reported-by: coverity (CID 146011)
> Fixes: 5b435de0d7868 ("net: wireless: add brcm80211 drivers")

Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/net/wireless/broadcom/brcm80211/brcmsmac/stf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/stf.c b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/stf.c
> index dd9162722495..0ab865de1491 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/stf.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/stf.c
> @@ -87,7 +87,7 @@ void
>  brcms_c_stf_ss_algo_channel_get(struct brcms_c_info *wlc, u16 *ss_algo_channel,
>  			    u16 chanspec)
>  {
> -	struct tx_power power;
> +	struct tx_power power = { };
>  	u8 siso_mcs_id, cdd_mcs_id, stbc_mcs_id;
>  
>  	/* Clear previous settings */
> 

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

[Arend Van Spriel]

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
> 

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

[Kalle Valo]

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?

-- 
Kalle Valo

Re: [PATCH 0/4] brcm80211: Misc coverity fixes

[Florian Fainelli]

On 07/19/2016 02:20 AM, Arend Van Spriel wrote:
> + Bob
> 
> On 19-7-2016 1:24, Florian Fainelli wrote:
>> Hi,
>>
>> This patch series addresses several coverity issues, they all seemed relevant
>> to me.
> 
> Hi Florian,
> 
> Been a while so nice to see coverity fixes popping up. Actually
> something that I have on my todo list to add our brcm80211 to coverity
> within Broadcom. So being curious as to whether this comes from a public
> coverity server like scan.coverity.com. Maybe bit redundant to setup
> internally if there is a good coverity analysis publicly available.

This is coming from the public coverity instance, if you create an
account there I could transfer to you the other bugs that affect the
brcm80211 drivers (hint: there is a ton of of them because of
brcmf_fil_iovar_int_get and friends).

> 
>> There is also a ton of warnings in Coverity caused by brcmf_fil_iovar_int_get()
>> and friends because of the initial access:
>>
>> __le32 data_le = cpu_to_le32(*data) which can utilize unitialized memory. I am
>> not sure if we actually care about any kind of initial, value, but if we don't,
>> then the fix should be fairly obvious.
> 
> If we are talking only about "get" variant than we mostly don't care.
> Some getters support filter variables to be passed towards firmware. I
> have not looked at the analysis to give any judgement here.

Alright, do you have a good way to test a patch that would just zero
initialize the data variable in brcmf_fil_iovar_int_get()? If so, I will
submit one with the appropriate CID references.

Thanks!
-- 
Florian

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

[Florian Fainelli]

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

Re: [1/4] brcmfmac: Fix glob_skb leak in brcmf_sdiod_recv_chain

[Kalle Valo]

Florian Fainelli <f.fainelli@gmail.com> wrote:
> In case brcmf_sdiod_recv_chain() cannot complete a succeful call to
> brcmf_sdiod_buffrw, we would be leaking glom_skb and not free it as we
> should, fix this.
> 
> Reported-by: coverity (CID 1164856)
> Fixes: a413e39a38573 ("brcmfmac: fix brcmf_sdcard_recv_chain() for host without sg support")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>

Thanks, 3 patches applied to wireless-drivers-next.git:

3bdae810721b brcmfmac: Fix glob_skb leak in brcmf_sdiod_recv_chain
5c5fa1f464ac brcmsmac: Free packet if dma_mapping_error() fails in dma_rxfill
f823a2aa8f46 brcmsmac: Initialize power in brcms_c_stf_ss_algo_channel_get()

-- 
Sent by pwcli
https://patchwork.kernel.org/patch/9235673/

Re: [PATCH 0/4] brcm80211: Misc coverity fixes

[Arend Van Spriel]

On 19-7-2016 18:41, Florian Fainelli wrote:
> On 07/19/2016 02:20 AM, Arend Van Spriel wrote:
>> + Bob
>>
>> On 19-7-2016 1:24, Florian Fainelli wrote:
>>> Hi,
>>>
>>> This patch series addresses several coverity issues, they all seemed relevant
>>> to me.
>>
>> Hi Florian,
>>
>> Been a while so nice to see coverity fixes popping up. Actually
>> something that I have on my todo list to add our brcm80211 to coverity
>> within Broadcom. So being curious as to whether this comes from a public
>> coverity server like scan.coverity.com. Maybe bit redundant to setup
>> internally if there is a good coverity analysis publicly available.
> 
> This is coming from the public coverity instance, if you create an
> account there I could transfer to you the other bugs that affect the
> brcm80211 drivers (hint: there is a ton of of them because of
> brcmf_fil_iovar_int_get and friends).

I did create an account and noticed "Broadcom Linux Kernel Open Source
Repository" project. Anyways, let me have it :-p

>>
>>> There is also a ton of warnings in Coverity caused by brcmf_fil_iovar_int_get()
>>> and friends because of the initial access:
>>>
>>> __le32 data_le = cpu_to_le32(*data) which can utilize unitialized memory. I am
>>> not sure if we actually care about any kind of initial, value, but if we don't,
>>> then the fix should be fairly obvious.
>>
>> If we are talking only about "get" variant than we mostly don't care.
>> Some getters support filter variables to be passed towards firmware. I
>> have not looked at the analysis to give any judgement here.
> 
> Alright, do you have a good way to test a patch that would just zero
> initialize the data variable in brcmf_fil_iovar_int_get()? If so, I will
> submit one with the appropriate CID references.

But why would we care to zero the data when firmware simply overwrites
it. These control messages are not high-prio so we can burn some cycles
to initialize the variable, but to me this is a false positive.

Regards,
Arend

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

[Arend Van Spriel]

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
>>>
> 
> 

Re: [PATCH 0/4] brcm80211: Misc coverity fixes

[Florian Fainelli]

On 07/19/2016 11:21 AM, Arend Van Spriel wrote:
> On 19-7-2016 18:41, Florian Fainelli wrote:
>> On 07/19/2016 02:20 AM, Arend Van Spriel wrote:
>>> + Bob
>>>
>>> On 19-7-2016 1:24, Florian Fainelli wrote:
>>>> Hi,
>>>>
>>>> This patch series addresses several coverity issues, they all seemed relevant
>>>> to me.
>>>
>>> Hi Florian,
>>>
>>> Been a while so nice to see coverity fixes popping up. Actually
>>> something that I have on my todo list to add our brcm80211 to coverity
>>> within Broadcom. So being curious as to whether this comes from a public
>>> coverity server like scan.coverity.com. Maybe bit redundant to setup
>>> internally if there is a good coverity analysis publicly available.
>>
>> This is coming from the public coverity instance, if you create an
>> account there I could transfer to you the other bugs that affect the
>> brcm80211 drivers (hint: there is a ton of of them because of
>> brcmf_fil_iovar_int_get and friends).
> 
> I did create an account and noticed "Broadcom Linux Kernel Open Source
> Repository" project. Anyways, let me have it :-p
> 
>>>
>>>> There is also a ton of warnings in Coverity caused by brcmf_fil_iovar_int_get()
>>>> and friends because of the initial access:
>>>>
>>>> __le32 data_le = cpu_to_le32(*data) which can utilize unitialized memory. I am
>>>> not sure if we actually care about any kind of initial, value, but if we don't,
>>>> then the fix should be fairly obvious.
>>>
>>> If we are talking only about "get" variant than we mostly don't care.
>>> Some getters support filter variables to be passed towards firmware. I
>>> have not looked at the analysis to give any judgement here.
>>
>> Alright, do you have a good way to test a patch that would just zero
>> initialize the data variable in brcmf_fil_iovar_int_get()? If so, I will
>> submit one with the appropriate CID references.
> 
> But why would we care to zero the data when firmware simply overwrites
> it. These control messages are not high-prio so we can burn some cycles
> to initialize the variable, but to me this is a false positive.

We have something like this all over the place:

func() {
	u32 val;

	brcmf_fil_iovar_int_get(..., &val, sizeof(val));
}

brcmf_fil_iovar_int_get(.., const u32 *data, size_t len)
{
	__le32 data_le = cpu_to_le32(*data);
	...
}

So here data_le also has an uninitialized value, and later on, this:


        buflen = brcmf_create_iovar(name, data, len, drvr->proto_buf,
                                    sizeof(drvr->proto_buf));

}

And this function does this:

static u32
brcmf_create_iovar(char *name, const char *data, u32 datalen,
                   char *buf, u32 buflen)
{
        u32 len;

        len = strlen(name) + 1;

        if ((len + datalen) > buflen)
                return 0;

        memcpy(buf, name, len);

        /* append data onto the end of the name string */
        if (data && datalen)
                memcpy(&buf[len], data, datalen);

        return len + datalen;
}


We are essentially copying into buf an uninitialized value coming from
the stack, which seems like a problem to me.
-- 
Florian

Re: [PATCH 0/4] brcm80211: Misc coverity fixes

[Arend Van Spriel]

On 19-7-2016 20:30, Florian Fainelli wrote:
> On 07/19/2016 11:21 AM, Arend Van Spriel wrote:
>> On 19-7-2016 18:41, Florian Fainelli wrote:
>>> On 07/19/2016 02:20 AM, Arend Van Spriel wrote:
>>>> + Bob
>>>>
>>>> On 19-7-2016 1:24, Florian Fainelli wrote:
>>>>> Hi,
>>>>>
>>>>> This patch series addresses several coverity issues, they all seemed relevant
>>>>> to me.
>>>>
>>>> Hi Florian,
>>>>
>>>> Been a while so nice to see coverity fixes popping up. Actually
>>>> something that I have on my todo list to add our brcm80211 to coverity
>>>> within Broadcom. So being curious as to whether this comes from a public
>>>> coverity server like scan.coverity.com. Maybe bit redundant to setup
>>>> internally if there is a good coverity analysis publicly available.
>>>
>>> This is coming from the public coverity instance, if you create an
>>> account there I could transfer to you the other bugs that affect the
>>> brcm80211 drivers (hint: there is a ton of of them because of
>>> brcmf_fil_iovar_int_get and friends).
>>
>> I did create an account and noticed "Broadcom Linux Kernel Open Source
>> Repository" project. Anyways, let me have it :-p
>>
>>>>
>>>>> There is also a ton of warnings in Coverity caused by brcmf_fil_iovar_int_get()
>>>>> and friends because of the initial access:
>>>>>
>>>>> __le32 data_le = cpu_to_le32(*data) which can utilize unitialized memory. I am
>>>>> not sure if we actually care about any kind of initial, value, but if we don't,
>>>>> then the fix should be fairly obvious.
>>>>
>>>> If we are talking only about "get" variant than we mostly don't care.
>>>> Some getters support filter variables to be passed towards firmware. I
>>>> have not looked at the analysis to give any judgement here.
>>>
>>> Alright, do you have a good way to test a patch that would just zero
>>> initialize the data variable in brcmf_fil_iovar_int_get()? If so, I will
>>> submit one with the appropriate CID references.
>>
>> But why would we care to zero the data when firmware simply overwrites
>> it. These control messages are not high-prio so we can burn some cycles
>> to initialize the variable, but to me this is a false positive.
> 
> We have something like this all over the place:
> 
> func() {
> 	u32 val;
> 
> 	brcmf_fil_iovar_int_get(..., &val, sizeof(val));

If I am not mistaken the iovar_int_get only passes &val.

> }
> 
> brcmf_fil_iovar_int_get(.., const u32 *data, size_t len)
> {
> 	__le32 data_le = cpu_to_le32(*data);
> 	...
> }
> 
> So here data_le also has an uninitialized value, and later on, this:
> 
> 
>         buflen = brcmf_create_iovar(name, data, len, drvr->proto_buf,
>                                     sizeof(drvr->proto_buf));

create_iovar is used for both get and set...

> }
> 
> And this function does this:
> 
> static u32
> brcmf_create_iovar(char *name, const char *data, u32 datalen,
>                    char *buf, u32 buflen)
> {
>         u32 len;
> 
>         len = strlen(name) + 1;
> 
>         if ((len + datalen) > buflen)
>                 return 0;
> 
>         memcpy(buf, name, len);
> 
>         /* append data onto the end of the name string */
>         if (data && datalen)
>                 memcpy(&buf[len], data, datalen);

but it does not have that info so it does the memcpy. We could add an
extra parameter to avoid the memcpy for get, but as said there are
instances where get passes query data to firmware.

>         return len + datalen;
> }
> 
> 
> We are essentially copying into buf an uninitialized value coming from
> the stack, which seems like a problem to me.

Functionally I still don't see the problem. For the instances where this
is true, the buf will be overwritten with data from firmware. It may be
a security issue as stack content is passed to an external device.

Regards,
Arend

Re: [PATCH 0/4] brcm80211: Misc coverity fixes

[Florian Fainelli]

On 07/19/2016 12:36 PM, Arend Van Spriel wrote:
> On 19-7-2016 20:30, Florian Fainelli wrote:
>> On 07/19/2016 11:21 AM, Arend Van Spriel wrote:
>>> On 19-7-2016 18:41, Florian Fainelli wrote:
>>>> On 07/19/2016 02:20 AM, Arend Van Spriel wrote:
>>>>> + Bob
>>>>>
>>>>> On 19-7-2016 1:24, Florian Fainelli wrote:
>>>>>> Hi,
>>>>>>
>>>>>> This patch series addresses several coverity issues, they all seemed relevant
>>>>>> to me.
>>>>>
>>>>> Hi Florian,
>>>>>
>>>>> Been a while so nice to see coverity fixes popping up. Actually
>>>>> something that I have on my todo list to add our brcm80211 to coverity
>>>>> within Broadcom. So being curious as to whether this comes from a public
>>>>> coverity server like scan.coverity.com. Maybe bit redundant to setup
>>>>> internally if there is a good coverity analysis publicly available.
>>>>
>>>> This is coming from the public coverity instance, if you create an
>>>> account there I could transfer to you the other bugs that affect the
>>>> brcm80211 drivers (hint: there is a ton of of them because of
>>>> brcmf_fil_iovar_int_get and friends).
>>>
>>> I did create an account and noticed "Broadcom Linux Kernel Open Source
>>> Repository" project. Anyways, let me have it :-p
>>>
>>>>>
>>>>>> There is also a ton of warnings in Coverity caused by brcmf_fil_iovar_int_get()
>>>>>> and friends because of the initial access:
>>>>>>
>>>>>> __le32 data_le = cpu_to_le32(*data) which can utilize unitialized memory. I am
>>>>>> not sure if we actually care about any kind of initial, value, but if we don't,
>>>>>> then the fix should be fairly obvious.
>>>>>
>>>>> If we are talking only about "get" variant than we mostly don't care.
>>>>> Some getters support filter variables to be passed towards firmware. I
>>>>> have not looked at the analysis to give any judgement here.
>>>>
>>>> Alright, do you have a good way to test a patch that would just zero
>>>> initialize the data variable in brcmf_fil_iovar_int_get()? If so, I will
>>>> submit one with the appropriate CID references.
>>>
>>> But why would we care to zero the data when firmware simply overwrites
>>> it. These control messages are not high-prio so we can burn some cycles
>>> to initialize the variable, but to me this is a false positive.
>>
>> We have something like this all over the place:
>>
>> func() {
>> 	u32 val;
>>
>> 	brcmf_fil_iovar_int_get(..., &val, sizeof(val));
> 
> If I am not mistaken the iovar_int_get only passes &val.

We pass data and len:

        err = brcmf_fil_iovar_data_get(ifp, name, &data_le,
sizeof(data_le));
        if (err == 0)



> 
>> }
>>
>> brcmf_fil_iovar_int_get(.., const u32 *data, size_t len)
>> {
>> 	__le32 data_le = cpu_to_le32(*data);
>> 	...
>> }
>>
>> So here data_le also has an uninitialized value, and later on, this:
>>
>>
>>         buflen = brcmf_create_iovar(name, data, len, drvr->proto_buf,
>>                                     sizeof(drvr->proto_buf));
> 
> create_iovar is used for both get and set...
> 
>> }
>>
>> And this function does this:
>>
>> static u32
>> brcmf_create_iovar(char *name, const char *data, u32 datalen,
>>                    char *buf, u32 buflen)
>> {
>>         u32 len;
>>
>>         len = strlen(name) + 1;
>>
>>         if ((len + datalen) > buflen)
>>                 return 0;
>>
>>         memcpy(buf, name, len);
>>
>>         /* append data onto the end of the name string */
>>         if (data && datalen)
>>                 memcpy(&buf[len], data, datalen);
> 
> but it does not have that info so it does the memcpy. We could add an
> extra parameter to avoid the memcpy for get, but as said there are
> instances where get passes query data to firmware.

OK.

> 
>>         return len + datalen;
>> }
>>
>>
>> We are essentially copying into buf an uninitialized value coming from
>> the stack, which seems like a problem to me.
> 
> Functionally I still don't see the problem. For the instances where this
> is true, the buf will be overwritten with data from firmware. It may be
> a security issue as stack content is passed to an external device.

That's the part that I am mostly concerned with, and the fact that this
is a genuine uninitialized value, not under direct control, but still.
-- 
Florian