[PATCH net-next v3 1/2] net: phy: mscc: fix signedness bug in vsc85xx_downshift_get

[Gustavo A. R. Silva]

Currently, the error handling for the call to function
phy_read_paged() doesn't work because *reg_val* is of
type u16 (16 bits, unsigned), which makes it impossible
for it to hold a value less than 0.

Fix this by changing the type of variable *reg_val* to int.

Addresses-Coverity-ID: 1473970 ("Unsigned compared against 0")
Fixes: 6a0bfbbe20b0 ("net: phy: mscc: migrate to phy_select/restore_page 
functions")
Reviewed-by: Quentin Schulz 
Signed-off-by: Gustavo A. R. Silva 
---
Changes in v3:
 - Post patch to netdev.

Changes in v2:
 - Add Quentin's Reviewed-by to the commit log.

 drivers/net/phy/mscc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index bffe077..bff56c3 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -522,7 +522,7 @@ static int vsc85xx_mdix_set(struct phy_device *phydev, u8 
mdix)
 
 static int vsc85xx_downshift_get(struct phy_device *phydev, u8 *count)
 {
-   u16 reg_val;
+   int reg_val;
 
reg_val = phy_read_paged(phydev, MSCC_PHY_PAGE_EXTENDED,
 MSCC_PHY_ACTIPHY_CNTL);
-- 
2.7.4

[PATCH] rds: send: Fix dead code in rds_sendmsg

[Gustavo A. R. Silva]

Currently, code at label *out* is unreachable. Fix this by updating
variable *ret* with -EINVAL, so the jump to *out* can be properly
executed instead of directly returning from function.

Addresses-Coverity-ID: 1472059 ("Structurally dead code")
Fixes: 1e2b44e78eea ("rds: Enable RDS IPv6 support")
Signed-off-by: Gustavo A. R. Silva 
---
 net/rds/send.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/rds/send.c b/net/rds/send.c
index 9604e1f..18e2b4d 100644
--- a/net/rds/send.c
+++ b/net/rds/send.c
@@ -1126,7 +1126,7 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, 
size_t payload_len)
if (addr4 == htonl(INADDR_ANY) ||
addr4 == htonl(INADDR_BROADCAST) ||
IN_MULTICAST(ntohl(addr4))) {
-   return -EINVAL;
+   ret = -EINVAL;
goto out;
}
}
-- 
2.7.4

[PATCH] net: sched: cls_api: fix dead code in switch

[Gustavo A. R. Silva]

Code at line 1850 is unreachable. Fix this by removing the break
statement above it, so the code for case RTM_GETCHAIN can be
properly executed.

Addresses-Coverity-ID: 1472050 ("Structurally dead code")
Fixes: 32a4f5ecd738 ("net: sched: introduce chain object to uapi")
Signed-off-by: Gustavo A. R. Silva 
---
 net/sched/cls_api.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 5f7098b..f3d78c2 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -1846,7 +1846,6 @@ static int tc_ctl_chain(struct sk_buff *skb, struct 
nlmsghdr *n,
tcf_chain_put_explicitly_created(chain);
break;
case RTM_GETCHAIN:
-   break;
err = tc_chain_notify(chain, skb, n->nlmsg_seq,
  n->nlmsg_seq, n->nlmsg_type, true);
if (err < 0)
-- 
2.7.4

Re: [PATCH] ptp: fix missing break in switch

[Gustavo A. R. Silva]

On 07/18/2018 05:26 PM, David Miller wrote:
> From: "Gustavo A. R. Silva" 
> Date: Tue, 17 Jul 2018 20:17:33 -0500
> 
>> It seems that a *break* is missing in order to avoid falling through
>> to the default case. Otherwise, checking *chan* makes no sense.
>>
>> Fixes: 72df7a7244c0 ("ptp: Allow reassigning calibration pin function")
>> Signed-off-by: Gustavo A. R. Silva 
> 
> Applied and queued up for -stable, thank you.
> 

Great. Glad to help. :)

Thanks
--
Gustavo

[PATCH] mwifiex: mark expected switch fall-throughs

[Gustavo A. R. Silva]

In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.

Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 drivers/net/wireless/marvell/mwifiex/cfg80211.c | 4 
 drivers/net/wireless/marvell/mwifiex/scan.c | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c 
b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index 54a2297..16a705d 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -1158,6 +1158,7 @@ mwifiex_cfg80211_change_virtual_intf(struct wiphy *wiphy,
case NL80211_IFTYPE_UNSPECIFIED:
mwifiex_dbg(priv->adapter, INFO,
"%s: kept type as IBSS\n", dev->name);
+   /* fall through */
case NL80211_IFTYPE_ADHOC:  /* This shouldn't happen */
return 0;
default:
@@ -1188,6 +1189,7 @@ mwifiex_cfg80211_change_virtual_intf(struct wiphy *wiphy,
case NL80211_IFTYPE_UNSPECIFIED:
mwifiex_dbg(priv->adapter, INFO,
"%s: kept type as STA\n", dev->name);
+   /* fall through */
case NL80211_IFTYPE_STATION:/* This shouldn't happen */
return 0;
default:
@@ -1210,6 +1212,7 @@ mwifiex_cfg80211_change_virtual_intf(struct wiphy *wiphy,
case NL80211_IFTYPE_UNSPECIFIED:
mwifiex_dbg(priv->adapter, INFO,
"%s: kept type as AP\n", dev->name);
+   /* fall through */
case NL80211_IFTYPE_AP: /* This shouldn't happen */
return 0;
default:
@@ -1249,6 +1252,7 @@ mwifiex_cfg80211_change_virtual_intf(struct wiphy *wiphy,
case NL80211_IFTYPE_UNSPECIFIED:
mwifiex_dbg(priv->adapter, INFO,
"%s: kept type as P2P\n", dev->name);
+   /* fall through */
case NL80211_IFTYPE_P2P_CLIENT:
case NL80211_IFTYPE_P2P_GO:
return 0;
diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c 
b/drivers/net/wireless/marvell/mwifiex/scan.c
index d7ce7f7..19df92b 100644
--- a/drivers/net/wireless/marvell/mwifiex/scan.c
+++ b/drivers/net/wireless/marvell/mwifiex/scan.c
@@ -1308,6 +1308,7 @@ int mwifiex_update_bss_desc_with_ie(struct 
mwifiex_adapter *adapter,
 
case WLAN_EID_CHANNEL_SWITCH:
bss_entry->chan_sw_ie_present = true;
+   /* fall through */
case WLAN_EID_PWR_CAPABILITY:
case WLAN_EID_TPC_REPORT:
case WLAN_EID_QUIET:
-- 
2.7.4

[PATCH] ath9k: mark expected switch fall-throughs

[Gustavo A. R. Silva]

In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.

Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 drivers/net/wireless/ath/ath9k/ar5008_phy.c | 2 ++
 drivers/net/wireless/ath/ath9k/ar9002_phy.c | 1 +
 drivers/net/wireless/ath/ath9k/main.c   | 1 +
 3 files changed, 4 insertions(+)

diff --git a/drivers/net/wireless/ath/ath9k/ar5008_phy.c 
b/drivers/net/wireless/ath/ath9k/ar5008_phy.c
index 7922550..ef2dd68 100644
--- a/drivers/net/wireless/ath/ath9k/ar5008_phy.c
+++ b/drivers/net/wireless/ath/ath9k/ar5008_phy.c
@@ -583,12 +583,14 @@ static void ar5008_hw_init_chain_masks(struct ath_hw *ah)
case 0x5:
REG_SET_BIT(ah, AR_PHY_ANALOG_SWAP,
AR_PHY_SWAP_ALT_CHAIN);
+   /* fall through */
case 0x3:
if (ah->hw_version.macVersion == AR_SREV_REVISION_5416_10) {
REG_WRITE(ah, AR_PHY_RX_CHAINMASK, 0x7);
REG_WRITE(ah, AR_PHY_CAL_CHAINMASK, 0x7);
break;
}
+   /* else: fall through */
case 0x1:
case 0x2:
case 0x7:
diff --git a/drivers/net/wireless/ath/ath9k/ar9002_phy.c 
b/drivers/net/wireless/ath/ath9k/ar9002_phy.c
index 61a9b85..7132918 100644
--- a/drivers/net/wireless/ath/ath9k/ar9002_phy.c
+++ b/drivers/net/wireless/ath/ath9k/ar9002_phy.c
@@ -119,6 +119,7 @@ static int ar9002_hw_set_channel(struct ath_hw *ah, struct 
ath9k_channel *chan)
aModeRefSel = 2;
if (aModeRefSel)
break;
+   /* else: fall through */
case 1:
default:
aModeRefSel = 0;
diff --git a/drivers/net/wireless/ath/ath9k/main.c 
b/drivers/net/wireless/ath/ath9k/main.c
index a3be8ad..11d84f4 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -1928,6 +1928,7 @@ static int ath9k_ampdu_action(struct ieee80211_hw *hw,
case IEEE80211_AMPDU_TX_STOP_FLUSH:
case IEEE80211_AMPDU_TX_STOP_FLUSH_CONT:
flush = true;
+   /* fall through */
case IEEE80211_AMPDU_TX_STOP_CONT:
ath9k_ps_wakeup(sc);
ath_tx_aggr_stop(sc, sta, tid);
-- 
2.7.4

Re: [PATCH v2] ath6kl: mark expected switch fall-throughs

[Gustavo A. R. Silva]

On 05/25/2018 01:27 PM, Steve deRosier wrote:

On Fri, May 25, 2018 at 11:23 AM Gustavo A. R. Silva
<gust...@embeddedor.com>
wrote:


In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.



Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
Changes in v2:
- Place code comments on a line of their own.



drivers/net/wireless/ath/ath6kl/cfg80211.c | 3 +++
1 file changed, 3 insertions(+)



diff --git a/drivers/net/wireless/ath/ath6kl/cfg80211.c

b/drivers/net/wireless/ath/ath6kl/cfg80211.c

index 2ba8cf3..a16ee5d 100644
--- a/drivers/net/wireless/ath/ath6kl/cfg80211.c
+++ b/drivers/net/wireless/ath/ath6kl/cfg80211.c
@@ -3899,16 +3899,19 @@ int ath6kl_cfg80211_init(struct ath6kl *ar)
   switch (ar->hw.cap) {
   case WMI_11AN_CAP:
   ht = true;
+   /* fall through */
   case WMI_11A_CAP:
   band_5gig = true;
   break;
   case WMI_11GN_CAP:
   ht = true;
+   /* fall through */
   case WMI_11G_CAP:
   band_2gig = true;
   break;
   case WMI_11AGN_CAP:
   ht = true;
+   /* fall through */
   case WMI_11AG_CAP:
   band_2gig = true;
   band_5gig = true;
--
2.7.4



Gustavo,

Thanks for the adjustment.  It now looks good to me.



Glad to help. :)


Reviewed-by: Steve deRosier <deros...@cal-sierra.com>


Thanks
--
Gustavo

[PATCH v2] ath6kl: mark expected switch fall-throughs

[Gustavo A. R. Silva]

In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.

Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
Changes in v2:
 - Place code comments on a line of their own.

 drivers/net/wireless/ath/ath6kl/cfg80211.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/wireless/ath/ath6kl/cfg80211.c 
b/drivers/net/wireless/ath/ath6kl/cfg80211.c
index 2ba8cf3..a16ee5d 100644
--- a/drivers/net/wireless/ath/ath6kl/cfg80211.c
+++ b/drivers/net/wireless/ath/ath6kl/cfg80211.c
@@ -3899,16 +3899,19 @@ int ath6kl_cfg80211_init(struct ath6kl *ar)
switch (ar->hw.cap) {
case WMI_11AN_CAP:
ht = true;
+   /* fall through */
case WMI_11A_CAP:
band_5gig = true;
break;
case WMI_11GN_CAP:
ht = true;
+   /* fall through */
case WMI_11G_CAP:
band_2gig = true;
break;
case WMI_11AGN_CAP:
ht = true;
+   /* fall through */
case WMI_11AG_CAP:
band_2gig = true;
band_5gig = true;
-- 
2.7.4

Re: [PATCH] ath6kl: mark expected switch fall-throughs

[Gustavo A. R. Silva]

On 05/25/2018 01:10 PM, Kalle Valo wrote:

Yeah, I was wondering the same. Was there a particular reason for this?



Sometimes people use this style for a one-line code block.

I can change it to the traditional style. No problem.


I would prefer that. So if you can send v2 that would be great.



Yep. No problem. I'll send it shortly.

Thanks
--
Gustavo

Re: [PATCH] ath6kl: mark expected switch fall-throughs

[Gustavo A. R. Silva]

On 05/25/2018 08:30 AM, Kalle Valo wrote:

Sergei Shtylyov <sergei.shtyl...@cogentembedded.com> writes:


On 5/25/2018 2:13 AM, Gustavo A. R. Silva wrote:


In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.

Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
   drivers/net/wireless/ath/ath6kl/cfg80211.c | 6 +++---
   1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/cfg80211.c 
b/drivers/net/wireless/ath/ath6kl/cfg80211.c
index 2ba8cf3..29e32cd 100644
--- a/drivers/net/wireless/ath/ath6kl/cfg80211.c
+++ b/drivers/net/wireless/ath/ath6kl/cfg80211.c
@@ -3898,17 +3898,17 @@ int ath6kl_cfg80211_init(struct ath6kl *ar)
wiphy->max_scan_ie_len = 1000; /* FIX: what is correct limit? */
switch (ar->hw.cap) {
case WMI_11AN_CAP:
-   ht = true;
+   ht = true; /* fall through */
case WMI_11A_CAP:
band_5gig = true;
break;
case WMI_11GN_CAP:
-   ht = true;
+   ht = true; /* fall through */
case WMI_11G_CAP:
band_2gig = true;
break;
case WMI_11AGN_CAP:
-   ht = true;
+   ht = true; /* fall through */
case WMI_11AG_CAP:
band_2gig = true;
band_5gig = true;


Hm, typically such comments are done on a line of their own, have
never seen this style...


Yeah, I was wondering the same. Was there a particular reason for this?



Sometimes people use this style for a one-line code block.

I can change it to the traditional style. No problem.

Thanks
--
Gustavo

[PATCH] ath6kl: mark expected switch fall-throughs

[Gustavo A. R. Silva]

In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.

Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 drivers/net/wireless/ath/ath6kl/cfg80211.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/cfg80211.c 
b/drivers/net/wireless/ath/ath6kl/cfg80211.c
index 2ba8cf3..29e32cd 100644
--- a/drivers/net/wireless/ath/ath6kl/cfg80211.c
+++ b/drivers/net/wireless/ath/ath6kl/cfg80211.c
@@ -3898,17 +3898,17 @@ int ath6kl_cfg80211_init(struct ath6kl *ar)
wiphy->max_scan_ie_len = 1000; /* FIX: what is correct limit? */
switch (ar->hw.cap) {
case WMI_11AN_CAP:
-   ht = true;
+   ht = true; /* fall through */
case WMI_11A_CAP:
band_5gig = true;
break;
case WMI_11GN_CAP:
-   ht = true;
+   ht = true; /* fall through */
case WMI_11G_CAP:
band_2gig = true;
break;
case WMI_11AGN_CAP:
-   ht = true;
+   ht = true; /* fall through */
case WMI_11AG_CAP:
band_2gig = true;
band_5gig = true;
-- 
2.7.4

[PATCH] ath10k: htt_tx: mark expected switch fall-throughs

[Gustavo A. R. Silva]

In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.

Notice that in this particular case, I replaced "pass through" with
a proper "fall through" comment, which is what GCC is expecting
to find.

Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 drivers/net/wireless/ath/ath10k/htt_tx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt_tx.c 
b/drivers/net/wireless/ath/ath10k/htt_tx.c
index 5d8b97a..89157c5 100644
--- a/drivers/net/wireless/ath/ath10k/htt_tx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_tx.c
@@ -1202,7 +1202,7 @@ static int ath10k_htt_tx_32(struct ath10k_htt *htt,
case ATH10K_HW_TXRX_RAW:
case ATH10K_HW_TXRX_NATIVE_WIFI:
flags0 |= HTT_DATA_TX_DESC_FLAGS0_MAC_HDR_PRESENT;
-   /* pass through */
+   /* fall through */
case ATH10K_HW_TXRX_ETHERNET:
if (ar->hw_params.continuous_frag_desc) {
ext_desc_t = htt->frag_desc.vaddr_desc_32;
@@ -1404,7 +1404,7 @@ static int ath10k_htt_tx_64(struct ath10k_htt *htt,
case ATH10K_HW_TXRX_RAW:
case ATH10K_HW_TXRX_NATIVE_WIFI:
flags0 |= HTT_DATA_TX_DESC_FLAGS0_MAC_HDR_PRESENT;
-   /* pass through */
+   /* fall through */
case ATH10K_HW_TXRX_ETHERNET:
if (ar->hw_params.continuous_frag_desc) {
ext_desc_t = htt->frag_desc.vaddr_desc_64;
-- 
2.7.4

[PATCH] ath5k: mark expected switch fall-through

[Gustavo A. R. Silva]

In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.

Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 drivers/net/wireless/ath/ath5k/pcu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/wireless/ath/ath5k/pcu.c 
b/drivers/net/wireless/ath/ath5k/pcu.c
index f23c851..05140d8 100644
--- a/drivers/net/wireless/ath/ath5k/pcu.c
+++ b/drivers/net/wireless/ath/ath5k/pcu.c
@@ -670,6 +670,7 @@ ath5k_hw_init_beacon_timers(struct ath5k_hw *ah, u32 
next_beacon, u32 interval)
break;
case NL80211_IFTYPE_ADHOC:
AR5K_REG_ENABLE_BITS(ah, AR5K_TXCFG, AR5K_TXCFG_ADHOC_BCN_ATIM);
+   /* fall through */
default:
/* On non-STA modes timer1 is used as next DMA
 * beacon alert (DBA) timer and timer2 as next
-- 
2.7.4

Re: [PATCH] rtlwifi: remove duplicate code

[Gustavo A. R. Silva]

Hi Joe,

On 05/24/2018 02:24 PM, Joe Perches wrote:

On Thu, 2018-05-24 at 13:54 -0500, Gustavo A. R. Silva wrote:

Remove and refactor some code in order to avoid having identical code
for different branches.


True and nice tool and patch submittal thanks.


Notice that the logic has been there since 2014.


But perhaps the original logic is a defective copy/paste
and it should be corrected instead.

Can anyone from realtek verify this?



I actually used gitk to track down the last changes made to this code 
and, it doesn't look like a copy/paste issue:


commit: c6821613e653aae4f54c75689e229e3f063b7f69
commit: 27a31a60a4de4c1b45e371152bb6e701e1a8cc40

Thanks
--
Gustavo


Addresses-Coverity-ID: 1426199 ("Identical code for different branches")
Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
  .../realtek/rtlwifi/btcoexist/halbtc8723b2ant.c| 23 --
  1 file changed, 4 insertions(+), 19 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8723b2ant.c 
b/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8723b2ant.c
index 279fe01..df3facc 100644
--- a/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8723b2ant.c
+++ b/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8723b2ant.c
@@ -2876,25 +2876,10 @@ static void btc8723b2ant_action_hid(struct btc_coexist 
*btcoexist)
btc8723b2ant_ps_tdma(btcoexist, NORMAL_EXEC, true, 13);
  
  	/* sw mechanism */

-   if (BTC_WIFI_BW_HT40 == wifi_bw) {
-   if ((wifi_rssi_state == BTC_RSSI_STATE_HIGH) ||
-   (wifi_rssi_state == BTC_RSSI_STATE_STAY_HIGH)) {
-   btc8723b2ant_sw_mechanism(btcoexist, true, true,
- false, false);
-   } else {
-   btc8723b2ant_sw_mechanism(btcoexist, true, true,
- false, false);
-   }
-   } else {
-   if ((wifi_rssi_state == BTC_RSSI_STATE_HIGH) ||
-   (wifi_rssi_state == BTC_RSSI_STATE_STAY_HIGH)) {
-   btc8723b2ant_sw_mechanism(btcoexist, false, true,
- false, false);
-   } else {
-   btc8723b2ant_sw_mechanism(btcoexist, false, true,
- false, false);
-   }
-   }
+   if (wifi_bw == BTC_WIFI_BW_HT40)
+   btc8723b2ant_sw_mechanism(btcoexist, true, true, false, false);
+   else
+   btc8723b2ant_sw_mechanism(btcoexist, false, true, false, false);
  }
  
  /* A2DP only / PAN(EDR) only/ A2DP+PAN(HS) */

[PATCH] rtlwifi: remove duplicate code

[Gustavo A. R. Silva]

Remove and refactor some code in order to avoid having identical code
for different branches.

Notice that the logic has been there since 2014.

Addresses-Coverity-ID: 1426199 ("Identical code for different branches")
Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 .../realtek/rtlwifi/btcoexist/halbtc8723b2ant.c| 23 --
 1 file changed, 4 insertions(+), 19 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8723b2ant.c 
b/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8723b2ant.c
index 279fe01..df3facc 100644
--- a/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8723b2ant.c
+++ b/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8723b2ant.c
@@ -2876,25 +2876,10 @@ static void btc8723b2ant_action_hid(struct btc_coexist 
*btcoexist)
btc8723b2ant_ps_tdma(btcoexist, NORMAL_EXEC, true, 13);
 
/* sw mechanism */
-   if (BTC_WIFI_BW_HT40 == wifi_bw) {
-   if ((wifi_rssi_state == BTC_RSSI_STATE_HIGH) ||
-   (wifi_rssi_state == BTC_RSSI_STATE_STAY_HIGH)) {
-   btc8723b2ant_sw_mechanism(btcoexist, true, true,
- false, false);
-   } else {
-   btc8723b2ant_sw_mechanism(btcoexist, true, true,
- false, false);
-   }
-   } else {
-   if ((wifi_rssi_state == BTC_RSSI_STATE_HIGH) ||
-   (wifi_rssi_state == BTC_RSSI_STATE_STAY_HIGH)) {
-   btc8723b2ant_sw_mechanism(btcoexist, false, true,
- false, false);
-   } else {
-   btc8723b2ant_sw_mechanism(btcoexist, false, true,
- false, false);
-   }
-   }
+   if (wifi_bw == BTC_WIFI_BW_HT40)
+   btc8723b2ant_sw_mechanism(btcoexist, true, true, false, false);
+   else
+   btc8723b2ant_sw_mechanism(btcoexist, false, true, false, false);
 }
 
 /* A2DP only / PAN(EDR) only/ A2DP+PAN(HS) */
-- 
2.7.4

Re: [PATCH 1/2] bpf: sockmap, double free in __sock_map_ctx_update_elem()

[Gustavo A. R. Silva]

Hi Dan,

On 05/18/2018 09:39 AM, Dan Carpenter wrote:

On Fri, May 18, 2018 at 10:27:18AM +0200, Daniel Borkmann wrote:


Thanks for the two fixes, appreciate it! There were two similar ones that
fix the same issues which were already applied yesterday to bpf-next:

https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=0e4364560361d57e8cd873a8990327f3471d7d8a
https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=a78622932c27e8ec33e5ba180f3d2e87fb806b28


Hey Gustavo,

We're sort of duplicating each other's work.  Could you CC
kernel-janit...@vger.kernel.org for static checker fixes so that I can
see what you're working on?



Sure thing.

I've been doing this work for more than a year now and just recently we 
are having these issues. I'm a bit curious about it.



We'll probably still send the occasional duplicate which is fine...



Yep. Not a big deal for me.

Have a good one.
--
Gustavo

Re: [PATCH 0/2] bpf: sockmap, fix uninitialized variable and double-free

[Gustavo A. R. Silva]

Hi Daniel,

On 05/17/2018 03:51 PM, Daniel Borkmann wrote:

On 05/17/2018 04:04 PM, Gustavo A. R. Silva wrote:

This patchset aims to fix an uninitialized variable issue and
a double-free issue in __sock_map_ctx_update_elem.

Both issues were reported by Coverity.

Thanks.

Gustavo A. R. Silva (2):
   bpf: sockmap, fix uninitialized variable
   bpf: sockmap, fix double-free

  kernel/bpf/sockmap.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)


Applied to bpf-next, thanks Gustavo!



Glad to help. :)


P.s.: Please indicate that next time in the email subject via '[PATCH 
bpf-next]'.



OK. Will do that.

Thanks
--
Gustavo

Re: [PATCH 1/2] bpf: sockmap, fix uninitialized variable

[Gustavo A. R. Silva]

Hi John,

On 05/17/2018 12:27 PM, John Fastabend wrote:

On 05/17/2018 07:08 AM, Gustavo A. R. Silva wrote:

There is a potential execution path in which variable err is
returned without being properly initialized previously.

Fix this by initializing variable err to 0.

Addresses-Coverity-ID: 1468964 ("Uninitialized scalar variable")
Fixes: e5cd3abcb31a ("bpf: sockmap, refactor sockmap routines to work
with hashmap")
Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
  kernel/bpf/sockmap.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index c6de139..41b41fc 100644
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c
@@ -1713,7 +1713,7 @@ static int __sock_map_ctx_update_elem(struct bpf_map *map,
struct smap_psock_map_entry *e = NULL;
struct smap_psock *psock;
bool new = false;
-   int err;
+   int err = 0;
  
  	/* 1. If sock map has BPF programs those will be inherited by the

 * sock being added. If the sock is already attached to BPF programs



Thanks for catching this and the quick fix. The path to hit this case
is to add a sock to a map (without a BPF program) where the sock already
has been added to another map. I don't have any tests for the case with
socks in multiple maps so I'll add some to the selftests so I remember
this case.



Glad to help. :)


The alternative fix would be to always 'return 0' at the end of the
function, but I think its probably better to init err here like above.



Yeah. I think initializing err is better in this case.


Acked-by: John Fastabend <john.fastab...@gmail.com>



Thank you
--
Gustavo

[PATCH 2/2] bpf: sockmap, fix double-free

[Gustavo A. R. Silva]

`e' is being freed twice.

Fix this by removing one of the kfree() calls.

Addresses-Coverity-ID: 1468983 ("Double free")
Fixes: 81110384441a ("bpf: sockmap, add hash map support")
Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 kernel/bpf/sockmap.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index 41b41fc..c682669 100644
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c
@@ -1823,7 +1823,6 @@ static int __sock_map_ctx_update_elem(struct bpf_map *map,
write_unlock_bh(>sk_callback_lock);
return err;
 out_free:
-   kfree(e);
smap_release_sock(psock, sock);
 out_progs:
if (verdict)
-- 
2.7.4

[PATCH 1/2] bpf: sockmap, fix uninitialized variable

[Gustavo A. R. Silva]

There is a potential execution path in which variable err is
returned without being properly initialized previously.

Fix this by initializing variable err to 0.

Addresses-Coverity-ID: 1468964 ("Uninitialized scalar variable")
Fixes: e5cd3abcb31a ("bpf: sockmap, refactor sockmap routines to work
with hashmap")
Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 kernel/bpf/sockmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index c6de139..41b41fc 100644
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c
@@ -1713,7 +1713,7 @@ static int __sock_map_ctx_update_elem(struct bpf_map *map,
struct smap_psock_map_entry *e = NULL;
struct smap_psock *psock;
bool new = false;
-   int err;
+   int err = 0;
 
/* 1. If sock map has BPF programs those will be inherited by the
 * sock being added. If the sock is already attached to BPF programs
-- 
2.7.4

[PATCH 0/2] bpf: sockmap, fix uninitialized variable and double-free

[Gustavo A. R. Silva]

This patchset aims to fix an uninitialized variable issue and
a double-free issue in __sock_map_ctx_update_elem.

Both issues were reported by Coverity.

Thanks.

Gustavo A. R. Silva (2):
  bpf: sockmap, fix uninitialized variable
  bpf: sockmap, fix double-free

 kernel/bpf/sockmap.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

-- 
2.7.4

[PATCH] net: atm: Fix potential Spectre v1

[Gustavo A. R. Silva]

ioc_data.dev_num can be controlled by user-space, hence leading to
a potential exploitation of the Spectre variant 1 vulnerability.

This issue was detected with the help of Smatch:
net/atm/lec.c:702 lec_vcc_attach() warn: potential spectre issue
'dev_lec'

Fix this by sanitizing ioc_data.dev_num before using it to index
dev_lec. Also, notice that there is another instance in which array
dev_lec is being indexed using ioc_data.dev_num at line 705:
lec_vcc_added(netdev_priv(dev_lec[ioc_data.dev_num]),

Notice that given that speculation windows are large, the policy is
to kill the speculation on the first load and not worry if it can be
completed with a dependent load/store [1].

[1] https://marc.info/?l=linux-kernel=152449131114778=2

Cc: sta...@vger.kernel.org
Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 net/atm/lec.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/atm/lec.c b/net/atm/lec.c
index 01d5d20..3138a86 100644
--- a/net/atm/lec.c
+++ b/net/atm/lec.c
@@ -41,6 +41,9 @@ static unsigned char bridge_ula_lec[] = { 0x01, 0x80, 0xc2, 
0x00, 0x00 };
 #include 
 #include 
 
+/* Hardening for Spectre-v1 */
+#include 
+
 #include "lec.h"
 #include "lec_arpc.h"
 #include "resources.h"
@@ -687,8 +690,10 @@ static int lec_vcc_attach(struct atm_vcc *vcc, void __user 
*arg)
bytes_left = copy_from_user(_data, arg, sizeof(struct atmlec_ioc));
if (bytes_left != 0)
pr_info("copy from user failed for %d bytes\n", bytes_left);
-   if (ioc_data.dev_num < 0 || ioc_data.dev_num >= MAX_LEC_ITF ||
-   !dev_lec[ioc_data.dev_num])
+   if (ioc_data.dev_num < 0 || ioc_data.dev_num >= MAX_LEC_ITF)
+   return -EINVAL;
+   ioc_data.dev_num = array_index_nospec(ioc_data.dev_num, MAX_LEC_ITF);
+   if (!dev_lec[ioc_data.dev_num])
return -EINVAL;
vpriv = kmalloc(sizeof(struct lec_vcc_priv), GFP_KERNEL);
if (!vpriv)
-- 
2.7.4

[PATCH] atm: zatm: Fix potential Spectre v1

[Gustavo A. R. Silva]

pool can be indirectly controlled by user-space, hence leading to
a potential exploitation of the Spectre variant 1 vulnerability.

This issue was detected with the help of Smatch:

drivers/atm/zatm.c:1462 zatm_ioctl() warn: potential spectre issue
'zatm_dev->pool_info' (local cap)

Fix this by sanitizing pool before using it to index
zatm_dev->pool_info

Notice that given that speculation windows are large, the policy is
to kill the speculation on the first load and not worry if it can be
completed with a dependent load/store [1].

[1] https://marc.info/?l=linux-kernel=152449131114778=2

Cc: sta...@vger.kernel.org
Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 drivers/atm/zatm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/atm/zatm.c b/drivers/atm/zatm.c
index 1ef67db..9c9a229 100644
--- a/drivers/atm/zatm.c
+++ b/drivers/atm/zatm.c
@@ -28,6 +28,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "uPD98401.h"
 #include "uPD98402.h"
@@ -1458,6 +1459,8 @@ static int zatm_ioctl(struct atm_dev *dev,unsigned int 
cmd,void __user *arg)
return -EFAULT;
if (pool < 0 || pool > ZATM_LAST_POOL)
return -EINVAL;
+   pool = array_index_nospec(pool,
+ ZATM_LAST_POOL + 1);
spin_lock_irqsave(_dev->lock, flags);
info = zatm_dev->pool_info[pool];
if (cmd == ZATM_GETPOOLZ) {
-- 
2.7.4

[PATCH] rsi_91x_usb: fix uninitialized variable

[Gustavo A. R. Silva]

There is a potential execution path in which variable ret is returned
without being properly initialized previously.

Fix this by storing the value returned by function
rsi_usb_master_reg_write into _ret_.

Addresses-Coverity-ID: 1468407 ("Uninitialized scalar variable")
Fixes: 16d3bb7b2f37 ("rsi: disable fw watchdog timer during reset")
Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 drivers/net/wireless/rsi/rsi_91x_usb.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/rsi/rsi_91x_usb.c 
b/drivers/net/wireless/rsi/rsi_91x_usb.c
index b065438..6ce6b75 100644
--- a/drivers/net/wireless/rsi/rsi_91x_usb.c
+++ b/drivers/net/wireless/rsi/rsi_91x_usb.c
@@ -687,9 +687,10 @@ static int rsi_reset_card(struct rsi_hw *adapter)
 */
msleep(100);
 
-   if (rsi_usb_master_reg_write(adapter, SWBL_REGOUT,
-RSI_FW_WDT_DISABLE_REQ,
-RSI_COMMON_REG_SIZE) < 0) {
+   ret = rsi_usb_master_reg_write(adapter, SWBL_REGOUT,
+  RSI_FW_WDT_DISABLE_REQ,
+  RSI_COMMON_REG_SIZE);
+   if (ret < 0) {
rsi_dbg(ERR_ZONE, "Disabling firmware watchdog timer failed\n");
goto fail;
}
-- 
2.7.4

[PATCH] rsi_91x_mac80211: fix structurally dead code

[Gustavo A. R. Silva]

Function rsi_hal_key_config returns before reaching code at line
922 if (status), hence this code is structurally dead.

Fix this by storing the value returned by rsi_hal_load_key
into _status_ for its further evaluation and use.

Addresses-Coverity-ID: 1468409 ("Structurally dead code")
Fixes: 4fd6c4762f37 ("rsi: roaming enhancements")
Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 drivers/net/wireless/rsi/rsi_91x_mac80211.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/rsi/rsi_91x_mac80211.c 
b/drivers/net/wireless/rsi/rsi_91x_mac80211.c
index 766d874..80e7f4f 100644
--- a/drivers/net/wireless/rsi/rsi_91x_mac80211.c
+++ b/drivers/net/wireless/rsi/rsi_91x_mac80211.c
@@ -911,14 +911,14 @@ static int rsi_hal_key_config(struct ieee80211_hw *hw,
}
}
 
-   return rsi_hal_load_key(adapter->priv,
-   key->key,
-   key->keylen,
-   key_type,
-   key->keyidx,
-   key->cipher,
-   sta_id,
-   vif);
+   status = rsi_hal_load_key(adapter->priv,
+ key->key,
+ key->keylen,
+ key_type,
+ key->keyidx,
+ key->cipher,
+ sta_id,
+ vif);
if (status)
return status;
 
-- 
2.7.4

Re: [PATCH v2] net: thunderx: rework mac addresses list to u64 array

[Gustavo A. R. Silva]

On 04/06/2018 06:43 AM, Vadim Lomovtsev wrote:

Hi Gustavo,

On Fri, Apr 06, 2018 at 06:36:28AM -0500, Gustavo A. R. Silva wrote:

Hi Vadim,

On 04/06/2018 06:14 AM, Vadim Lomovtsev wrote:

From: Vadim Lomovtsev <vadim.lomovt...@cavium.com>

It is too expensive to pass u64 values via linked list, instead
allocate array for them by overall number of mac addresses from netdev.

This eventually removes multiple kmalloc() calls, aviod memory
fragmentation and allow to put single null check on kmalloc
return value in order to prevent a potential null pointer dereference.

Addresses-Coverity-ID: 1467429 ("Dereference null return value")
Fixes: 37c3347eb247 ("net: thunderx: add ndo_set_rx_mode callback implementation for 
VF")


It'd be nice if you add:

Reported-by: Gustavo A. R. Silva <gust...@embeddedor.com>


Ok, I could do that.

Just to explain .. I didn't do it yet since I get such report from
Dan Carpenter intially 
(https://www.spinics.net/lists/linux-kernel-janitors/msg40720.html)
and was working on it when found you patches, so then asking you to give
me some time to prepare and test update to driver.



Oh I got it. Not big deal. I think in this case you should add Dan's 
Reported-by instead.


BTW nice patch. :)

Thanks
--
Gustavo


So would it be acceptable put two tags 'Reported-by:' then ?

WBR,
Vadim



Thanks
--
Gustavo


Signed-off-by: Vadim Lomovtsev <vadim.lomovt...@cavium.com>
---
Changes from v1 to v2:
   - C99 syntax: update xcast_addr_list struct field mc[0] -> mc[];

---
   drivers/net/ethernet/cavium/thunder/nic.h|  7 +-
   drivers/net/ethernet/cavium/thunder/nicvf_main.c | 28 
+---
   2 files changed, 11 insertions(+), 24 deletions(-)

diff --git a/drivers/net/ethernet/cavium/thunder/nic.h 
b/drivers/net/ethernet/cavium/thunder/nic.h
index 5fc46c5a4f36..448d1fafc827 100644
--- a/drivers/net/ethernet/cavium/thunder/nic.h
+++ b/drivers/net/ethernet/cavium/thunder/nic.h
@@ -265,14 +265,9 @@ struct nicvf_drv_stats {
   struct cavium_ptp;
-struct xcast_addr {
-   struct list_head list;
-   u64  addr;
-};
-
   struct xcast_addr_list {
-   struct list_head list;
int  count;
+   u64  mc[];
   };
   struct nicvf_work {
diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c 
b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
index 1e9a31fef729..a26d8bc92e01 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
@@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct 
*work_arg)
  work.work);
struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
union nic_mbx mbx = {};
-   struct xcast_addr *xaddr, *next;
+   u8 idx = 0;
if (!vf_work)
return;
@@ -1956,16 +1956,10 @@ static void nicvf_set_rx_mode_task(struct work_struct 
*work_arg)
/* check if we have any specific MACs to be added to PF DMAC filter */
if (vf_work->mc) {
/* now go through kernel list of MACs and add them one by one */
-   list_for_each_entry_safe(xaddr, next,
-_work->mc->list, list) {
+   for (idx = 0; idx < vf_work->mc->count; idx++) {
mbx.xcast.msg = NIC_MBOX_MSG_ADD_MCAST;
-   mbx.xcast.data.mac = xaddr->addr;
+   mbx.xcast.data.mac = vf_work->mc->mc[idx];
nicvf_send_msg_to_pf(nic, );
-
-   /* after receiving ACK from PF release memory */
-   list_del(>list);
-   kfree(xaddr);
-   vf_work->mc->count--;
}
kfree(vf_work->mc);
}
@@ -1996,17 +1990,15 @@ static void nicvf_set_rx_mode(struct net_device *netdev)
mode |= BGX_XCAST_MCAST_FILTER;
/* here we need to copy mc addrs */
if (netdev_mc_count(netdev)) {
-   struct xcast_addr *xaddr;
-
-   mc_list = kmalloc(sizeof(*mc_list), GFP_ATOMIC);
-   INIT_LIST_HEAD(_list->list);
+   mc_list = kmalloc(sizeof(*mc_list) +
+ sizeof(u64) * 
netdev_mc_count(netdev),
+ GFP_ATOMIC);
+   if (unlikely(!mc_list))
+   return;
+   mc_list->count = 0;
netdev_hw_addr_list_for_each(ha, >mc) {
-   xaddr = kmalloc(sizeof(*xaddr),
-  

Re: [PATCH v2] net: thunderx: rework mac addresses list to u64 array

[Gustavo A. R. Silva]

Hi Vadim,

On 04/06/2018 06:14 AM, Vadim Lomovtsev wrote:

From: Vadim Lomovtsev <vadim.lomovt...@cavium.com>

It is too expensive to pass u64 values via linked list, instead
allocate array for them by overall number of mac addresses from netdev.

This eventually removes multiple kmalloc() calls, aviod memory
fragmentation and allow to put single null check on kmalloc
return value in order to prevent a potential null pointer dereference.

Addresses-Coverity-ID: 1467429 ("Dereference null return value")
Fixes: 37c3347eb247 ("net: thunderx: add ndo_set_rx_mode callback implementation for 
VF")


It'd be nice if you add:

Reported-by: Gustavo A. R. Silva <gust...@embeddedor.com>

Thanks
--
Gustavo


Signed-off-by: Vadim Lomovtsev <vadim.lomovt...@cavium.com>
---
Changes from v1 to v2:
  - C99 syntax: update xcast_addr_list struct field mc[0] -> mc[];

---
  drivers/net/ethernet/cavium/thunder/nic.h|  7 +-
  drivers/net/ethernet/cavium/thunder/nicvf_main.c | 28 +---
  2 files changed, 11 insertions(+), 24 deletions(-)

diff --git a/drivers/net/ethernet/cavium/thunder/nic.h 
b/drivers/net/ethernet/cavium/thunder/nic.h
index 5fc46c5a4f36..448d1fafc827 100644
--- a/drivers/net/ethernet/cavium/thunder/nic.h
+++ b/drivers/net/ethernet/cavium/thunder/nic.h
@@ -265,14 +265,9 @@ struct nicvf_drv_stats {
  
  struct cavium_ptp;
  
-struct xcast_addr {

-   struct list_head list;
-   u64  addr;
-};
-
  struct xcast_addr_list {
-   struct list_head list;
int  count;
+   u64  mc[];
  };
  
  struct nicvf_work {

diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c 
b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
index 1e9a31fef729..a26d8bc92e01 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
@@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct 
*work_arg)
  work.work);
struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
union nic_mbx mbx = {};
-   struct xcast_addr *xaddr, *next;
+   u8 idx = 0;
  
  	if (!vf_work)

return;
@@ -1956,16 +1956,10 @@ static void nicvf_set_rx_mode_task(struct work_struct 
*work_arg)
/* check if we have any specific MACs to be added to PF DMAC filter */
if (vf_work->mc) {
/* now go through kernel list of MACs and add them one by one */
-   list_for_each_entry_safe(xaddr, next,
-_work->mc->list, list) {
+   for (idx = 0; idx < vf_work->mc->count; idx++) {
mbx.xcast.msg = NIC_MBOX_MSG_ADD_MCAST;
-   mbx.xcast.data.mac = xaddr->addr;
+   mbx.xcast.data.mac = vf_work->mc->mc[idx];
nicvf_send_msg_to_pf(nic, );
-
-   /* after receiving ACK from PF release memory */
-   list_del(>list);
-   kfree(xaddr);
-   vf_work->mc->count--;
}
kfree(vf_work->mc);
}
@@ -1996,17 +1990,15 @@ static void nicvf_set_rx_mode(struct net_device *netdev)
mode |= BGX_XCAST_MCAST_FILTER;
/* here we need to copy mc addrs */
if (netdev_mc_count(netdev)) {
-   struct xcast_addr *xaddr;
-
-   mc_list = kmalloc(sizeof(*mc_list), GFP_ATOMIC);
-   INIT_LIST_HEAD(_list->list);
+   mc_list = kmalloc(sizeof(*mc_list) +
+ sizeof(u64) * 
netdev_mc_count(netdev),
+ GFP_ATOMIC);
+   if (unlikely(!mc_list))
+   return;
+   mc_list->count = 0;
netdev_hw_addr_list_for_each(ha, >mc) {
-   xaddr = kmalloc(sizeof(*xaddr),
-   GFP_ATOMIC);
-   xaddr->addr =
+   mc_list->mc[mc_list->count] =
ether_addr_to_u64(ha->addr);
-   list_add_tail(>list,
- _list->list);
mc_list->count++;
}
}

Re: [PATCH] qtnfmac: pearl: pcie: fix memory leak in qtnf_fw_work_handler

[Gustavo A. R. Silva]

Hi Sergey,

On 04/05/2018 11:31 AM, Sergey Matyukevich wrote:

Hello Gustavo,


In case memory resources for fw were succesfully allocated, release
them before jumping to fw_load_fail.

Addresses-Coverity-ID: 1466092 ("Resource leak")
Fixes: c3b2f7ca4186 ("qtnfmac: implement asynchronous firmware loading")
Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
  drivers/net/wireless/quantenna/qtnfmac/pearl/pcie.c | 4 
  1 file changed, 4 insertions(+)


Thanks for the patch!



Glad to help. :)


Reviewed-by: Sergey Matyukevich <sergey.matyukevich...@quantenna.com>



Thanks
--
Gustavo

[PATCH] ieee802154: mcr20a: Fix memory leak in mcr20a_probe

[Gustavo A. R. Silva]

Free allocated memory for pdata before return.

Addresses-Coverity-ID: 1466096 ("Resource leak")
Fixes: 8c6ad9cc5157 ("ieee802154: Add NXP MCR20A IEEE 802.15.4 transceiver 
driver")
Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 drivers/net/ieee802154/mcr20a.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ieee802154/mcr20a.c b/drivers/net/ieee802154/mcr20a.c
index 55a22c7..944470d 100644
--- a/drivers/net/ieee802154/mcr20a.c
+++ b/drivers/net/ieee802154/mcr20a.c
@@ -1267,7 +1267,7 @@ mcr20a_probe(struct spi_device *spi)
ret = mcr20a_get_platform_data(spi, pdata);
if (ret < 0) {
dev_crit(>dev, "mcr20a_get_platform_data failed.\n");
-   return ret;
+   goto free_pdata;
}
 
/* init reset gpio */
@@ -1275,7 +1275,7 @@ mcr20a_probe(struct spi_device *spi)
ret = devm_gpio_request_one(>dev, pdata->rst_gpio,
GPIOF_OUT_INIT_HIGH, "reset");
if (ret)
-   return ret;
+   goto free_pdata;
}
 
/* reset mcr20a */
@@ -1291,7 +1291,8 @@ mcr20a_probe(struct spi_device *spi)
hw = ieee802154_alloc_hw(sizeof(*lp), _hw_ops);
if (!hw) {
dev_crit(>dev, "ieee802154_alloc_hw failed\n");
-   return -ENOMEM;
+   ret = -ENOMEM;
+   goto free_pdata;
}
 
/* init mcr20a local data */
@@ -1366,6 +1367,8 @@ mcr20a_probe(struct spi_device *spi)
 
 free_dev:
ieee802154_free_hw(lp->hw);
+free_pdata:
+   kfree(pdata);
 
return ret;
 }
-- 
2.7.4

[PATCH] qtnfmac: pearl: pcie: fix memory leak in qtnf_fw_work_handler

[Gustavo A. R. Silva]

In case memory resources for fw were succesfully allocated, release
them before jumping to fw_load_fail.

Addresses-Coverity-ID: 1466092 ("Resource leak")
Fixes: c3b2f7ca4186 ("qtnfmac: implement asynchronous firmware loading")
Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 drivers/net/wireless/quantenna/qtnfmac/pearl/pcie.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/wireless/quantenna/qtnfmac/pearl/pcie.c 
b/drivers/net/wireless/quantenna/qtnfmac/pearl/pcie.c
index f117904..6c1e139 100644
--- a/drivers/net/wireless/quantenna/qtnfmac/pearl/pcie.c
+++ b/drivers/net/wireless/quantenna/qtnfmac/pearl/pcie.c
@@ -1185,6 +1185,10 @@ static void qtnf_fw_work_handler(struct work_struct 
*work)
if (qtnf_poll_state(>bda->bda_ep_state, QTN_EP_FW_LOADRDY,
QTN_FW_DL_TIMEOUT_MS)) {
pr_err("card is not ready\n");
+
+   if (!flashboot)
+   release_firmware(fw);
+
goto fw_load_fail;
}
 
-- 
2.7.4

Re: [PATCH v2] Bluetooth: Remove VLA usage in aes_cmac

[Gustavo A. R. Silva]

On 04/05/2018 03:46 AM, Marcel Holtmann wrote:


By the way, what is you opinion on replacing crypto_shash_descsize(ctx) with 
PAGE_SIZE / 8 in SHASH_DESC_ON_STACK?

Does it work for you?


isn’t that just waste?



Agree.


The macro itself is this.

#define SHASH_DESC_ON_STACK(shash, ctx)   \
 char __##shash##_desc[sizeof(struct shash_desc) + \
 crypto_shash_descsize(ctx)] CRYPTO_MINALIGN_ATTR; \
 struct shash_desc *shash = (struct shash_desc *)__##shash##_desc

For AES-CMAC, we could always do this with a manual macro that gives us the 
right size. However that is error prone if any internals change. I think what 
has to happened that crypto_shash_decsize becomes something the compiler can 
evaluate at compile time.



Yeah. That would imply an analysis of the algorithm each of the callers 
use. In the case of AES-CMAC, what is the maximum digest size?


I tried to find a fixed-length value for AES-CMAC but I didn't get any 
output with git grep -n _DIGEST_SIZE | grep AES


Thanks
--
Gustavo

Re: [PATCH v2] Bluetooth: Remove VLA usage in aes_cmac

[Gustavo A. R. Silva]

Hi Marcel,

On 04/05/2018 02:23 AM, Marcel Holtmann wrote:


so I took this patch back out of bluetooth-next before sending the pull 
request. I think the discussion on how to fix SHASH_DESC_ON_STACK macro needs 
to complete first. Once that has concluded we can revisit if this patch is 
still needed or if another solution has been found. Same as with WiFi, these 
are not just one-shot calls where a memory allocation doesn’t matter. We need 
this for random address resolution and thus there can be many of the aes_cmac 
calls when seeing neighboring devices.



Yeah. I agree.

Based on Herbert's response to the discussion about SHASH_DESC_ON_STACK 
https://lkml.org/lkml/2018/3/27/300


it seems it is feasible to fix that macro very easily. I will follow up 
on this.


By the way, what is you opinion on replacing crypto_shash_descsize(ctx) 
with PAGE_SIZE / 8 in SHASH_DESC_ON_STACK?


Does it work for you?

Thanks
--
Gustavo

[rtlwifi-btcoex] Suspicious code in halbtc8821a1ant driver

[Gustavo A. R. Silva]

Hi all,

While doing some static analysis I came across the following piece of code at 
drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a1ant.c:1581:

1581 static void btc8821a1ant_act_bt_sco_hid_only_busy(struct btc_coexist 
*btcoexist,
1582   u8 wifi_status)
1583 {
1584 /* tdma and coex table */
1585 btc8821a1ant_ps_tdma(btcoexist, NORMAL_EXEC, true, 5);
1586 
1587 if (BT_8821A_1ANT_WIFI_STATUS_NON_CONNECTED_ASSO_AUTH_SCAN ==
1588 wifi_status)
1589 btc8821a1ant_coex_table_with_type(btcoexist, NORMAL_EXEC, 
1);
1590 else
1591 btc8821a1ant_coex_table_with_type(btcoexist, NORMAL_EXEC, 
1);
1592 }

The issue here is that the code for both branches of the if-else statement is 
identical.

The if-else was introduced a year ago in this commit c6821613e653

I wonder if an argument should be changed in any of the calls to 
btc8821a1ant_coex_table_with_type?

What do you think?

Thanks
--
Gustavo

[PATCH] brcm80211: brcmsmac: phy_lcn: remove duplicate code

[Gustavo A. R. Silva]

Remove and refactor some code in order to avoid having identical code
for different branches.

Notice that this piece of code hasn't been modified since 2011.

Addresses-Coverity-ID: 1226756 ("Identical code for different branches")
Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_lcn.c | 9 ++---
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_lcn.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_lcn.c
index 93d4cde..9d830d2 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_lcn.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_lcn.c
@@ -3388,13 +3388,8 @@ void wlc_lcnphy_deaf_mode(struct brcms_phy *pi, bool 
mode)
u8 phybw40;
phybw40 = CHSPEC_IS40(pi->radio_chanspec);
 
-   if (LCNREV_LT(pi->pubpi.phy_rev, 2)) {
-   mod_phy_reg(pi, 0x4b0, (0x1 << 5), (mode) << 5);
-   mod_phy_reg(pi, 0x4b1, (0x1 << 9), 0 << 9);
-   } else {
-   mod_phy_reg(pi, 0x4b0, (0x1 << 5), (mode) << 5);
-   mod_phy_reg(pi, 0x4b1, (0x1 << 9), 0 << 9);
-   }
+   mod_phy_reg(pi, 0x4b0, (0x1 << 5), (mode) << 5);
+   mod_phy_reg(pi, 0x4b1, (0x1 << 9), 0 << 9);
 
if (phybw40 == 0) {
mod_phy_reg((pi), 0x410,
-- 
2.7.4

Re: [PATCH v2] net: thunderx: nicvf_main: Fix potential NULL pointer dereferences

[Gustavo A. R. Silva]

On 04/04/2018 01:36 AM, Eric Dumazet wrote:



On 04/03/2018 03:04 PM, Gustavo A. R. Silva wrote:

Add null check on kmalloc() return value in order to prevent
a null pointer dereference.

Addresses-Coverity-ID: 1467429 ("Dereference null return value")
Fixes: 37c3347eb247 ("net: thunderx: add ndo_set_rx_mode callback implementation for 
VF")
Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
Changes in v2:
  - Add a null check on a second kmalloc a few lines below. Thanks to
Eric Dumazet for pointing this out.

  drivers/net/ethernet/cavium/thunder/nicvf_main.c | 4 
  1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c 
b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
index 1e9a31f..f7b5ca5 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
@@ -1999,10 +1999,14 @@ static void nicvf_set_rx_mode(struct net_device *netdev)
struct xcast_addr *xaddr;
  
  mc_list = kmalloc(sizeof(*mc_list), GFP_ATOMIC);

+   if (unlikely(!mc_list))
+   return;
INIT_LIST_HEAD(_list->list);
netdev_hw_addr_list_for_each(ha, >mc) {
xaddr = kmalloc(sizeof(*xaddr),
GFP_ATOMIC);
+   if (unlikely(!xaddr))
+   return;


So now you leak memory :/



Oh god. I'm sorry.

I had problems with this file since the beginning.

I'll send v3 shortly.

Thanks
--
Gustavo

[PATCH v2] net: thunderx: nicvf_main: Fix potential NULL pointer dereferences

[Gustavo A. R. Silva]

Add null check on kmalloc() return value in order to prevent
a null pointer dereference.

Addresses-Coverity-ID: 1467429 ("Dereference null return value")
Fixes: 37c3347eb247 ("net: thunderx: add ndo_set_rx_mode callback 
implementation for VF")
Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
Changes in v2:
 - Add a null check on a second kmalloc a few lines below. Thanks to
   Eric Dumazet for pointing this out.

 drivers/net/ethernet/cavium/thunder/nicvf_main.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c 
b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
index 1e9a31f..f7b5ca5 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
@@ -1999,10 +1999,14 @@ static void nicvf_set_rx_mode(struct net_device *netdev)
struct xcast_addr *xaddr;
 
mc_list = kmalloc(sizeof(*mc_list), GFP_ATOMIC);
+   if (unlikely(!mc_list))
+   return;
INIT_LIST_HEAD(_list->list);
netdev_hw_addr_list_for_each(ha, >mc) {
xaddr = kmalloc(sizeof(*xaddr),
GFP_ATOMIC);
+   if (unlikely(!xaddr))
+   return;
xaddr->addr =
ether_addr_to_u64(ha->addr);
list_add_tail(>list,
-- 
2.7.4

Re: [PATCH] net: thunderx: nicvf_main: Fix potential NULL pointer dereference

[Gustavo A. R. Silva]

On 04/03/2018 04:47 PM, Eric Dumazet wrote:



On 04/03/2018 02:29 PM, Gustavo A. R. Silva wrote:

Add null check on kmalloc() return value in order to prevent
a null pointer dereference.

Addresses-Coverity-ID: 1467429 ("Dereference null return value")
Fixes: 37c3347eb247 ("net: thunderx: add ndo_set_rx_mode callback implementation for 
VF")
Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
  drivers/net/ethernet/cavium/thunder/nicvf_main.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c 
b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
index 1e9a31f..468321a 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
@@ -1999,6 +1999,8 @@ static void nicvf_set_rx_mode(struct net_device *netdev)
struct xcast_addr *xaddr;
  
  mc_list = kmalloc(sizeof(*mc_list), GFP_ATOMIC);

+   if (unlikely(!mc_list))
+   return;
INIT_LIST_HEAD(_list->list);
netdev_hw_addr_list_for_each(ha, >mc) {
xaddr = kmalloc(sizeof(*xaddr),



What about the second kmalloc() right there ?



Oops. I thought I had it covered.

I'll send v2 with that change shortly.

Thanks for the feedback, Eric.
--
Gustavo

[PATCH] net: thunderx: nicvf_main: Fix potential NULL pointer dereference

[Gustavo A. R. Silva]

Add null check on kmalloc() return value in order to prevent
a null pointer dereference.

Addresses-Coverity-ID: 1467429 ("Dereference null return value")
Fixes: 37c3347eb247 ("net: thunderx: add ndo_set_rx_mode callback 
implementation for VF")
Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 drivers/net/ethernet/cavium/thunder/nicvf_main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c 
b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
index 1e9a31f..468321a 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
@@ -1999,6 +1999,8 @@ static void nicvf_set_rx_mode(struct net_device *netdev)
struct xcast_addr *xaddr;
 
mc_list = kmalloc(sizeof(*mc_list), GFP_ATOMIC);
+   if (unlikely(!mc_list))
+   return;
INIT_LIST_HEAD(_list->list);
netdev_hw_addr_list_for_each(ha, >mc) {
xaddr = kmalloc(sizeof(*xaddr),
-- 
2.7.4

[PATCH] mt7601u: phy: mark expected switch fall-through

[Gustavo A. R. Silva]

In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.

Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 drivers/net/wireless/mediatek/mt7601u/phy.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/wireless/mediatek/mt7601u/phy.c 
b/drivers/net/wireless/mediatek/mt7601u/phy.c
index ca09a5d..9a90f1f 100644
--- a/drivers/net/wireless/mediatek/mt7601u/phy.c
+++ b/drivers/net/wireless/mediatek/mt7601u/phy.c
@@ -795,6 +795,7 @@ mt7601u_phy_rf_pa_mode_val(struct mt7601u_dev *dev, int 
phy_mode, int tx_rate)
switch (phy_mode) {
case MT_PHY_TYPE_OFDM:
tx_rate += 4;
+   /* fall through */
case MT_PHY_TYPE_CCK:
reg = dev->rf_pa_mode[0];
break;
-- 
2.7.4

[PATCH] Bluetooth: Mark expected switch fall-throughs

[Gustavo A. R. Silva]

In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.

Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 net/bluetooth/mgmt.c| 1 +
 net/bluetooth/rfcomm/sock.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 6e9fc86..8a80d48 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -4801,6 +4801,7 @@ static int load_long_term_keys(struct sock *sk, struct 
hci_dev *hdev,
case MGMT_LTK_P256_DEBUG:
authenticated = 0x00;
type = SMP_LTK_P256_DEBUG;
+   /* fall through */
default:
continue;
}
diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
index 93a3b21..d606e92 100644
--- a/net/bluetooth/rfcomm/sock.c
+++ b/net/bluetooth/rfcomm/sock.c
@@ -221,6 +221,7 @@ static void __rfcomm_sock_close(struct sock *sk)
case BT_CONFIG:
case BT_CONNECTED:
rfcomm_dlc_close(d, 0);
+   /* fall through */
 
default:
sock_set_flag(sk, SOCK_ZAPPED);
-- 
2.7.4

[PATCH] ath9k: dfs: remove accidental use of stack VLA

[Gustavo A. R. Silva]

In preparation to enabling -Wvla, remove accidental use of stack VLA.

This avoids an accidental stack VLA (since the compiler thinks
the value of FFT_NUM_SAMPLES can change, even when marked
"const"). This just replaces it with a #define.

Also, fixed as part of the directive to remove all VLAs from
the kernel: https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 drivers/net/wireless/ath/ath9k/dfs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/dfs.c 
b/drivers/net/wireless/ath/ath9k/dfs.c
index 6fee9a4..c8844f5 100644
--- a/drivers/net/wireless/ath/ath9k/dfs.c
+++ b/drivers/net/wireless/ath/ath9k/dfs.c
@@ -41,7 +41,7 @@ static const int BIN_DELTA_MAX= 10;
 
 /* we need at least 3 deltas / 4 samples for a reliable chirp detection */
 #define NUM_DIFFS 3
-static const int FFT_NUM_SAMPLES   = (NUM_DIFFS + 1);
+#define FFT_NUM_SAMPLES(NUM_DIFFS + 1)
 
 /* Threshold for difference of delta peaks */
 static const int MAX_DIFF  = 2;
@@ -114,7 +114,7 @@ static bool ath9k_check_chirping(struct ath_softc *sc, u8 
*data,
 
ath_dbg(common, DFS, "HT40: datalen=%d, num_fft_packets=%d\n",
datalen, num_fft_packets);
-   if (num_fft_packets < (FFT_NUM_SAMPLES)) {
+   if (num_fft_packets < FFT_NUM_SAMPLES) {
ath_dbg(common, DFS, "not enough packets for chirp\n");
return false;
}
@@ -136,7 +136,7 @@ static bool ath9k_check_chirping(struct ath_softc *sc, u8 
*data,
return false;
ath_dbg(common, DFS, "HT20: datalen=%d, num_fft_packets=%d\n",
datalen, num_fft_packets);
-   if (num_fft_packets < (FFT_NUM_SAMPLES)) {
+   if (num_fft_packets < FFT_NUM_SAMPLES) {
ath_dbg(common, DFS, "not enough packets for chirp\n");
return false;
}
-- 
2.7.4

Re: [PATCH] ISDN: eicon: message: remove redundant check

[Gustavo A. R. Silva]

On 03/30/2018 11:19 AM, Joe Perches wrote:

On Fri, 2018-03-30 at 10:46 -0500, Gustavo A. R. Silva wrote:

Check on plci->internal_command is unnecessary.


Probably all of these are unnecessary too:

$ for length in {7..2} ; do \
 grep-2.5.4 -rP --include=*.[ch] -n 
"^\t{$length,$length}break;\n\t{$(($length-1)),$(($length-1))}break;" * ; \
   done
drivers/staging/wilc1000/wilc_wlan.c:691:   break;
break;
drivers/media/dvb-frontends/drxd_hard.c:2261:   break;
break;
drivers/media/dvb-frontends/drxd_hard.c:2266:   break;
break;
drivers/media/usb/gspca/sn9c20x.c:1860: break;
break;
drivers/isdn/i4l/isdn_common.c:624: break;
break;
drivers/isdn/i4l/isdn_common.c:642: break;
break;
drivers/isdn/i4l/isdn_common.c:654: break;
break;
drivers/isdn/hardware/eicon/message.c:13890:break;
break;
sound/usb/mixer_quirks.c:1832:  break;
break;



Oh, cool.

I'll take a look at them.

Thanks, Joe.
--
Gustavo

[PATCH] ISDN: eicon: message: remove redundant check

[Gustavo A. R. Silva]

Check on plci->internal_command is unnecessary.

Addresses-Coverity-ID: 1268778 ("Identical code for different branches")
Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 drivers/isdn/hardware/eicon/message.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/isdn/hardware/eicon/message.c 
b/drivers/isdn/hardware/eicon/message.c
index def7992..0ac18fc 100644
--- a/drivers/isdn/hardware/eicon/message.c
+++ b/drivers/isdn/hardware/eicon/message.c
@@ -13886,8 +13886,6 @@ static void adjust_b_restore(dword Id, PLCI *plci, byte 
Rc)
dbug(1, dprintf("[%06lx] %s,%d: Adjust B restore 
failed",
UnMapId(Id), (char *)(FILE_), 
__LINE__));
}
-   if (plci->internal_command)
-   break;
break;
}
 }
-- 
2.7.4

[PATCH] qed: Use true and false for boolean values

[Gustavo A. R. Silva]

Assign true or false to boolean variables instead of an integer value.

This issue was detected with the help of Coccinelle.

Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 drivers/net/ethernet/qlogic/qed/qed_dev.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qed/qed_dev.c 
b/drivers/net/ethernet/qlogic/qed/qed_dev.c
index 553a6d1..cdb3eec 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_dev.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_dev.c
@@ -298,8 +298,8 @@ static void qed_init_qm_params(struct qed_hwfn *p_hwfn)
qm_info->start_vport = (u8) RESC_START(p_hwfn, QED_VPORT);
 
/* rate limiting and weighted fair queueing are always enabled */
-   qm_info->vport_rl_en = 1;
-   qm_info->vport_wfq_en = 1;
+   qm_info->vport_rl_en = true;
+   qm_info->vport_wfq_en = true;
 
/* TC config is different for AH 4 port */
four_port = p_hwfn->cdev->num_ports_in_engine == MAX_NUM_PORTS_K2;
@@ -1276,9 +1276,9 @@ static int qed_hw_init_common(struct qed_hwfn *p_hwfn,
 
if (p_hwfn->mcp_info) {
if (p_hwfn->mcp_info->func_info.bandwidth_max)
-   qm_info->pf_rl_en = 1;
+   qm_info->pf_rl_en = true;
if (p_hwfn->mcp_info->func_info.bandwidth_min)
-   qm_info->pf_wfq_en = 1;
+   qm_info->pf_wfq_en = true;
}
 
memset(, 0, sizeof(params));
@@ -1630,7 +1630,7 @@ static int qed_vf_start(struct qed_hwfn *p_hwfn,
qed_vf_pf_tunnel_param_update(p_hwfn, p_params->p_tunn);
}
 
-   p_hwfn->b_int_enabled = 1;
+   p_hwfn->b_int_enabled = true;
 
return 0;
 }
-- 
2.7.4

[PATCH] dpaa_eth: use true and false for boolean values

[Gustavo A. R. Silva]

Assign true or false to boolean variables instead of an integer value.

This issue was detected with the help of Coccinelle.

Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c 
b/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c
index 85306d1..c7ea193 100644
--- a/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c
+++ b/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c
@@ -344,7 +344,7 @@ static void dpaa_get_ethtool_stats(struct net_device 
*net_dev,
 
/* gather congestion related counters */
cg_num= 0;
-   cg_status = 0;
+   cg_status = false;
cg_time   = jiffies_to_msecs(priv->cgr_data.congested_jiffies);
if (qman_query_cgr_congested(>cgr_data.cgr, _status) == 0) {
cg_num= priv->cgr_data.cgr_congested_count;
-- 
2.7.4

Re: [PATCH] caif_dev: use true and false for boolean values

[Gustavo A. R. Silva]

Hi all,

I was just wondering about the status of this patch.

Thanks!
--
Gustavo

On 03/05/2018 04:05 PM, Gustavo A. R. Silva wrote:

Assign true or false to boolean variables instead of an integer value.

This issue was detected with the help of Coccinelle.

Signed-off-by: Gustavo A. R. Silva <garsi...@embeddedor.com>
---
  net/caif/caif_dev.c | 8 
  1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/caif/caif_dev.c b/net/caif/caif_dev.c
index e0adcd1..f2848d6 100644
--- a/net/caif/caif_dev.c
+++ b/net/caif/caif_dev.c
@@ -139,7 +139,7 @@ static void caif_flow_cb(struct sk_buff *skb)
  
  	spin_lock_bh(>flow_lock);

send_xoff = caifd->xoff;
-   caifd->xoff = 0;
+   caifd->xoff = false;
dtor = caifd->xoff_skb_dtor;
  
  	if (WARN_ON(caifd->xoff_skb != skb))

@@ -213,7 +213,7 @@ static int transmit(struct cflayer *layer, struct cfpkt 
*pkt)
pr_debug("queue has stopped(%d) or is full (%d > %d)\n",
netif_queue_stopped(caifd->netdev),
qlen, high);
-   caifd->xoff = 1;
+   caifd->xoff = true;
caifd->xoff_skb = skb;
caifd->xoff_skb_dtor = skb->destructor;
skb->destructor = caif_flow_cb;
@@ -400,7 +400,7 @@ static int caif_device_notify(struct notifier_block *me, 
unsigned long what,
break;
}
  
-		caifd->xoff = 0;

+   caifd->xoff = false;
cfcnfg_set_phy_state(cfg, >layer, true);
rcu_read_unlock();
  
@@ -435,7 +435,7 @@ static int caif_device_notify(struct notifier_block *me, unsigned long what,

if (caifd->xoff_skb_dtor != NULL && caifd->xoff_skb != NULL)
caifd->xoff_skb->destructor = caifd->xoff_skb_dtor;
  
-		caifd->xoff = 0;

+   caifd->xoff = false;
caifd->xoff_skb_dtor = NULL;
caifd->xoff_skb = NULL;
  

[PATCH v2] net/mlx5: Fix use-after-free

[Gustavo A. R. Silva]

_rule_ is being freed and then dereferenced by accessing rule->ctx

Fix this by copying the value returned by PTR_ERR(rule->ctx) into a local
variable for its safe use after freeing _rule_

Addresses-Coverity-ID: 1466041 ("Read from pointer after free")
Fixes: 05564d0ae075 ("net/mlx5: Add flow-steering commands for FPGA IPSec 
implementation")
Reviewed-by: Yuval Shaia <yuval.sh...@oracle.com>
Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
Changes in v2:
 - Use a short subject prefix as suggested by Yuval Shaia.
 - Add Yuval's Reviewed-by.

 drivers/net/ethernet/mellanox/mlx5/core/fpga/ipsec.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fpga/ipsec.c 
b/drivers/net/ethernet/mellanox/mlx5/core/fpga/ipsec.c
index 4f15685..0f5da49 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/fpga/ipsec.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/fpga/ipsec.c
@@ -1061,8 +1061,9 @@ static int fpga_ipsec_fs_create_fte(struct mlx5_core_dev 
*dev,
 
rule->ctx = mlx5_fpga_ipsec_fs_create_sa_ctx(dev, fte, is_egress);
if (IS_ERR(rule->ctx)) {
+   int err = PTR_ERR(rule->ctx);
kfree(rule);
-   return PTR_ERR(rule->ctx);
+   return err;
}
 
rule->fte = fte;
-- 
2.7.4

Re: [PATCH] net/mlx5/core/fpga/ipsec: Fix use-after-free

[Gustavo A. R. Silva]

Hi Yuval,

On 03/22/2018 01:32 PM, Yuval Shaia wrote:

On Thu, Mar 22, 2018 at 01:03:42PM -0500, Gustavo A. R. Silva wrote:

_rule_ is being freed and then dereferenced by accessing rule->ctx

Fix this by copying the value returned by PTR_ERR(rule->ctx) into a local
variable for its safe use after freeing _rule_

Addresses-Coverity-ID: 1466041 ("Read from pointer after free")
Fixes: 05564d0ae075 ("net/mlx5: Add flow-steering commands for FPGA IPSec 
implementation")
Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>


Prefix should not be that long, a short one as this is enough.



Yeah. Actually, I was suspicious about it.


net/mlx5: Fix use-after-free

Besides that - lgtm.

Reviewed-by: Yuval Shaia <yuval.sh...@oracle.com>



I'll send v2 with a short prefix and add your Reviewed-by.

Thanks for the feedback.
--
Gustavo

[PATCH] net/mlx5/core/fpga/ipsec: Fix use-after-free

[Gustavo A. R. Silva]

_rule_ is being freed and then dereferenced by accessing rule->ctx

Fix this by copying the value returned by PTR_ERR(rule->ctx) into a local
variable for its safe use after freeing _rule_

Addresses-Coverity-ID: 1466041 ("Read from pointer after free")
Fixes: 05564d0ae075 ("net/mlx5: Add flow-steering commands for FPGA IPSec 
implementation")
Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/fpga/ipsec.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fpga/ipsec.c 
b/drivers/net/ethernet/mellanox/mlx5/core/fpga/ipsec.c
index 4f15685..0f5da49 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/fpga/ipsec.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/fpga/ipsec.c
@@ -1061,8 +1061,9 @@ static int fpga_ipsec_fs_create_fte(struct mlx5_core_dev 
*dev,
 
rule->ctx = mlx5_fpga_ipsec_fs_create_sa_ctx(dev, fte, is_egress);
if (IS_ERR(rule->ctx)) {
+   int err = PTR_ERR(rule->ctx);
kfree(rule);
-   return PTR_ERR(rule->ctx);
+   return err;
}
 
rule->fte = fte;
-- 
2.7.4

Re: [PATCH v2] Bluetooth: Remove VLA usage in aes_cmac

[Gustavo A. R. Silva]

On 03/21/2018 08:45 AM, Marcel Holtmann wrote:

Hi Gustavo,


In preparation to enabling -Wvla, remove VLA and replace it
with dynamic memory allocation instead.

The use of stack Variable Length Arrays needs to be avoided, as they
can be a vector for stack exhaustion, which can be both a runtime bug
or a security flaw. Also, in general, as code evolves it is easy to
lose track of how big a VLA can get. Thus, we can end up having runtime
failures that are hard to debug.

Also, fixed as part of the directive to remove all VLAs from
the kernel: https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Gustavo A. R. Silva <garsi...@embeddedor.com>
---
Changes in v2:
- Fix memory leak in previous patch.

net/bluetooth/smp.c | 17 -
1 file changed, 12 insertions(+), 5 deletions(-)


patch has been applied to bluetooth-next tree.

Regards

Marcel



Awesome.

Thanks, Marcel.
--
Gustavo

Re: [PATCH] mac80211: aes-cmac: remove VLA usage

[Gustavo A. R. Silva]

On 03/21/2018 08:58 AM, Johannes Berg wrote:

On Wed, 2018-03-21 at 08:57 -0500, Gustavo A. R. Silva wrote:


SHA_DESC_ON_STACK is currently being used in multiple places. But, yeah,
I think we can define multiple macros of the same kind and adjust to the
characteristics of each the component.

How big do you think tfm can get?


I have no idea, I guess you'll have to take that with Herbert.

johannes



I see. I'll contact him then.

Thanks for the feedback.
--
Gustavo

Re: [PATCH] mac80211: aes-cmac: remove VLA usage

[Gustavo A. R. Silva]

On 03/21/2018 08:48 AM, Johannes Berg wrote:

On Wed, 2018-03-21 at 08:42 -0500, Gustavo A. R. Silva wrote:

In preparation to enabling -Wvla, remove VLAs and replace them
with dynamic memory allocation instead.

The use of stack Variable Length Arrays needs to be avoided, as they
can be a vector for stack exhaustion, which can be both a runtime bug
or a security flaw. Also, in general, as code evolves it is easy to
lose track of how big a VLA can get. Thus, we can end up having runtime
failures that are hard to debug.

Also, fixed as part of the directive to remove all VLAs from
the kernel: https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
  net/mac80211/aes_cmac.c | 36 
  1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/net/mac80211/aes_cmac.c b/net/mac80211/aes_cmac.c
index 2fb6558..c9444bf 100644
--- a/net/mac80211/aes_cmac.c
+++ b/net/mac80211/aes_cmac.c
@@ -27,30 +27,42 @@ static const u8 zero[CMAC_TLEN_256];
  void ieee80211_aes_cmac(struct crypto_shash *tfm, const u8 *aad,
const u8 *data, size_t data_len, u8 *mic)
  {
-   SHASH_DESC_ON_STACK(desc, tfm);
+   struct shash_desc *shash;
u8 out[AES_BLOCK_SIZE];
  
-	desc->tfm = tfm;

+   shash = kmalloc(sizeof(*shash) + crypto_shash_descsize(tfm),
+   GFP_KERNEL);
+   if (!shash)
+   return;


Honestly, this seems like a really bad idea - you're now hitting
kmalloc for every TX/RX frame here.

SHA_DESC_ON_STACK() should just be fixed to not need a VLA, but take
some sort of maximum, I guess?



SHA_DESC_ON_STACK is currently being used in multiple places. But, yeah, 
I think we can define multiple macros of the same kind and adjust to the 
characteristics of each the component.


How big do you think tfm can get?

Thanks
--
Gustavo

Re: [PATCH] netfilter: nfnetlink_cthelper: Remove VLA usage

[Gustavo A. R. Silva]

On 03/20/2018 07:36 AM, Pablo Neira Ayuso wrote:

On Mon, Mar 12, 2018 at 07:21:38PM -0500, Gustavo A. R. Silva wrote:

In preparation to enabling -Wvla, remove VLA and replace it
with dynamic memory allocation.

 From a security viewpoint, the use of Variable Length Arrays can be
a vector for stack overflow attacks. Also, in general, as the code
evolves it is easy to lose track of how big a VLA can get. Thus, we
can end up having segfaults that are hard to debug.

Also, fixed as part of the directive to remove all VLAs from
the kernel: https://lkml.org/lkml/2018/3/7/621


also applied, thanks.



Awesome.

Thanks, Pablo.
--
Gustavo

[PATCH] mac80211: aes-cmac: remove VLA usage

[Gustavo A. R. Silva]

In preparation to enabling -Wvla, remove VLAs and replace them
with dynamic memory allocation instead.

The use of stack Variable Length Arrays needs to be avoided, as they
can be a vector for stack exhaustion, which can be both a runtime bug
or a security flaw. Also, in general, as code evolves it is easy to
lose track of how big a VLA can get. Thus, we can end up having runtime
failures that are hard to debug.

Also, fixed as part of the directive to remove all VLAs from
the kernel: https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 net/mac80211/aes_cmac.c | 36 
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/net/mac80211/aes_cmac.c b/net/mac80211/aes_cmac.c
index 2fb6558..c9444bf 100644
--- a/net/mac80211/aes_cmac.c
+++ b/net/mac80211/aes_cmac.c
@@ -27,30 +27,42 @@ static const u8 zero[CMAC_TLEN_256];
 void ieee80211_aes_cmac(struct crypto_shash *tfm, const u8 *aad,
const u8 *data, size_t data_len, u8 *mic)
 {
-   SHASH_DESC_ON_STACK(desc, tfm);
+   struct shash_desc *shash;
u8 out[AES_BLOCK_SIZE];
 
-   desc->tfm = tfm;
+   shash = kmalloc(sizeof(*shash) + crypto_shash_descsize(tfm),
+   GFP_KERNEL);
+   if (!shash)
+   return;
 
-   crypto_shash_init(desc);
-   crypto_shash_update(desc, aad, AAD_LEN);
-   crypto_shash_update(desc, data, data_len - CMAC_TLEN);
-   crypto_shash_finup(desc, zero, CMAC_TLEN, out);
+   shash->tfm = tfm;
+
+   crypto_shash_init(shash);
+   crypto_shash_update(shash, aad, AAD_LEN);
+   crypto_shash_update(shash, data, data_len - CMAC_TLEN);
+   crypto_shash_finup(shash, zero, CMAC_TLEN, out);
 
memcpy(mic, out, CMAC_TLEN);
+   kfree(shash);
 }
 
 void ieee80211_aes_cmac_256(struct crypto_shash *tfm, const u8 *aad,
const u8 *data, size_t data_len, u8 *mic)
 {
-   SHASH_DESC_ON_STACK(desc, tfm);
+   struct shash_desc *shash;
+
+   shash = kmalloc(sizeof(*shash) + crypto_shash_descsize(tfm),
+   GFP_KERNEL);
+   if (!shash)
+   return;
 
-   desc->tfm = tfm;
+   shash->tfm = tfm;
 
-   crypto_shash_init(desc);
-   crypto_shash_update(desc, aad, AAD_LEN);
-   crypto_shash_update(desc, data, data_len - CMAC_TLEN_256);
-   crypto_shash_finup(desc, zero, CMAC_TLEN_256, mic);
+   crypto_shash_init(shash);
+   crypto_shash_update(shash, aad, AAD_LEN);
+   crypto_shash_update(shash, data, data_len - CMAC_TLEN_256);
+   crypto_shash_finup(shash, zero, CMAC_TLEN_256, mic);
+   kfree(shash);
 }
 
 struct crypto_shash *ieee80211_aes_cmac_key_setup(const u8 key[],
-- 
2.7.4

Re: [PATCH] Bluetooth: Remove VLA usage in aes_cmac

[Gustavo A. R. Silva]

Hi,

I've just discovered an issue in this patch. Please, drop it. I'll send 
v2 shortly.


Thanks
--
Gustavo

On 03/20/2018 06:34 PM, Gustavo A. R. Silva wrote:

In preparation to enabling -Wvla, remove VLA and replace it
with dynamic memory allocation instead.

The use of stack Variable Length Arrays needs to be avoided, as they
can be a vector for stack exhaustion, which can be both a runtime bug
or a security flaw. Also, in general, as code evolves it is easy to
lose track of how big a VLA can get. Thus, we can end up having runtime
failures that are hard to debug.

Also, fixed as part of the directive to remove all VLAs from
the kernel: https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Gustavo A. R. Silva <garsi...@embeddedor.com>
---
  net/bluetooth/smp.c | 16 +++-
  1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index a2ddae2..23c694d 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -173,7 +173,7 @@ static int aes_cmac(struct crypto_shash *tfm, const u8 
k[16], const u8 *m,
size_t len, u8 mac[16])
  {
uint8_t tmp[16], mac_msb[16], msg_msb[CMAC_MSG_MAX];
-   SHASH_DESC_ON_STACK(desc, tfm);
+   struct shash_desc *shash;
int err;
  
  	if (len > CMAC_MSG_MAX)

@@ -184,8 +184,13 @@ static int aes_cmac(struct crypto_shash *tfm, const u8 
k[16], const u8 *m,
return -EINVAL;
}
  
-	desc->tfm = tfm;

-   desc->flags = 0;
+   shash = kzalloc(sizeof(*shash) + crypto_shash_descsize(tfm),
+   GFP_KERNEL);
+   if (!shash)
+   return -ENOMEM;
+
+   shash->tfm = tfm;
+   shash->flags = 0;
  
  	/* Swap key and message from LSB to MSB */

swap_buf(k, tmp, 16);
@@ -200,8 +205,9 @@ static int aes_cmac(struct crypto_shash *tfm, const u8 
k[16], const u8 *m,
return err;
}
  
-	err = crypto_shash_digest(desc, msg_msb, len, mac_msb);

-   shash_desc_zero(desc);
+   err = crypto_shash_digest(shash, msg_msb, len, mac_msb);
+   shash_desc_zero(shash);
+   kfree(shash);
if (err) {
BT_ERR("Hash computation error %d", err);
return err;

[PATCH v2] Bluetooth: Remove VLA usage in aes_cmac

[Gustavo A. R. Silva]

In preparation to enabling -Wvla, remove VLA and replace it
with dynamic memory allocation instead.

The use of stack Variable Length Arrays needs to be avoided, as they
can be a vector for stack exhaustion, which can be both a runtime bug
or a security flaw. Also, in general, as code evolves it is easy to
lose track of how big a VLA can get. Thus, we can end up having runtime
failures that are hard to debug.

Also, fixed as part of the directive to remove all VLAs from
the kernel: https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Gustavo A. R. Silva <garsi...@embeddedor.com>
---
Changes in v2:
 - Fix memory leak in previous patch.

 net/bluetooth/smp.c | 17 -
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index a2ddae2..0fa7035 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -173,7 +173,7 @@ static int aes_cmac(struct crypto_shash *tfm, const u8 
k[16], const u8 *m,
size_t len, u8 mac[16])
 {
uint8_t tmp[16], mac_msb[16], msg_msb[CMAC_MSG_MAX];
-   SHASH_DESC_ON_STACK(desc, tfm);
+   struct shash_desc *shash;
int err;
 
if (len > CMAC_MSG_MAX)
@@ -184,8 +184,13 @@ static int aes_cmac(struct crypto_shash *tfm, const u8 
k[16], const u8 *m,
return -EINVAL;
}
 
-   desc->tfm = tfm;
-   desc->flags = 0;
+   shash = kzalloc(sizeof(*shash) + crypto_shash_descsize(tfm),
+   GFP_KERNEL);
+   if (!shash)
+   return -ENOMEM;
+
+   shash->tfm = tfm;
+   shash->flags = 0;
 
/* Swap key and message from LSB to MSB */
swap_buf(k, tmp, 16);
@@ -197,11 +202,13 @@ static int aes_cmac(struct crypto_shash *tfm, const u8 
k[16], const u8 *m,
err = crypto_shash_setkey(tfm, tmp, 16);
if (err) {
BT_ERR("cipher setkey failed: %d", err);
+   kfree(shash);
return err;
}
 
-   err = crypto_shash_digest(desc, msg_msb, len, mac_msb);
-   shash_desc_zero(desc);
+   err = crypto_shash_digest(shash, msg_msb, len, mac_msb);
+   shash_desc_zero(shash);
+   kfree(shash);
if (err) {
BT_ERR("Hash computation error %d", err);
return err;
-- 
2.7.4

[PATCH] Bluetooth: Remove VLA usage in aes_cmac

[Gustavo A. R. Silva]

In preparation to enabling -Wvla, remove VLA and replace it
with dynamic memory allocation instead.

The use of stack Variable Length Arrays needs to be avoided, as they
can be a vector for stack exhaustion, which can be both a runtime bug
or a security flaw. Also, in general, as code evolves it is easy to
lose track of how big a VLA can get. Thus, we can end up having runtime
failures that are hard to debug.

Also, fixed as part of the directive to remove all VLAs from
the kernel: https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Gustavo A. R. Silva <garsi...@embeddedor.com>
---
 net/bluetooth/smp.c | 16 +++-
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index a2ddae2..23c694d 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -173,7 +173,7 @@ static int aes_cmac(struct crypto_shash *tfm, const u8 
k[16], const u8 *m,
size_t len, u8 mac[16])
 {
uint8_t tmp[16], mac_msb[16], msg_msb[CMAC_MSG_MAX];
-   SHASH_DESC_ON_STACK(desc, tfm);
+   struct shash_desc *shash;
int err;
 
if (len > CMAC_MSG_MAX)
@@ -184,8 +184,13 @@ static int aes_cmac(struct crypto_shash *tfm, const u8 
k[16], const u8 *m,
return -EINVAL;
}
 
-   desc->tfm = tfm;
-   desc->flags = 0;
+   shash = kzalloc(sizeof(*shash) + crypto_shash_descsize(tfm),
+   GFP_KERNEL);
+   if (!shash)
+   return -ENOMEM;
+
+   shash->tfm = tfm;
+   shash->flags = 0;
 
/* Swap key and message from LSB to MSB */
swap_buf(k, tmp, 16);
@@ -200,8 +205,9 @@ static int aes_cmac(struct crypto_shash *tfm, const u8 
k[16], const u8 *m,
return err;
}
 
-   err = crypto_shash_digest(desc, msg_msb, len, mac_msb);
-   shash_desc_zero(desc);
+   err = crypto_shash_digest(shash, msg_msb, len, mac_msb);
+   shash_desc_zero(shash);
+   kfree(shash);
if (err) {
BT_ERR("Hash computation error %d", err);
return err;
-- 
2.7.4

Re: [PATCH] pktgen: use dynamic allocation for debug print buffer

[Gustavo A. R. Silva]

Arnd:

Thanks for the fix.

On 03/13/2018 10:02 PM, Wang Jian wrote:

+  kfree(buf);

free tb? buf is an array.



Wang:

Thanks for the report. I already sent a patch to fix this: 
https://patchwork.kernel.org/patch/10281587/


--
Gustavo


On Wed, Mar 14, 2018 at 8:25 AM, David Miller  wrote:

From: Arnd Bergmann 
Date: Tue, 13 Mar 2018 21:58:39 +0100


After the removal of the VLA, we get a harmless warning about a large
stack frame:

net/core/pktgen.c: In function 'pktgen_if_write':
net/core/pktgen.c:1710:1: error: the frame size of 1076 bytes is larger than 
1024 bytes [-Werror=frame-larger-than=]

The function was previously shown to be safe despite hitting
the 1024 bye warning level. To get rid of the annoyging warning,
while keeping it readable, this changes it to use strndup_user().

Obviously this is not a fast path, so the kmalloc() overhead
can be disregarded.

Fixes: 35951393bbff ("pktgen: Remove VLA usage")
Signed-off-by: Arnd Bergmann 


Applied, thanks.

[PATCH] pktgen: Fix memory leak in pktgen_if_write

[Gustavo A. R. Silva]

_buf_ is an array and the one that must be freed is _tp_ instead.

Fixes: a870a02cc963 ("pktgen: use dynamic allocation for debug print buffer")
Reported-by: Wang Jian <jianjian.wa...@gmail.com>
Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 net/core/pktgen.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index fd65761..545cf08 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -913,7 +913,7 @@ static ssize_t pktgen_if_write(struct file *file,
return PTR_ERR(tp);
 
pr_debug("%s,%zu  buffer -:%s:-\n", name, count, tp);
-   kfree(buf);
+   kfree(tp);
}
 
if (!strcmp(name, "min_pkt_size")) {
-- 
2.7.4

Re: linux-next: build warning after merge of the net-next tree

[Gustavo A. R. Silva]

On 03/13/2018 10:33 AM, David Miller wrote:

From: "Gustavo A. R. Silva" <gust...@embeddedor.com>
Date: Tue, 13 Mar 2018 06:46:24 -0500


Hi Stephen,

On 03/13/2018 01:11 AM, Stephen Rothwell wrote:

Hi all,
After merging the net-next tree, today's linux-next build (sparc
defconfig) produced this warning:
net/core/pktgen.c: In function 'pktgen_if_write':
net/core/pktgen.c:1710:1: warning: the frame size of 1048 bytes is
larger than 1024 bytes [-Wframe-larger-than=]
   }
   ^
Introduced by commit
35951393bbff ("pktgen: Remove VLA usage")



Thanks for the report.

David:

If this code is not going to be executed very often [1], then I think
it is safe to use dynamic memory allocation instead, as this is not
going to impact the performance.

What do you think?

[1] https://lkml.org/lkml/2018/3/9/630


Sure, that works.

It is only invoked when pktgen configuration changes are made.



OK. I'll send a new patch for this.

Thanks
--
Gustavo

Re: linux-next: build warning after merge of the net-next tree

[Gustavo A. R. Silva]

Hi Stephen,

On 03/13/2018 01:11 AM, Stephen Rothwell wrote:

Hi all,

After merging the net-next tree, today's linux-next build (sparc
defconfig) produced this warning:

net/core/pktgen.c: In function 'pktgen_if_write':
net/core/pktgen.c:1710:1: warning: the frame size of 1048 bytes is larger than 
1024 bytes [-Wframe-larger-than=]
  }
  ^

Introduced by commit

   35951393bbff ("pktgen: Remove VLA usage")



Thanks for the report.

David:

If this code is not going to be executed very often [1], then I think it 
is safe to use dynamic memory allocation instead, as this is not going 
to impact the performance.


What do you think?

[1] https://lkml.org/lkml/2018/3/9/630

Thanks
--
Gustavo

[PATCH v2] netfilter: nf_tables: remove VLA usage

[Gustavo A. R. Silva]

In preparation to enabling -Wvla, remove VLA and replace it
with dynamic memory allocation.

>From a security viewpoint, the use of Variable Length Arrays can be
a vector for stack overflow attacks. Also, in general, as the code
evolves it is easy to lose track of how big a VLA can get. Thus, we
can end up having segfaults that are hard to debug.

Also, fixed as part of the directive to remove all VLAs from
the kernel: https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
Changes in v2:
 - Use kmalloc_array instead of kcalloc.

 net/netfilter/nf_tables_api.c | 23 +++
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 3f815b6..5a42e97 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -4357,16 +4357,20 @@ static struct nft_object *nft_obj_init(const struct 
nft_ctx *ctx,
   const struct nft_object_type *type,
   const struct nlattr *attr)
 {
-   struct nlattr *tb[type->maxattr + 1];
+   struct nlattr **tb;
const struct nft_object_ops *ops;
struct nft_object *obj;
-   int err;
+   int err = -ENOMEM;
+
+   tb = kmalloc_array(type->maxattr + 1, sizeof(*tb), GFP_KERNEL);
+   if (!tb)
+   goto err1;
 
if (attr) {
err = nla_parse_nested(tb, type->maxattr, attr, type->policy,
   NULL);
if (err < 0)
-   goto err1;
+   goto err2;
} else {
memset(tb, 0, sizeof(tb[0]) * (type->maxattr + 1));
}
@@ -4375,7 +4379,7 @@ static struct nft_object *nft_obj_init(const struct 
nft_ctx *ctx,
ops = type->select_ops(ctx, (const struct nlattr * const *)tb);
if (IS_ERR(ops)) {
err = PTR_ERR(ops);
-   goto err1;
+   goto err2;
}
} else {
ops = type->ops;
@@ -4383,18 +4387,21 @@ static struct nft_object *nft_obj_init(const struct 
nft_ctx *ctx,
 
err = -ENOMEM;
obj = kzalloc(sizeof(*obj) + ops->size, GFP_KERNEL);
-   if (obj == NULL)
-   goto err1;
+   if (!obj)
+   goto err2;
 
err = ops->init(ctx, (const struct nlattr * const *)tb, obj);
if (err < 0)
-   goto err2;
+   goto err3;
 
obj->ops = ops;
 
+   kfree(tb);
return obj;
-err2:
+err3:
kfree(obj);
+err2:
+   kfree(tb);
 err1:
return ERR_PTR(err);
 }
-- 
2.7.4

[PATCH] netfilter: nf_tables: remove VLA usage

[Gustavo A. R. Silva]

In preparation to enabling -Wvla, remove VLA and replace it
with dynamic memory allocation.

>From a security viewpoint, the use of Variable Length Arrays can be
a vector for stack overflow attacks. Also, in general, as the code
evolves it is easy to lose track of how big a VLA can get. Thus, we
can end up having segfaults that are hard to debug.

Also, fixed as part of the directive to remove all VLAs from
the kernel: https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 net/netfilter/nf_tables_api.c | 23 +++
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 3f815b6..ea76903 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -4357,16 +4357,20 @@ static struct nft_object *nft_obj_init(const struct 
nft_ctx *ctx,
   const struct nft_object_type *type,
   const struct nlattr *attr)
 {
-   struct nlattr *tb[type->maxattr + 1];
+   struct nlattr **tb;
const struct nft_object_ops *ops;
struct nft_object *obj;
-   int err;
+   int err = -ENOMEM;
+
+   tb = kcalloc(type->maxattr + 1, sizeof(*tb), GFP_KERNEL);
+   if (!tb)
+   goto err1;
 
if (attr) {
err = nla_parse_nested(tb, type->maxattr, attr, type->policy,
   NULL);
if (err < 0)
-   goto err1;
+   goto err2;
} else {
memset(tb, 0, sizeof(tb[0]) * (type->maxattr + 1));
}
@@ -4375,7 +4379,7 @@ static struct nft_object *nft_obj_init(const struct 
nft_ctx *ctx,
ops = type->select_ops(ctx, (const struct nlattr * const *)tb);
if (IS_ERR(ops)) {
err = PTR_ERR(ops);
-   goto err1;
+   goto err2;
}
} else {
ops = type->ops;
@@ -4383,18 +4387,21 @@ static struct nft_object *nft_obj_init(const struct 
nft_ctx *ctx,
 
err = -ENOMEM;
obj = kzalloc(sizeof(*obj) + ops->size, GFP_KERNEL);
-   if (obj == NULL)
-   goto err1;
+   if (!obj)
+   goto err2;
 
err = ops->init(ctx, (const struct nlattr * const *)tb, obj);
if (err < 0)
-   goto err2;
+   goto err3;
 
obj->ops = ops;
 
+   kfree(tb);
return obj;
-err2:
+err3:
kfree(obj);
+err2:
+   kfree(tb);
 err1:
return ERR_PTR(err);
 }
-- 
2.7.4

[PATCH] netfilter: nfnetlink_cthelper: Remove VLA usage

[Gustavo A. R. Silva]

In preparation to enabling -Wvla, remove VLA and replace it
with dynamic memory allocation.

>From a security viewpoint, the use of Variable Length Arrays can be
a vector for stack overflow attacks. Also, in general, as the code
evolves it is easy to lose track of how big a VLA can get. Thus, we
can end up having segfaults that are hard to debug.

Also, fixed as part of the directive to remove all VLAs from
the kernel: https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 net/netfilter/nfnetlink_cthelper.c | 25 +
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/net/netfilter/nfnetlink_cthelper.c 
b/net/netfilter/nfnetlink_cthelper.c
index d33ce6d..4a4b293 100644
--- a/net/netfilter/nfnetlink_cthelper.c
+++ b/net/netfilter/nfnetlink_cthelper.c
@@ -314,23 +314,30 @@ nfnl_cthelper_update_policy_one(const struct 
nf_conntrack_expect_policy *policy,
 static int nfnl_cthelper_update_policy_all(struct nlattr *tb[],
   struct nf_conntrack_helper *helper)
 {
-   struct nf_conntrack_expect_policy new_policy[helper->expect_class_max + 
1];
+   struct nf_conntrack_expect_policy *new_policy;
struct nf_conntrack_expect_policy *policy;
-   int i, err;
+   int i, ret = 0;
+
+   new_policy = kmalloc_array(helper->expect_class_max + 1,
+  sizeof(*new_policy), GFP_KERNEL);
+   if (!new_policy)
+   return -ENOMEM;
 
/* Check first that all policy attributes are well-formed, so we don't
 * leave things in inconsistent state on errors.
 */
for (i = 0; i < helper->expect_class_max + 1; i++) {
 
-   if (!tb[NFCTH_POLICY_SET + i])
-   return -EINVAL;
+   if (!tb[NFCTH_POLICY_SET + i]) {
+   ret = -EINVAL;
+   goto err;
+   }
 
-   err = nfnl_cthelper_update_policy_one(>expect_policy[i],
+   ret = nfnl_cthelper_update_policy_one(>expect_policy[i],
  _policy[i],
  tb[NFCTH_POLICY_SET + i]);
-   if (err < 0)
-   return err;
+   if (ret < 0)
+   goto err;
}
/* Now we can safely update them. */
for (i = 0; i < helper->expect_class_max + 1; i++) {
@@ -340,7 +347,9 @@ static int nfnl_cthelper_update_policy_all(struct nlattr 
*tb[],
policy->timeout = new_policy->timeout;
}
 
-   return 0;
+err:
+   kfree(new_policy);
+   return ret;
 }
 
 static int nfnl_cthelper_update_policy(struct nf_conntrack_helper *helper,
-- 
2.7.4

[PATCH] netfilter: cttimeout: remove VLA usage

[Gustavo A. R. Silva]

In preparation to enabling -Wvla, remove VLA and replace it
with dynamic memory allocation.

>From a security viewpoint, the use of Variable Length Arrays can be
a vector for stack overflow attacks. Also, in general, as the code
evolves it is easy to lose track of how big a VLA can get. Thus, we
can end up having segfaults that are hard to debug.

Also, fixed as part of the directive to remove all VLAs from
the kernel: https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 net/netfilter/nfnetlink_cttimeout.c | 26 +-
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/net/netfilter/nfnetlink_cttimeout.c 
b/net/netfilter/nfnetlink_cttimeout.c
index 6819300..dcd7bd3 100644
--- a/net/netfilter/nfnetlink_cttimeout.c
+++ b/net/netfilter/nfnetlink_cttimeout.c
@@ -51,19 +51,27 @@ ctnl_timeout_parse_policy(void *timeouts,
  const struct nf_conntrack_l4proto *l4proto,
  struct net *net, const struct nlattr *attr)
 {
+   struct nlattr **tb;
int ret = 0;
 
-   if (likely(l4proto->ctnl_timeout.nlattr_to_obj)) {
-   struct nlattr *tb[l4proto->ctnl_timeout.nlattr_max+1];
+   if (!l4proto->ctnl_timeout.nlattr_to_obj)
+   return 0;
 
-   ret = nla_parse_nested(tb, l4proto->ctnl_timeout.nlattr_max,
-  attr, l4proto->ctnl_timeout.nla_policy,
-  NULL);
-   if (ret < 0)
-   return ret;
+   tb = kcalloc(l4proto->ctnl_timeout.nlattr_max + 1, sizeof(*tb),
+GFP_KERNEL);
 
-   ret = l4proto->ctnl_timeout.nlattr_to_obj(tb, net, timeouts);
-   }
+   if (!tb)
+   return -ENOMEM;
+
+   ret = nla_parse_nested(tb, l4proto->ctnl_timeout.nlattr_max, attr,
+  l4proto->ctnl_timeout.nla_policy, NULL);
+   if (ret < 0)
+   goto err;
+
+   ret = l4proto->ctnl_timeout.nlattr_to_obj(tb, net, timeouts);
+
+err:
+   kfree(tb);
return ret;
 }
 
-- 
2.7.4

Re: [RFC] netfilter: cttimeout: remove VLA in ctnl_timeout_parse_policy

[Gustavo A. R. Silva]

On 03/11/2018 05:21 PM, Pablo Neira Ayuso wrote:

On Sun, Mar 11, 2018 at 05:12:09PM -0500, Gustavo A. R. Silva wrote:

Hi Pablo,

On 03/11/2018 05:04 PM, Pablo Neira Ayuso wrote:

On Tue, Mar 06, 2018 at 12:47:55PM -0600, Gustavo A. R. Silva wrote:

In preparation to enabling -Wvla, remove VLA and replace it
with dynamic memory allocation.


Looks good but...


Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
   net/netfilter/nfnetlink_cttimeout.c | 12 ++--
   1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nfnetlink_cttimeout.c 
b/net/netfilter/nfnetlink_cttimeout.c
index 95b0470..a2f7d92 100644
--- a/net/netfilter/nfnetlink_cttimeout.c
+++ b/net/netfilter/nfnetlink_cttimeout.c
@@ -52,18 +52,26 @@ ctnl_timeout_parse_policy(void *timeouts,
  struct net *net, const struct nlattr *attr)
   {
int ret = 0;
+   struct nlattr **tb = NULL;


I think we don't need to initialize this, right?



We actually do have to initialized it because in the unlikely case that the
code block inside the 'if' below is not executed, then we will end up
freeing an uninitialized pointer.


I see, you're right indeed.

We can probably simplify this code, but just doing:

 if (!l4proto->ctnl_timeout.nlattr_to_obj))
 return 0;



I wonder if it is better to code this instead:

if (unlikely(!l4proto->ctnl_timeout.nlattr_to_obj)))
return 0;



 netlink attribute parsing here.

You could even remove the likely() thing, which doesn't make much
sense for control plane code.



Why is that?


I understand this is a larger change, but I think this function will
look better while we're removing VLA.

Would you mind having a look? I'd appreciate if so.



I can do that. No problem.

Thanks
--
Gustavo

Re: [RFC] netfilter: cttimeout: remove VLA in ctnl_timeout_parse_policy

[Gustavo A. R. Silva]

Hi Pablo,

On 03/11/2018 05:04 PM, Pablo Neira Ayuso wrote:

On Tue, Mar 06, 2018 at 12:47:55PM -0600, Gustavo A. R. Silva wrote:

In preparation to enabling -Wvla, remove VLA and replace it
with dynamic memory allocation.


Looks good but...


Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
  net/netfilter/nfnetlink_cttimeout.c | 12 ++--
  1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nfnetlink_cttimeout.c 
b/net/netfilter/nfnetlink_cttimeout.c
index 95b0470..a2f7d92 100644
--- a/net/netfilter/nfnetlink_cttimeout.c
+++ b/net/netfilter/nfnetlink_cttimeout.c
@@ -52,18 +52,26 @@ ctnl_timeout_parse_policy(void *timeouts,
  struct net *net, const struct nlattr *attr)
  {
int ret = 0;
+   struct nlattr **tb = NULL;


I think we don't need to initialize this, right?



We actually do have to initialized it because in the unlikely case that 
the code block inside the 'if' below is not executed, then we will end 
up freeing an uninitialized pointer.


Thanks
--
Gustavo

  
  	if (likely(l4proto->ctnl_timeout.nlattr_to_obj)) {

-   struct nlattr *tb[l4proto->ctnl_timeout.nlattr_max+1];
+   tb = kcalloc(l4proto->ctnl_timeout.nlattr_max + 1, sizeof(*tb),
+GFP_KERNEL);
+
+   if (!tb)
+   return -ENOMEM;
  
  		ret = nla_parse_nested(tb, l4proto->ctnl_timeout.nlattr_max,

   attr, l4proto->ctnl_timeout.nla_policy,
   NULL);
if (ret < 0)
-   return ret;
+   goto err;
  
  		ret = l4proto->ctnl_timeout.nlattr_to_obj(tb, net, timeouts);

}
+
+err:
+   kfree(tb);
return ret;
  }
  
--

2.7.4

Re: [PATCH] drivers: net: wireless: ath: ath9: dfs: remove VLA usage

[Gustavo A. R. Silva]

On 03/10/2018 05:12 PM, Kees Cook wrote:

On Sat, Mar 10, 2018 at 3:06 PM, Arend van Spriel
 wrote:

On 3/9/2018 1:30 PM, Andreas Christoforou wrote:


The kernel would like to have all stack VLA usage removed.



I think there was a remark made earlier to give more explanation here. It
should explain why we want "VLA on stack" removed.


Signed-off-by: Andreas Christoforou 
---
   drivers/net/wireless/ath/ath9k/dfs.c | 3 +--
   1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/dfs.c
b/drivers/net/wireless/ath/ath9k/dfs.c
index 6fee9a4..cfb0f84 100644
--- a/drivers/net/wireless/ath/ath9k/dfs.c
+++ b/drivers/net/wireless/ath/ath9k/dfs.c
@@ -41,7 +41,6 @@ static const int BIN_DELTA_MAX= 10;

   /* we need at least 3 deltas / 4 samples for a reliable chirp detection
*/
   #define NUM_DIFFS 3
-static const int FFT_NUM_SAMPLES   = (NUM_DIFFS + 1);



What about this instead?

#define FFT_NUM_SAMPLES (NUM_DIFFS + 1)


   /* Threshold for difference of delta peaks */
   static const int MAX_DIFF = 2;
@@ -101,7 +100,7 @@ static bool ath9k_check_chirping(struct ath_softc *sc,
u8 *data,
  int datalen, bool is_ctl, bool is_ext)
   {
 int i;
-   int max_bin[FFT_NUM_SAMPLES];
+   int max_bin[NUM_DIFFS + 1];



Just wondering. Is this actually a VLA. FFT_NUM_SAMPLES was static const so
not really going to show a lot of variation. This array will always have the
same size on the stack.


The problem is that it's not a "constant expression", so the compiler
frontend still yells about it under -Wvla. I would characterize this
mainly as a fix for "accidental VLA" or "misdetected VLA" or something
like that. AIUI, there really isn't a functional change here.

-Kees



--
Gustavo

Re: [PATCH] pktgen: Remove VLA usage

[Gustavo A. R. Silva]

On 03/09/2018 10:58 AM, David Miller wrote:

From: "Gustavo A. R. Silva" <gust...@embeddedor.com>
Date: Thu, 8 Mar 2018 23:43:40 -0600


In preparation to enabling -Wvla, remove VLA usage and replace it
with a fixed-length array instead.

Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
David,

I'm not sure how often this function is being called and,
depending on the frequency it may be worth to use
dynamic memory allocation instead?


It happens every time a config setting is made via the sysfs
files when debug is enabled.

This is not something that happens often.



I got it.


So your patch is fine, applied to net-next, thanks.



Awesome.

Thanks, David.
--
Gustavo

[PATCH] pktgen: Remove VLA usage

[Gustavo A. R. Silva]

In preparation to enabling -Wvla, remove VLA usage and replace it
with a fixed-length array instead.

Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
David,

I'm not sure how often this function is being called and,
depending on the frequency it may be worth to use
dynamic memory allocation instead?

Thanks
--
Gustavo

 net/core/pktgen.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index d81bddd..e2d6ae3 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -907,7 +907,7 @@ static ssize_t pktgen_if_write(struct file *file,
 
if (debug) {
size_t copy = min_t(size_t, count, 1023);
-   char tb[copy + 1];
+   char tb[1024];
if (copy_from_user(tb, user_buffer, copy))
return -EFAULT;
tb[copy] = 0;
-- 
2.7.4

[RFC] Removing VLA usage in l1oip_core

[Gustavo A. R. Silva]

Hi Karsten,

I'm trying to figure out the best way to fix the following VLA warning:

drivers/isdn/mISDN/l1oip_core.c:282:2: warning: ISO C90 forbids variable length 
array ‘frame’ [-Wvla]
  u8 frame[len + 32];
  ^~

So while doing some research I've found the following.

Based on this code at include/linux/mISDNhw.h:38: #define MAX_DFRAME_LEN_L1 300 
and the following at drivers/isdn/mISDN/l1oip_core.c:1115:


if (skb->len > MAX_DFRAME_LEN_L1 || skb->len > L1OIP_MAX_LEN) {
printk(KERN_WARNING "%s: skb too large\n",
   __func__);
break;
}
/* check for AIS / ulaw-silence */
l = skb->len;
if (!memchr_inv(skb->data, 0xff, l)) {
if (debug & DEBUG_L1OIP_MSG)
printk(KERN_DEBUG "%s: got AIS, not sending, "
   "but counting\n", __func__);
hc->chan[bch->slot].tx_counter += l;
skb_trim(skb, 0);
queue_ch_frame(ch, PH_DATA_CNF, hh->id, skb);
return 0;
}
/* check for silence */
l = skb->len;
if (!memchr_inv(skb->data, 0x2a, l)) {
if (debug & DEBUG_L1OIP_MSG)
printk(KERN_DEBUG "%s: got silence, not sending"
   ", but counting\n", __func__);
hc->chan[bch->slot].tx_counter += l;
skb_trim(skb, 0);
queue_ch_frame(ch, PH_DATA_CNF, hh->id, skb);
return 0;
}

/* send frame */
p = skb->data;
l = skb->len;
while (l) {
ll = (l < L1OIP_MAX_PERFRAME) ? l : L1OIP_MAX_PERFRAME;
l1oip_socket_send(hc, hc->codec, bch->slot, 0,
  hc->chan[bch->slot].tx_counter, p, 
ll);
hc->chan[bch->slot].tx_counter += ll;
p += ll;
l -= ll;
}


it seems that the maximum value 'len' can take at 
drivers/isdn/mISDN/l1oip_core.c:282 is 300


/*
 * send a frame via socket, if open and restart timer
 */
static int
l1oip_socket_send(struct l1oip *hc, u8 localcodec, u8 channel, u32 chanmask,
  u16 timebase, u8 *buf, int len)
{
u8 *p;
u8 frame[len + 32];


If this is correct, I could send the following patch to fix the VLA warning:

diff --git a/drivers/isdn/mISDN/l1oip_core.c b/drivers/isdn/mISDN/l1oip_core.c
index 21d50e4..31e3cd5 100644
--- a/drivers/isdn/mISDN/l1oip_core.c
+++ b/drivers/isdn/mISDN/l1oip_core.c
@@ -279,7 +279,7 @@ l1oip_socket_send(struct l1oip *hc, u8 localcodec, u8 
channel, u32 chanmask,
  u16 timebase, u8 *buf, int len)
 {
u8 *p;
-   u8 frame[len + 32];
+   u8 frame[332];
struct socket *socket = NULL;
 
if (debug & DEBUG_L1OIP_MSG)

But I wanted to ask for your feedback first, in case I may be missing something.

Another solution is to use dynamic memory allocation, but if the maximum size 
for 'frame' is in the hundreds of bytes, it might not be worth the performace 
penalty.

What do you think?

Thanks!
--
Gustavo

[PATCH v2] cxgb3: remove VLA usage

[Gustavo A. R. Silva]

Remove VLA usage and change the 'len' argument to a u8 and use a 256
byte buffer on the stack. Notice that these lengths are limited by the
encoding field in the VPD structure, which is a u8 [1].

[1] https://marc.info/?l=linux-netdev=152044354814024=2

Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
Changes in v2:
 - Update the code as suggested by David Miller.
 - Update changelog based on David's comments.
 - Update subject.

 drivers/net/ethernet/chelsio/cxgb3/t3_hw.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb3/t3_hw.c 
b/drivers/net/ethernet/chelsio/cxgb3/t3_hw.c
index a89721f..080918a 100644
--- a/drivers/net/ethernet/chelsio/cxgb3/t3_hw.c
+++ b/drivers/net/ethernet/chelsio/cxgb3/t3_hw.c
@@ -681,18 +681,18 @@ int t3_seeprom_wp(struct adapter *adapter, int enable)
return t3_seeprom_write(adapter, EEPROM_STAT_ADDR, enable ? 0xc : 0);
 }
 
-static int vpdstrtouint(char *s, int len, unsigned int base, unsigned int *val)
+static int vpdstrtouint(char *s, u8 len, unsigned int base, unsigned int *val)
 {
-   char tok[len + 1];
+   char tok[256];
 
memcpy(tok, s, len);
tok[len] = 0;
return kstrtouint(strim(tok), base, val);
 }
 
-static int vpdstrtou16(char *s, int len, unsigned int base, u16 *val)
+static int vpdstrtou16(char *s, u8 len, unsigned int base, u16 *val)
 {
-   char tok[len + 1];
+   char tok[256];
 
memcpy(tok, s, len);
tok[len] = 0;
-- 
2.7.4

Re: [PATCH] cxgb3: remove VLA

[Gustavo A. R. Silva]

On 03/07/2018 11:25 AM, David Miller wrote:

From: "Gustavo A. R. Silva" <gust...@embeddedor.com>
Date: Mon, 5 Mar 2018 23:39:34 -0600


In preparation to enabling -Wvla, remove VLA and replace it
with dynamic memory allocation.

Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>


All of these lengths are limited by the encoding of the field in the
VPD, which is a u8, and thus 255 bytes.

Please just change the 'len' argument to these functions to a u8 and
use a 256 byte buffer on the stack.

Thanks.




OK. I got it.

Thanks for the feedback.
--
Gustavo

[RFC] netfilter: cttimeout: remove VLA in ctnl_timeout_parse_policy

[Gustavo A. R. Silva]

In preparation to enabling -Wvla, remove VLA and replace it
with dynamic memory allocation.

Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 net/netfilter/nfnetlink_cttimeout.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nfnetlink_cttimeout.c 
b/net/netfilter/nfnetlink_cttimeout.c
index 95b0470..a2f7d92 100644
--- a/net/netfilter/nfnetlink_cttimeout.c
+++ b/net/netfilter/nfnetlink_cttimeout.c
@@ -52,18 +52,26 @@ ctnl_timeout_parse_policy(void *timeouts,
  struct net *net, const struct nlattr *attr)
 {
int ret = 0;
+   struct nlattr **tb = NULL;
 
if (likely(l4proto->ctnl_timeout.nlattr_to_obj)) {
-   struct nlattr *tb[l4proto->ctnl_timeout.nlattr_max+1];
+   tb = kcalloc(l4proto->ctnl_timeout.nlattr_max + 1, sizeof(*tb),
+GFP_KERNEL);
+
+   if (!tb)
+   return -ENOMEM;
 
ret = nla_parse_nested(tb, l4proto->ctnl_timeout.nlattr_max,
   attr, l4proto->ctnl_timeout.nla_policy,
   NULL);
if (ret < 0)
-   return ret;
+   goto err;
 
ret = l4proto->ctnl_timeout.nlattr_to_obj(tb, net, timeouts);
}
+
+err:
+   kfree(tb);
return ret;
 }
 
-- 
2.7.4

[PATCH] cxgb3: remove VLA

[Gustavo A. R. Silva]

In preparation to enabling -Wvla, remove VLA and replace it
with dynamic memory allocation.

Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 drivers/net/ethernet/chelsio/cxgb3/t3_hw.c | 25 +
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb3/t3_hw.c 
b/drivers/net/ethernet/chelsio/cxgb3/t3_hw.c
index a89721f..ad6a280 100644
--- a/drivers/net/ethernet/chelsio/cxgb3/t3_hw.c
+++ b/drivers/net/ethernet/chelsio/cxgb3/t3_hw.c
@@ -683,20 +683,37 @@ int t3_seeprom_wp(struct adapter *adapter, int enable)
 
 static int vpdstrtouint(char *s, int len, unsigned int base, unsigned int *val)
 {
-   char tok[len + 1];
+   char *tok;
+   int ret;
+
+   tok = kcalloc(len + 1, sizeof(*tok), GFP_KERNEL);
+   if (!tok)
+   return -ENOMEM;
 
memcpy(tok, s, len);
tok[len] = 0;
-   return kstrtouint(strim(tok), base, val);
+   ret = kstrtouint(strim(tok), base, val);
+
+   kfree(tok);
+   return ret;
 }
 
 static int vpdstrtou16(char *s, int len, unsigned int base, u16 *val)
 {
-   char tok[len + 1];
+   char *tok;
+   int ret;
+
+   tok = kcalloc(len + 1, sizeof(*tok), GFP_KERNEL);
+   if (!tok)
+   return -ENOMEM;
 
memcpy(tok, s, len);
tok[len] = 0;
-   return kstrtou16(strim(tok), base, val);
+
+   ret = kstrtou16(strim(tok), base, val);
+
+   kfree(tok);
+   return ret;
 }
 
 /**
-- 
2.7.4

[PATCH] caif_dev: use true and false for boolean values

[Gustavo A. R. Silva]

Assign true or false to boolean variables instead of an integer value.

This issue was detected with the help of Coccinelle.

Signed-off-by: Gustavo A. R. Silva <garsi...@embeddedor.com>
---
 net/caif/caif_dev.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/caif/caif_dev.c b/net/caif/caif_dev.c
index e0adcd1..f2848d6 100644
--- a/net/caif/caif_dev.c
+++ b/net/caif/caif_dev.c
@@ -139,7 +139,7 @@ static void caif_flow_cb(struct sk_buff *skb)
 
spin_lock_bh(>flow_lock);
send_xoff = caifd->xoff;
-   caifd->xoff = 0;
+   caifd->xoff = false;
dtor = caifd->xoff_skb_dtor;
 
if (WARN_ON(caifd->xoff_skb != skb))
@@ -213,7 +213,7 @@ static int transmit(struct cflayer *layer, struct cfpkt 
*pkt)
pr_debug("queue has stopped(%d) or is full (%d > %d)\n",
netif_queue_stopped(caifd->netdev),
qlen, high);
-   caifd->xoff = 1;
+   caifd->xoff = true;
caifd->xoff_skb = skb;
caifd->xoff_skb_dtor = skb->destructor;
skb->destructor = caif_flow_cb;
@@ -400,7 +400,7 @@ static int caif_device_notify(struct notifier_block *me, 
unsigned long what,
break;
}
 
-   caifd->xoff = 0;
+   caifd->xoff = false;
cfcnfg_set_phy_state(cfg, >layer, true);
rcu_read_unlock();
 
@@ -435,7 +435,7 @@ static int caif_device_notify(struct notifier_block *me, 
unsigned long what,
if (caifd->xoff_skb_dtor != NULL && caifd->xoff_skb != NULL)
caifd->xoff_skb->destructor = caifd->xoff_skb_dtor;
 
-   caifd->xoff = 0;
+   caifd->xoff = false;
caifd->xoff_skb_dtor = NULL;
caifd->xoff_skb = NULL;
 
-- 
2.7.4

Re: [PATCH] netfilter: ipt_ah: return boolean instead of integer

[Gustavo A. R. Silva]

On 03/05/2018 04:10 PM, Pablo Neira Ayuso wrote:

On Tue, Feb 13, 2018 at 08:25:57AM -0600, Gustavo A. R. Silva wrote:

Return statements in functions returning bool should use
true/false instead of 1/0.

This issue was detected with the help of Coccinelle.

This one didn't get in time for the previous merge window.

Now applied, thanks.

Great.
Thanks, Pablo.
--
Gustavo

[PATCH] tipc: bcast: use true and false for boolean values

[Gustavo A. R. Silva]

Assign true or false to boolean variables instead of an integer value.

This issue was detected with the help of Coccinelle.

Signed-off-by: Gustavo A. R. Silva <garsi...@embeddedor.com>
---
 net/tipc/bcast.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c
index 37892b3..f371117 100644
--- a/net/tipc/bcast.c
+++ b/net/tipc/bcast.c
@@ -574,5 +574,5 @@ void tipc_nlist_purge(struct tipc_nlist *nl)
 {
tipc_dest_list_purge(>list);
nl->remote = 0;
-   nl->local = 0;
+   nl->local = false;
 }
-- 
2.7.4

[PATCH] xfrm_policy: use true and false for boolean values

[Gustavo A. R. Silva]

Assign true or false to boolean variables instead of an integer value.

This issue was detected with the help of Coccinelle.

Signed-off-by: Gustavo A. R. Silva <garsi...@embeddedor.com>
---
 net/xfrm/xfrm_policy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index eb88a7d..8a0ac6a 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -1743,7 +1743,7 @@ static void xfrm_pcpu_work_fn(struct work_struct *work)
 void xfrm_policy_cache_flush(void)
 {
struct xfrm_dst *old;
-   bool found = 0;
+   bool found = false;
int cpu;
 
might_sleep();
-- 
2.7.4

[PATCH] ipv6: ndisc: use true and false for boolean values

[Gustavo A. R. Silva]

Assign true or false to boolean variables instead of an integer value.

This issue was detected with the help of Coccinelle.

Signed-off-by: Gustavo A. R. Silva <garsi...@embeddedor.com>
---
 net/ipv6/ndisc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index 0a19ce3..8af5eef 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -527,7 +527,7 @@ void ndisc_send_na(struct net_device *dev, const struct 
in6_addr *daddr,
}
 
if (!dev->addr_len)
-   inc_opt = 0;
+   inc_opt = false;
if (inc_opt)
optlen += ndisc_opt_addr_space(dev,
   NDISC_NEIGHBOUR_ADVERTISEMENT);
-- 
2.7.4

[PATCH] ipvs: use true and false for boolean values

[Gustavo A. R. Silva]

Assign true or false to boolean variables instead of an integer value.

This issue was detected with the help of Coccinelle.

Signed-off-by: Gustavo A. R. Silva <garsi...@embeddedor.com>
---
 net/netfilter/ipvs/ip_vs_lblc.c  | 4 ++--
 net/netfilter/ipvs/ip_vs_lblcr.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_lblc.c b/net/netfilter/ipvs/ip_vs_lblc.c
index 6a340c9..942e835 100644
--- a/net/netfilter/ipvs/ip_vs_lblc.c
+++ b/net/netfilter/ipvs/ip_vs_lblc.c
@@ -238,7 +238,7 @@ static void ip_vs_lblc_flush(struct ip_vs_service *svc)
int i;
 
spin_lock_bh(>sched_lock);
-   tbl->dead = 1;
+   tbl->dead = true;
for (i = 0; i < IP_VS_LBLC_TAB_SIZE; i++) {
hlist_for_each_entry_safe(en, next, >bucket[i], list) {
ip_vs_lblc_del(en);
@@ -369,7 +369,7 @@ static int ip_vs_lblc_init_svc(struct ip_vs_service *svc)
tbl->max_size = IP_VS_LBLC_TAB_SIZE*16;
tbl->rover = 0;
tbl->counter = 1;
-   tbl->dead = 0;
+   tbl->dead = false;
tbl->svc = svc;
 
/*
diff --git a/net/netfilter/ipvs/ip_vs_lblcr.c b/net/netfilter/ipvs/ip_vs_lblcr.c
index 0627881..a5acab2 100644
--- a/net/netfilter/ipvs/ip_vs_lblcr.c
+++ b/net/netfilter/ipvs/ip_vs_lblcr.c
@@ -404,7 +404,7 @@ static void ip_vs_lblcr_flush(struct ip_vs_service *svc)
struct hlist_node *next;
 
spin_lock_bh(>sched_lock);
-   tbl->dead = 1;
+   tbl->dead = true;
for (i = 0; i < IP_VS_LBLCR_TAB_SIZE; i++) {
hlist_for_each_entry_safe(en, next, >bucket[i], list) {
ip_vs_lblcr_free(en);
@@ -532,7 +532,7 @@ static int ip_vs_lblcr_init_svc(struct ip_vs_service *svc)
tbl->max_size = IP_VS_LBLCR_TAB_SIZE*16;
tbl->rover = 0;
tbl->counter = 1;
-   tbl->dead = 0;
+   tbl->dead = false;
tbl->svc = svc;
 
/*
-- 
2.7.4

Re: [PATCH] rds: send: mark expected switch fall-through in rds_rm_size

[Gustavo A. R. Silva]

Hi Santosh,

On 02/20/2018 11:54 AM, Santosh Shilimkar wrote:

Hi,

2/19/2018 10:10 AM, Gustavo A. R. Silva wrote:

In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.

Addresses-Coverity-ID: 1465362 ("Missing break in switch")
Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
  net/rds/send.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/net/rds/send.c b/net/rds/send.c
index 028ab59..79d158b 100644
--- a/net/rds/send.c
+++ b/net/rds/send.c
@@ -902,6 +902,8 @@ static int rds_rm_size(struct msghdr *msg, int 
num_sgs)

  case RDS_CMSG_ZCOPY_COOKIE:
  zcopy_cookie = true;
+    /* fall through */
+
  case RDS_CMSG_RDMA_DEST:
  case RDS_CMSG_RDMA_MAP:
  cmsg_groups |= 2;


So coverity greps for commet as "fall through" for
-Wimplicit-fallthrough build ?



No. Basically, Coverity only reports cases in which a break, return or 
continue statement is missing.


Now, if the statements I mention above are missing and if you add the 
following line to your Makefile:


KBUILD_CFLAGS  += $(call cc-option,-Wimplicit-fallthrough)

You will get a warning if a fall-through comment is missing.


Adding that comments itself if fine but was curious
about it if some one makes a spell error in this
comment what happens ;-)



In this case, Coverity would still report the same "Missing break in 
switch" error, but you'll get a GCC warning.



For patch itself,
Acked-by: Santosh Shilimkar <santosh.shilim...@oracle.com>


--
Gustavo

Re: [PATCH] rds: send: mark expected switch fall-through in rds_rm_size

[Gustavo A. R. Silva]

On 02/20/2018 12:01 PM, David Miller wrote:

From: Santosh Shilimkar 
Date: Tue, 20 Feb 2018 09:54:09 -0800


So coverity greps for commet as "fall through" for
-Wimplicit-fallthrough build ?


 From what I understand, 'gcc' does in the latest versions.  Coverity
might as well, I don't know.



Yeah, the one that reports those warnings is GCC.

Coverity only knows about missing break, return and continue.

--
Gustavo

[PATCH] rds: send: mark expected switch fall-through in rds_rm_size

[Gustavo A. R. Silva]

In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.

Addresses-Coverity-ID: 1465362 ("Missing break in switch")
Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 net/rds/send.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/rds/send.c b/net/rds/send.c
index 028ab59..79d158b 100644
--- a/net/rds/send.c
+++ b/net/rds/send.c
@@ -902,6 +902,8 @@ static int rds_rm_size(struct msghdr *msg, int num_sgs)
 
case RDS_CMSG_ZCOPY_COOKIE:
zcopy_cookie = true;
+   /* fall through */
+
case RDS_CMSG_RDMA_DEST:
case RDS_CMSG_RDMA_MAP:
cmsg_groups |= 2;
-- 
2.7.4

[PATCH] net: dsa: mv88e6xxx: hwtstamp: remove unnecessary range checking tests

[Gustavo A. R. Silva]

_port_ is already known to be a valid index in the callers [1]. So
these checks are unnecessary.

[1] https://lkml.org/lkml/2018/2/16/469

Addresses-Coverity-ID: 1465287
Addresses-Coverity-ID: 1465291
Suggested-by: Richard Cochran <richardcoch...@gmail.com>
Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 drivers/net/dsa/mv88e6xxx/hwtstamp.c | 9 -
 1 file changed, 9 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.c 
b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
index b251d53..c6d6a35 100644
--- a/drivers/net/dsa/mv88e6xxx/hwtstamp.c
+++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
@@ -179,9 +179,6 @@ int mv88e6xxx_port_hwtstamp_set(struct dsa_switch *ds, int 
port,
if (!chip->info->ptp_support)
return -EOPNOTSUPP;
 
-   if (port < 0 || port >= mv88e6xxx_num_ports(chip))
-   return -EINVAL;
-
if (copy_from_user(, ifr->ifr_data, sizeof(config)))
return -EFAULT;
 
@@ -206,9 +203,6 @@ int mv88e6xxx_port_hwtstamp_get(struct dsa_switch *ds, int 
port,
if (!chip->info->ptp_support)
return -EOPNOTSUPP;
 
-   if (port < 0 || port >= mv88e6xxx_num_ports(chip))
-   return -EINVAL;
-
return copy_to_user(ifr->ifr_data, config, sizeof(*config)) ?
-EFAULT : 0;
 }
@@ -255,9 +249,6 @@ static u8 *mv88e6xxx_should_tstamp(struct mv88e6xxx_chip 
*chip, int port,
if (!chip->info->ptp_support)
return NULL;
 
-   if (port < 0 || port >= mv88e6xxx_num_ports(chip))
-   return NULL;
-
hdr = parse_ptp_header(skb, type);
if (!hdr)
return NULL;
-- 
2.7.4

Re: [PATCH v2] net: dsa: mv88e6xxx: hwtstamp: fix potential negative array index read

[Gustavo A. R. Silva]

On 02/16/2018 09:56 AM, Richard Cochran wrote:

On Fri, Feb 16, 2018 at 07:48:46AM -0800, Richard Cochran wrote:

On Thu, Feb 15, 2018 at 12:31:39PM -0600, Gustavo A. R. Silva wrote:

_port_ is being used as index to array port_hwtstamp before verifying
it is a non-negative number and a valid index at line 209 and 258:

if (port < 0 || port >= mv88e6xxx_num_ports(chip))

Fix this by checking _port_ before using it as index to array
port_hwtstamp.


NAK.   Port is already known to be valid in the callers.


And so the real bug is the pointless range checking tests.  I would
welcome patches to remove those.



I just sent a patch for this.

Thank you both, Andrew and Richard for the feedback.
--
Gustavo

Re: [PATCH] i40evf/i40evf_main: Fix variable assignment in i40evf_parse_cls_flower

[Gustavo A. R. Silva]

On 02/15/2018 12:11 PM, Jeff Kirsher wrote:

On Thu, 2018-02-15 at 09:56 -0800, Ramamurthy, Harshitha wrote:

On Thu, 2018-02-15 at 11:44 -0600, Gustavo A. R. Silva wrote:

It seems this is a copy-paste error and that the proper variable to
use
in this particular case is _src_ instead of _dst_.

Addresses-Coverity-ID: 1465282 ("Copy-paste error")
Fixes: 0075fa0fadd0 ("i40evf: Add support to apply cloud filters")
Signed-off-by: Gustavo A. R. Silva <garsi...@embeddedor.com>


Thanks for the patch!



Glad to help. :)


Acked-by:Harshitha Ramamurthy <harshitha.ramamur...@intel.com>


Acked-by: Jeff Kirsher <jeffrey.t.kirs...@intel.com>

Dave, no need for me to pick this patch up and then push to you.  You
can circumvent me and pick this up.


---
  drivers/net/ethernet/intel/i40evf/i40evf_main.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)


Thanks guys.
--
Gustavo

[PATCH] net: dsa: mv88e6xxx: hwtstamp: fix potential negative array index read

[Gustavo A. R. Silva]

_port_ is being used as index to array port_hwtstamp before verifying
it is a non-negative number and a valid index at 209:

if (port < 0 || port >= mv88e6xxx_num_ports(chip))

Fix this by checking _port_ before using it as index to array
port_hwtstamp.

Addresses-Coverity-ID: 1465287 ("Negative array index read")
Fixes: c6fe0ad2c349 ("net: dsa: mv88e6xxx: add rx/tx timestamping support")
Signed-off-by: Gustavo A. R. Silva <garsi...@embeddedor.com>
---
 drivers/net/dsa/mv88e6xxx/hwtstamp.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.c 
b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
index b251d53..761decc 100644
--- a/drivers/net/dsa/mv88e6xxx/hwtstamp.c
+++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
@@ -200,8 +200,8 @@ int mv88e6xxx_port_hwtstamp_get(struct dsa_switch *ds, int 
port,
struct ifreq *ifr)
 {
struct mv88e6xxx_chip *chip = ds->priv;
-   struct mv88e6xxx_port_hwtstamp *ps = >port_hwtstamp[port];
-   struct hwtstamp_config *config = >tstamp_config;
+   struct mv88e6xxx_port_hwtstamp *ps;
+   struct hwtstamp_config *config;
 
if (!chip->info->ptp_support)
return -EOPNOTSUPP;
@@ -209,6 +209,9 @@ int mv88e6xxx_port_hwtstamp_get(struct dsa_switch *ds, int 
port,
if (port < 0 || port >= mv88e6xxx_num_ports(chip))
return -EINVAL;
 
+   ps = >port_hwtstamp[port];
+   config = >tstamp_config;
+
return copy_to_user(ifr->ifr_data, config, sizeof(*config)) ?
-EFAULT : 0;
 }
-- 
2.7.4

[PATCH v2] net: dsa: mv88e6xxx: hwtstamp: fix potential negative array index read

[Gustavo A. R. Silva]

_port_ is being used as index to array port_hwtstamp before verifying
it is a non-negative number and a valid index at line 209 and 258:

if (port < 0 || port >= mv88e6xxx_num_ports(chip))

Fix this by checking _port_ before using it as index to array
port_hwtstamp.

Addresses-Coverity-ID: 1465287 ("Negative array index read")
Addresses-Coverity-ID: 1465291 ("Negative array index read")
Fixes: c6fe0ad2c349 ("net: dsa: mv88e6xxx: add rx/tx timestamping support")
Signed-off-by: Gustavo A. R. Silva <garsi...@embeddedor.com>
---
Changes in v2:
 -Fix the same issue in mv88e6xxx_should_tstamp.
 -Update commit message.

 drivers/net/dsa/mv88e6xxx/hwtstamp.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.c 
b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
index b251d53..5a665aa 100644
--- a/drivers/net/dsa/mv88e6xxx/hwtstamp.c
+++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
@@ -200,8 +200,8 @@ int mv88e6xxx_port_hwtstamp_get(struct dsa_switch *ds, int 
port,
struct ifreq *ifr)
 {
struct mv88e6xxx_chip *chip = ds->priv;
-   struct mv88e6xxx_port_hwtstamp *ps = >port_hwtstamp[port];
-   struct hwtstamp_config *config = >tstamp_config;
+   struct mv88e6xxx_port_hwtstamp *ps;
+   struct hwtstamp_config *config;
 
if (!chip->info->ptp_support)
return -EOPNOTSUPP;
@@ -209,6 +209,9 @@ int mv88e6xxx_port_hwtstamp_get(struct dsa_switch *ds, int 
port,
if (port < 0 || port >= mv88e6xxx_num_ports(chip))
return -EINVAL;
 
+   ps = >port_hwtstamp[port];
+   config = >tstamp_config;
+
return copy_to_user(ifr->ifr_data, config, sizeof(*config)) ?
-EFAULT : 0;
 }
@@ -249,7 +252,7 @@ static u8 *parse_ptp_header(struct sk_buff *skb, unsigned 
int type)
 static u8 *mv88e6xxx_should_tstamp(struct mv88e6xxx_chip *chip, int port,
   struct sk_buff *skb, unsigned int type)
 {
-   struct mv88e6xxx_port_hwtstamp *ps = >port_hwtstamp[port];
+   struct mv88e6xxx_port_hwtstamp *ps;
u8 *hdr;
 
if (!chip->info->ptp_support)
@@ -262,6 +265,7 @@ static u8 *mv88e6xxx_should_tstamp(struct mv88e6xxx_chip 
*chip, int port,
if (!hdr)
return NULL;
 
+   ps = >port_hwtstamp[port];
if (!test_bit(MV88E6XXX_HWTSTAMP_ENABLED, >state))
return NULL;
 
-- 
2.7.4

[PATCH] i40evf/i40evf_main: Fix variable assignment in i40evf_parse_cls_flower

[Gustavo A. R. Silva]

It seems this is a copy-paste error and that the proper variable to use
in this particular case is _src_ instead of _dst_.

Addresses-Coverity-ID: 1465282 ("Copy-paste error")
Fixes: 0075fa0fadd0 ("i40evf: Add support to apply cloud filters")
Signed-off-by: Gustavo A. R. Silva <garsi...@embeddedor.com>
---
 drivers/net/ethernet/intel/i40evf/i40evf_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/i40evf/i40evf_main.c 
b/drivers/net/ethernet/intel/i40evf/i40evf_main.c
index 4955ce3..58be99e 100644
--- a/drivers/net/ethernet/intel/i40evf/i40evf_main.c
+++ b/drivers/net/ethernet/intel/i40evf/i40evf_main.c
@@ -2763,7 +2763,7 @@ static int i40evf_parse_cls_flower(struct i40evf_adapter 
*adapter,
 
if (key->src) {
filter->f.mask.tcp_spec.src_port |= cpu_to_be16(0x);
-   filter->f.data.tcp_spec.src_port = key->dst;
+   filter->f.data.tcp_spec.src_port = key->src;
}
}
filter->f.field_flags = field_flags;
-- 
2.7.4

[PATCH] netfilter: ipt_ah: return boolean instead of integer

[Gustavo A. R. Silva]

Return statements in functions returning bool should use
true/false instead of 1/0.

This issue was detected with the help of Coccinelle.

Signed-off-by: Gustavo A. R. Silva <garsi...@embeddedor.com>
---
 net/ipv4/netfilter/ipt_ah.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/netfilter/ipt_ah.c b/net/ipv4/netfilter/ipt_ah.c
index a787d07..7c6c20e 100644
--- a/net/ipv4/netfilter/ipt_ah.c
+++ b/net/ipv4/netfilter/ipt_ah.c
@@ -47,7 +47,7 @@ static bool ah_mt(const struct sk_buff *skb, struct 
xt_action_param *par)
 */
pr_debug("Dropping evil AH tinygram.\n");
par->hotdrop = true;
-   return 0;
+   return false;
}
 
return spi_match(ahinfo->spis[0], ahinfo->spis[1],
-- 
2.7.4

[PATCH] atm: he: use 64-bit arithmetic instead of 32-bit

[Gustavo A. R. Silva]

Add suffix ULL to constants 272, 204, 136 and 68 in order to give the
compiler complete information about the proper arithmetic to use.
Notice that these constants are used in contexts that expect
expressions of type unsigned long long (64 bits, unsigned).

The following expressions are currently being evaluated using 32-bit
arithmetic:

272 * mult
204 * mult
136 * mult
68 * mult

Addresses-Coverity-ID: 201058
Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 drivers/atm/he.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/atm/he.c b/drivers/atm/he.c
index e58538c..29f102d 100644
--- a/drivers/atm/he.c
+++ b/drivers/atm/he.c
@@ -738,13 +738,13 @@ static int he_init_cs_block_rcm(struct he_dev *he_dev)
 #else
/* this is pretty, but avoids _divdu3 and is mostly correct */
mult = he_dev->atm_dev->link_rate / ATM_OC3_PCR;
-   if (rate_cps > (272 * mult))
+   if (rate_cps > (272ULL * mult))
buf = 4;
-   else if (rate_cps > (204 * mult))
+   else if (rate_cps > (204ULL * mult))
buf = 3;
-   else if (rate_cps > (136 * mult))
+   else if (rate_cps > (136ULL * mult))
buf = 2;
-   else if (rate_cps > (68 * mult))
+   else if (rate_cps > (68ULL * mult))
buf = 1;
else
buf = 0;
-- 
2.7.4

Re: [PATCH] tcp_lp: use 64-bit arithmetic instead of 32-bit

[Gustavo A. R. Silva]

Hi Andrew,

Quoting Andrew Lunn <and...@lunn.ch>:


On Wed, Jan 31, 2018 at 07:07:49PM -0600, Gustavo A. R. Silva wrote:


Hi Alan,

Quoting Alan Cox <gno...@lxorguk.ukuu.org.uk>:

>On Wed, 31 Jan 2018 18:24:07 -0600
>"Gustavo A. R. Silva" <gust...@embeddedor.com> wrote:
>
>>Cast to s64 some variables and a macro in order to give the
>>compiler complete information about the proper arithmetic to
>>use. Notice that these elements are used in contexts that
>>expect expressions of type s64 (64 bits, signed).
>>
>>Currently such expression are being evaluated using 32-bit
>>arithmetic.
>
>The question you need to ask is 'can it overflow 32bit maths', otherwise
>you are potentially making the system do extra work for no reason.
>

Yeah, I get your point and it seems that in this particular case there is no
risk of a 32bit overflow, but in general and IMHO as the code evolves, the
use of incorrect arithmetic may have security implications in the future, so
I advocate for code correctness in this case.


Hi Gustavo

Is this on the hotpath? How much overhead does it add to 32 bit
architectures which don't have 64 bit arithmetic in hardware? There
are a lot of embedded systems which are 32 bit.



I'm sorry, I don't have access to 32-bit hardware at the moment.

Thanks
--
Gustavo

Re: [PATCH] tcp_lp: use 64-bit arithmetic instead of 32-bit

[Gustavo A. R. Silva]

Hi David,

Quoting David Miller <da...@davemloft.net>:


From: "Gustavo A. R. Silva" <gust...@embeddedor.com>
Date: Wed, 31 Jan 2018 18:24:07 -0600


Cast to s64 some variables and a macro in order to give the
compiler complete information about the proper arithmetic to
use. Notice that these elements are used in contexts that
expect expressions of type s64 (64 bits, signed).

Currently such expression are being evaluated using 32-bit
arithmetic.

Addresses-Coverity-ID: 200687
Addresses-Coverity-ID: 200688
Addresses-Coverity-ID: 200689
Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>


Sorry, I'm not applying this, the domain of the input values
means that the calculation cannot overflow.


Yep, that's fine.

Thanks for the feedback.
--
Gustavo

Re: [PATCH] tcp_lp: use 64-bit arithmetic instead of 32-bit

[Gustavo A. R. Silva]

Hi David,

Quoting David Laight :


> The question you need to ask is 'can it overflow 32bit maths', otherwise
> you are potentially making the system do extra work for no reason.
>

Yeah, I get your point and it seems that in this particular case there
is no risk of a 32bit overflow, but in general and IMHO as the code
evolves, the use of incorrect arithmetic may have security
implications in the future, so I advocate for code correctness in this
case.


Even if the variable are 64bit you still need to worry (maybe less)
about arithmetic overflow.
The only real way to avoid overflow is to understand the domain
of the values being used.



Yep, that's correct.

Thanks for the feedback.
--
Gustavo

Re: [PATCH] tcp_lp: use 64-bit arithmetic instead of 32-bit

[Gustavo A. R. Silva]

Hi Alan,

Quoting Alan Cox <gno...@lxorguk.ukuu.org.uk>:


On Wed, 31 Jan 2018 18:24:07 -0600
"Gustavo A. R. Silva" <gust...@embeddedor.com> wrote:


Cast to s64 some variables and a macro in order to give the
compiler complete information about the proper arithmetic to
use. Notice that these elements are used in contexts that
expect expressions of type s64 (64 bits, signed).

Currently such expression are being evaluated using 32-bit
arithmetic.


The question you need to ask is 'can it overflow 32bit maths', otherwise
you are potentially making the system do extra work for no reason.



Yeah, I get your point and it seems that in this particular case there  
is no risk of a 32bit overflow, but in general and IMHO as the code  
evolves, the use of incorrect arithmetic may have security  
implications in the future, so I advocate for code correctness in this  
case.


Thanks
--
Gustavo

[PATCH] tcp_lp: use 64-bit arithmetic instead of 32-bit

[Gustavo A. R. Silva]

Cast to s64 some variables and a macro in order to give the
compiler complete information about the proper arithmetic to
use. Notice that these elements are used in contexts that
expect expressions of type s64 (64 bits, signed).

Currently such expression are being evaluated using 32-bit
arithmetic.

Addresses-Coverity-ID: 200687
Addresses-Coverity-ID: 200688
Addresses-Coverity-ID: 200689
Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 net/ipv4/tcp_lp.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/tcp_lp.c b/net/ipv4/tcp_lp.c
index ae10ed6..4999111 100644
--- a/net/ipv4/tcp_lp.c
+++ b/net/ipv4/tcp_lp.c
@@ -134,7 +134,7 @@ static u32 tcp_lp_remote_hz_estimator(struct sock *sk)
 {
struct tcp_sock *tp = tcp_sk(sk);
struct lp *lp = inet_csk_ca(sk);
-   s64 rhz = lp->remote_hz << 6;   /* remote HZ << 6 */
+   s64 rhz = (s64)lp->remote_hz << 6;  /* remote HZ << 6 */
s64 m = 0;
 
/* not yet record reference time
@@ -147,7 +147,7 @@ static u32 tcp_lp_remote_hz_estimator(struct sock *sk)
tp->rx_opt.rcv_tsecr == lp->local_ref_time)
goto out;
 
-   m = TCP_TS_HZ *
+   m = (s64)TCP_TS_HZ *
(tp->rx_opt.rcv_tsval - lp->remote_ref_time) /
(tp->rx_opt.rcv_tsecr - lp->local_ref_time);
if (m < 0)
@@ -193,8 +193,8 @@ static u32 tcp_lp_owd_calculator(struct sock *sk)
 
if (lp->flag & LP_VALID_RHZ) {
owd =
-   tp->rx_opt.rcv_tsval * (LP_RESOL / lp->remote_hz) -
-   tp->rx_opt.rcv_tsecr * (LP_RESOL / TCP_TS_HZ);
+   (s64)tp->rx_opt.rcv_tsval * (LP_RESOL / lp->remote_hz) -
+   (s64)tp->rx_opt.rcv_tsecr * (LP_RESOL / TCP_TS_HZ);
if (owd < 0)
owd = -owd;
}
-- 
2.7.4

Re: [PATCH] libceph: use 64-bit arithmetic instead of 32-bit

[Gustavo A. R. Silva]

Hello Ilya,

Quoting Ilya Dryomov <idryo...@gmail.com>:


On Wed, Jan 31, 2018 at 6:29 AM, Gustavo A. R. Silva
<gust...@embeddedor.com> wrote:

Cast objsetno to u64 in order to give the compiler complete
information about the proper arithmetic to use. Notice
that this variable is used in a context that expects an
expression of type u64 (64 bits, unsigned).

The expression objsetno * sc + stripepos is currently
being evaluated using 32-bit arithmetic.

In general, the use of incorrect arithmetic has security
implications.

Addresses-Coverity-ID: 200686
Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 net/ceph/osdmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ceph/osdmap.c b/net/ceph/osdmap.c
index 0da27c6..58dc965 100644
--- a/net/ceph/osdmap.c
+++ b/net/ceph/osdmap.c
@@ -2183,7 +2183,7 @@ int ceph_calc_file_object_mapping(struct  
ceph_file_layout *layout,

stripepos = bl % sc;
objsetno = stripeno / su_per_object;

-   *ono = objsetno * sc + stripepos;
+   *ono = (u64)objsetno * sc + stripepos;
dout("objset %u * sc %u = ono %u\n", objsetno, sc,  
(unsigned int)*ono);


/* *oxoff = *off % layout->fl_stripe_unit;  # offset in su */


Hi Gustavo,

This (and other u32/u64 issues in this function, is this the only
warning?) is fixed in my striping v2 work branch.  I wasn't going to
push that patch separately, but I guess I should post it.



Yeah, this was the only one warning reported by Coverity in this  
particular module.


Thanks
--
Gustavo

[PATCH] libceph: use 64-bit arithmetic instead of 32-bit

[Gustavo A. R. Silva]

Cast objsetno to u64 in order to give the compiler complete
information about the proper arithmetic to use. Notice
that this variable is used in a context that expects an
expression of type u64 (64 bits, unsigned).

The expression objsetno * sc + stripepos is currently
being evaluated using 32-bit arithmetic.

In general, the use of incorrect arithmetic has security
implications.

Addresses-Coverity-ID: 200686
Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 net/ceph/osdmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ceph/osdmap.c b/net/ceph/osdmap.c
index 0da27c6..58dc965 100644
--- a/net/ceph/osdmap.c
+++ b/net/ceph/osdmap.c
@@ -2183,7 +2183,7 @@ int ceph_calc_file_object_mapping(struct ceph_file_layout 
*layout,
stripepos = bl % sc;
objsetno = stripeno / su_per_object;
 
-   *ono = objsetno * sc + stripepos;
+   *ono = (u64)objsetno * sc + stripepos;
dout("objset %u * sc %u = ono %u\n", objsetno, sc, (unsigned int)*ono);
 
/* *oxoff = *off % layout->fl_stripe_unit;  # offset in su */
-- 
2.7.4

[PATCH] openvswitch: meter: Use 64-bit arithmetic instead of 32-bit

[Gustavo A. R. Silva]

Add suffix LL to constant 1000 in order to give the compiler
complete information about the proper arithmetic to use. Notice
that this constant is used in a context that expects an expression
of type long long int (64 bits, signed).

The expression (band->burst_size + band->rate) * 1000 is currently
being evaluated using 32-bit arithmetic.

Addresses-Coverity-ID: 1461563 ("Unintentional integer overflow")
Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
 net/openvswitch/meter.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/openvswitch/meter.c b/net/openvswitch/meter.c
index 3fbfc78..04b9428 100644
--- a/net/openvswitch/meter.c
+++ b/net/openvswitch/meter.c
@@ -488,7 +488,7 @@ bool ovs_meter_execute(struct datapath *dp, struct sk_buff 
*skb,
long long int max_bucket_size;
 
band = >bands[i];
-   max_bucket_size = (band->burst_size + band->rate) * 1000;
+   max_bucket_size = (band->burst_size + band->rate) * 1000LL;
 
band->bucket += delta_ms * band->rate;
if (band->bucket > max_bucket_size)
-- 
2.7.4