Re: [PATCH v3 00/18] wilc1000: move out of staging

From: Dan Carpenter <dan.carpenter@oracle.com>
To: Ajay.Kathat@microchip.com
Cc: linux-wireless@vger.kernel.org, devel@driverdev.osuosl.org,
	devicetree@vger.kernel.org, Venkateswara.Kaja@microchip.com,
	gregkh@linuxfoundation.org, Nicolas.Ferre@microchip.com,
	Adham.Abozaeid@microchip.com, johannes@sipsolutions.net
Subject: Re: [PATCH v3 00/18] wilc1000: move out of staging
Date: Mon, 2 Mar 2020 12:23:46 +0300	[thread overview]
Message-ID: <20200302092346.GA24308@kadam> (raw)
In-Reply-To: <20200225074105.7740-1-ajay.kathat@microchip.com>

There are a few static checker warnings from Friday's linux-next.  Only
the first one is important.  (Not all these Smatch warnings have been
published).

drivers/staging/wilc1000/hif.c:804 wilc_hif_pack_sta_param() warn: '&params->ht_capa' sometimes too small '8' size = 29

drivers/staging/wilc1000/hif.c
   787  static void wilc_hif_pack_sta_param(u8 *cur_byte, const u8 *mac,
   788                                      struct station_parameters *params)
   789  {
   790          ether_addr_copy(cur_byte, mac);
   791          cur_byte += ETH_ALEN;
   792  
   793          put_unaligned_le16(params->aid, cur_byte);
   794          cur_byte += 2;
   795  
   796          *cur_byte++ = params->supported_rates_len;
   797          if (params->supported_rates_len > 0)
   798                  memcpy(cur_byte, params->supported_rates,
   799                         params->supported_rates_len);
   800          cur_byte += params->supported_rates_len;
   801  
   802          if (params->ht_capa) {
   803                  *cur_byte++ = true;
   804                  memcpy(cur_byte, &params->ht_capa,
                                         ^^^^^^^^^^^^^^^^
This is copying the wrong data.  The "&" is wrong.

   805                         sizeof(struct ieee80211_ht_cap));
   806          } else {
   807                  *cur_byte++ = false;
   808          }
   809          cur_byte += sizeof(struct ieee80211_ht_cap);
   810  
   811          put_unaligned_le16(params->sta_flags_mask, cur_byte);
   812          cur_byte += 2;
   813          put_unaligned_le16(params->sta_flags_set, cur_byte);
   814  }

drivers/staging/wilc1000/cfg80211.c:904 del_pmksa() warn: 'i < priv->pmkid_list.numpmkid' 'true' implies 'priv->pmkid_list.numpmkid > 0' is 'true'

drivers/staging/wilc1000/cfg80211.c
   887  static int del_pmksa(struct wiphy *wiphy, struct net_device *netdev,
   888                       struct cfg80211_pmksa *pmksa)
   889  {
   890          u32 i;
   891          int ret = 0;
   892          struct wilc_vif *vif = netdev_priv(netdev);
   893          struct wilc_priv *priv = &vif->priv;
   894  
   895          for (i = 0; i < priv->pmkid_list.numpmkid; i++) {
   896                  if (!memcmp(pmksa->bssid, priv->pmkid_list.pmkidlist[i].bssid,
   897                              ETH_ALEN)) {
   898                          memset(&priv->pmkid_list.pmkidlist[i], 0,
   899                                 sizeof(struct wilc_pmkid));
   900                          break;
   901                  }
   902          }
   903  
   904          if (i < priv->pmkid_list.numpmkid && priv->pmkid_list.numpmkid > 0) {
                                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This part of the condition is a given (must be true).  Delete it.  It's
better to reverse the test and say:

	if (i == priv->pmkid_list.numpmkid)
		return -EINVAL;

   905                  for (; i < (priv->pmkid_list.numpmkid - 1); i++) {
   906                          memcpy(priv->pmkid_list.pmkidlist[i].bssid,
   907                                 priv->pmkid_list.pmkidlist[i + 1].bssid,
   908                                 ETH_ALEN);
   909                          memcpy(priv->pmkid_list.pmkidlist[i].pmkid,
   910                                 priv->pmkid_list.pmkidlist[i + 1].pmkid,
   911                                 WLAN_PMKID_LEN);
   912                  }
   913                  priv->pmkid_list.numpmkid--;
   914          } else {
   915                  ret = -EINVAL;
   916          }
   917  
   918          return ret;
   919  }

drivers/staging/wilc1000/wlan.c:706 wilc_wlan_handle_rx_buff() warn: 'pkt_len' 'true' implies 'pkt_len > 0' is 'true'

drivers/staging/wilc1000/wlan.c
   686          int is_cfg_packet;
   687          u8 *buff_ptr;
   688  
   689          do {
   690                  buff_ptr = buffer + offset;
   691                  header = get_unaligned_le32(buff_ptr);
   692  
   693                  is_cfg_packet = FIELD_GET(WILC_PKT_HDR_CONFIG_FIELD, header);
   694                  pkt_offset = FIELD_GET(WILC_PKT_HDR_OFFSET_FIELD, header);
   695                  tp_len = FIELD_GET(WILC_PKT_HDR_TOTAL_LEN_FIELD, header);
   696                  pkt_len = FIELD_GET(WILC_PKT_HDR_LEN_FIELD, header);
   697  
   698                  if (pkt_len == 0 || tp_len == 0)
                            ^^^^^^^^^^^^

   699                          break;
   700  
   701                  if (pkt_offset & IS_MANAGMEMENT) {
   702                          buff_ptr += HOST_HDR_OFFSET;
   703                          wilc_wfi_mgmt_rx(wilc, buff_ptr, pkt_len);
   704                  } else {
   705                          if (!is_cfg_packet) {
   706                                  if (pkt_len > 0) {
                                            ^^^^^^^^^^^
Delete.

   707                                          wilc_frmw_to_host(wilc, buff_ptr,
   708                                                            pkt_len, pkt_offset);
   709                                  }
   710                          } else {
   711                                  struct wilc_cfg_rsp rsp;
   712  
   713                                  buff_ptr += pkt_offset;
   714  

regards,
dan carpenter