Hello Xinming Hu,
The patch cbf6e05527a7: "mwifiex: add rx histogram statistics
support" from Dec 23, 2014, leads to the following static checker
warning:
drivers/net/wireless/marvell/mwifiex/util.c:714 mwifiex_hist_data_set()
error: buffer underflow 'phist_data->snr' '(-128)-127'
drivers/net/wireless/marvell/mwifiex/util.c
706 /* function to add histogram record */
707 void mwifiex_hist_data_set(struct mwifiex_private *priv, u8 rx_rate, s8 snr,
^^^^^^
708 s8 nflr)
709 {
710 struct mwifiex_histogram_data *phist_data = priv->hist_data;
711
712 atomic_inc(&phist_data->num_samples);
713 atomic_inc(&phist_data->rx_rate[rx_rate]);
714 atomic_inc(&phist_data->snr[snr]);
715 atomic_inc(&phist_data->noise_flr[128 + nflr]);
716 atomic_inc(&phist_data->sig_str[nflr - snr]);
Smatch complains that "snr" comes from skb->data so it's untrusted and
it can be less than zero and underflow the ->snr array.
->snr, ->noise_flr and ->sig_str all have 256 elements. Obviously it
seems like "snr" should be declared as a u8 instead of an s8. But I'm
not totally sure what to do about the ->noise_flr and ->sig_str[]
arrays.
717 }
regards,
dan carpenter
Hi Dan,
Thanks for the report!
We also notice it recently, and have already fix it.
Just upstream the below fix
https://patchwork.kernel.org/patch/10408353/
Regards,
Simon
________________________________________
From: Dan Carpenter <[email protected]>
Sent: Thursday, May 17, 2018 17:27
To: Xinming Hu
Cc: [email protected]
Subject: [EXT] [bug report] mwifiex: add rx histogram statistics support
External Email
----------------------------------------------------------------------
Hello Xinming Hu,
The patch cbf6e05527a7: "mwifiex: add rx histogram statistics
support" from Dec 23, 2014, leads to the following static checker
warning:
drivers/net/wireless/marvell/mwifiex/util.c:714 mwifiex_hist_data_set()
error: buffer underflow 'phist_data->snr' '(-128)-127'
drivers/net/wireless/marvell/mwifiex/util.c
706 /* function to add histogram record */
707 void mwifiex_hist_data_set(struct mwifiex_private *priv, u8 rx_rate, s8 snr,
^^^^^^
708 s8 nflr)
709 {
710 struct mwifiex_histogram_data *phist_data = priv->hist_data;
711
712 atomic_inc(&phist_data->num_samples);
713 atomic_inc(&phist_data->rx_rate[rx_rate]);
714 atomic_inc(&phist_data->snr[snr]);
715 atomic_inc(&phist_data->noise_flr[128 + nflr]);
716 atomic_inc(&phist_data->sig_str[nflr - snr]);
Smatch complains that "snr" comes from skb->data so it's untrusted and
it can be less than zero and underflow the ->snr array.
->snr, ->noise_flr and ->sig_str all have 256 elements. Obviously it
seems like "snr" should be declared as a u8 instead of an s8. But I'm
not totally sure what to do about the ->noise_flr and ->sig_str[]
arrays.
717 }
regards,
dan carpenter