On Thu, Jun 4, 2020 at 1:18 PM Kees Cook [off-list ref] wrote:
On Thu, Jun 04, 2020 at 01:08:44PM -0700, Nick Desaulniers wrote:
quoted
On Wed, Jun 3, 2020 at 4:32 PM Kees Cook [off-list ref] wrote:
quoted
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.
oh, and there's an early return for `phy->rev < 3` I just noticed.
Yeah, that's why I put the "(unreachable!)" note in the commit log. ;)
I don't think that note is correct.
quoted
quoted
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?
I mean, sort of the best thing to do is just remove nearly everything
here since it's actually unreachable. But it is commented as "when
This code is reachable. Consider `phy->rev >= 7 && phy->rev < 19`. If
`rf_pwr_offset_table` was NULL, it would have returned early on L4246,
so the checks added in this patch are unnecessary. Forgive me if
there's some other control flow I'm not considering.
supported ..." etc, so I figured I'd leave it. As part of that I didn't
want to leave any chance of a NULL deref, so I added the explicit tests
just for robustness.
*shrug*
--
Thanks,
~Nick Desaulniers