On Wed, Jun 3, 2020 at 4:32 PM Kees Cook [off-list ref] wrote:
quoted hunk ↗ jump to hunk
Using uninitialized_var() is dangerous as it papers over real bugs[1]
(or can in the future), and suppresses unrelated compiler warnings (e.g.
"unused variable"). If the compiler thinks it is uninitialized, either
simply initialize the variable or make compiler changes. As a precursor
to removing[2] this[3] macro[4], just initialize this variable to NULL,
and make the (unreachable!) code do a conditional test.
[1] https://lore.kernel.org/lkml/20200603174714.192027-1-glider@google.com/ (local)
[2] https://lore.kernel.org/lkml/CA+55aFw+Vbj0i=1TGqCR5vQkCzWJ0QxK6CernOU6eedsudAixw@mail.gmail.com/ (local)
[3] https://lore.kernel.org/lkml/CA+55aFwgbgqhbp1fkxvRKEpzyR5J8n1vKT1VZdz9knmPuXhOeg@mail.gmail.com/ (local)
[4] https://lore.kernel.org/lkml/CA+55aFz2500WfbKXAx8s67wrm9=yVJu65TpLgN_ybYNv0VEOKA@mail.gmail.com/ (local)
Signed-off-by: Kees Cook <redacted>
---
drivers/net/wireless/broadcom/b43/phy_n.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/broadcom/b43/phy_n.c b/drivers/net/wireless/broadcom/b43/phy_n.c
index d3c001fa8eb4..88cdcea10d61 100644
--- a/drivers/net/wireless/broadcom/b43/phy_n.c
+++ b/drivers/net/wireless/broadcom/b43/phy_n.c
@@ -4222,7 +4222,7 @@ static void b43_nphy_tx_gain_table_upload(struct b43_wldev *dev)
The TODOs and `#if 0` in this function are concerning. It looks like
`rf_pwr_offset_table` is only used when `phy->rev` is >=7 && < 19.
Further, the loop has a case for `phy->rev >= 19` but we would have
returned earlier if that was the case.
quoted hunk ↗ jump to hunk
u32 rfpwr_offset;
u8 pga_gain, pad_gain;
int i;
- const s16 *uninitialized_var(rf_pwr_offset_table);
+ const s16 *rf_pwr_offset_table = NULL;
table = b43_nphy_get_tx_gain_table(dev);
if (!table)@@ -4256,9 +4256,13 @@ static void b43_nphy_tx_gain_table_upload(struct b43_wldev *dev)
pga_gain = (table[i] >> 24) & 0xf;
pad_gain = (table[i] >> 19) & 0x1f;
if (b43_current_band(dev->wl) == NL80211_BAND_2GHZ)
- rfpwr_offset = rf_pwr_offset_table[pad_gain];
+ rfpwr_offset = rf_pwr_offset_table
+ ? rf_pwr_offset_table[pad_gain]
+ : 0;
else
- rfpwr_offset = rf_pwr_offset_table[pga_gain];
+ rfpwr_offset = rf_pwr_offset_table
+ ? rf_pwr_offset_table[pga_gain]
+ : 0;
The code is trying to check `phy->rev >= 7 && phy->rev < 19` once
before the loop, then set `rf_pwr_offset_table`, so having another
conditional on `rf_pwr_offset_table` in the loop is unnecessary. I'm
ok with initializing it to `NULL`, but I'm not sure the conditional
check is necessary. Do you get a compiler warning otherwise?
} else {
pga_gain = (table[i] >> 24) & 0xF;
if (b43_current_band(dev->wl) == NL80211_BAND_2GHZ)
--
2.25.1
--
You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20200603233203.1695403-4-keescook%40chromium.org.
--
Thanks,
~Nick Desaulniers