From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Gustavo A. R. Silva" Subject: Re: [PATCH] ath10k: htt_rx: Fix signedness bug in ath10k_update_per_peer_tx_stats Date: Fri, 5 Oct 2018 21:15:49 +0200 Message-ID: References: <20181005184245.GA11700@embeddedor.com> <3a55bec6-d20d-f500-e741-b228a86b7117@candelatech.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Cc: ath10k-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, linux-wireless-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Ben Greear , Kalle Valo , "David S. Miller" Return-path: In-Reply-To: Content-Language: en-GB Sender: linux-wireless-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: netdev.vger.kernel.org On 10/5/18 9:14 PM, Gustavo A. R. Silva wrote: > > > On 10/5/18 9:09 PM, Ben Greear wrote: >> On 10/05/2018 11:42 AM, Gustavo A. R. Silva wrote: >>> Currently, the error handling for the call to function >>> ath10k_get_legacy_rate_idx() doesn't work because >>> *rate_idx* is of type u8 (8 bits, unsigned), which >>> makes it impossible for it to hold a value less >>> than 0. >>> >>> Fix this by changing the type of variable *rate_idx* >>> to s8 (8 bits, signed). >> >> There are more than 127 rates, are you sure this is doing >> what you want? >> > > Based on the following function, rate_idx can only hold values from 0 to 11 > ... and of course -EINVAL too > static inline int ath10k_get_legacy_rate_idx(struct ath10k *ar, u8 rate) > { > static const u8 legacy_rates[] = {1, 2, 5, 11, 6, 9, 12, > 18, 24, 36, 48, 54}; > int i; > > for (i = 0; i < ARRAY_SIZE(legacy_rates); i++) { > if (rate == legacy_rates[i]) > return i; > } > > ath10k_warn(ar, "Invalid legacy rate %hhd peer stats", rate); > return -EINVAL; > } > > Thanks > -- > Gustavo > >> Thanks, >> Ben >> >>> >>> Addresses-Coverity-ID: 1473914 ("Unsigned compared against 0") >>> Fixes: 0189dbd71cbd ("ath10k: get the legacy rate index to update the txrate table") >>> Signed-off-by: Gustavo A. R. Silva >>> --- >>>  drivers/net/wireless/ath/ath10k/htt_rx.c | 3 ++- >>>  1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c >>> index f240525..edd0e74 100644 >>> --- a/drivers/net/wireless/ath/ath10k/htt_rx.c >>> +++ b/drivers/net/wireless/ath/ath10k/htt_rx.c >>> @@ -2753,7 +2753,8 @@ ath10k_update_per_peer_tx_stats(struct ath10k *ar, >>>                  struct ath10k_per_peer_tx_stats *peer_stats) >>>  { >>>      struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv; >>> -    u8 rate = 0, rate_idx = 0, sgi; >>> +    u8 rate = 0, sgi; >>> +    s8 rate_idx = 0; >>>      struct rate_info txrate; >>> >>>      lockdep_assert_held(&ar->data_lock); >>> >> >>