Bug 177341
| Summary: | iwlwifi: dvm/mvm: UBSAN warning in rs.c - WIFILNX-119 | ||
|---|---|---|---|
| Product: | Drivers | Reporter: | Bob Copeland (me) |
| Component: | network-wireless | Assignee: | DO NOT USE - assign "network-wireless-intel" component instead (linuxwifi) |
| Status: | CLOSED CODE_FIX | ||
| Severity: | normal | CC: | luca |
| Priority: | P1 | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Kernel Version: | 4.8 | Subsystem: | |
| Regression: | No | Bisected commit-id: | |
Also Johannes pointed out mvm might have the same kind of issue. Thanks for reporting! We'll take a look into this. Yup - this is a problem, but I'd say that we don't really care since if i is negative, the conditional checking will prevent any use of mask.
Do you agree with this:
diff --git a/drivers/net/wireless/intel/iwlwifi/dvm/rs.c b/drivers/net/wireless/intel/iwlwifi/dvm/rs.c
index 9187879..44ff88d 100644
--- a/drivers/net/wireless/intel/iwlwifi/dvm/rs.c
+++ b/drivers/net/wireless/intel/iwlwifi/dvm/rs.c
@@ -740,7 +740,10 @@ static u16 rs_get_adjacent_rate(struct iwl_priv *priv, u8 index, u16 rate_mask,
/* Find the previous rate that is in the rate mask */
i = index - 1;
- for (mask = (1 << i); i >= 0; i--, mask >>= 1) {
+ if (i > 0)
+ mask = BIT(i);
+
+ for (; i >= 0; i--, mask >>= 1) {
if (rate_mask & mask) {
low = i;
break;
err...
Obviously, I need to say:
diff --git a/drivers/net/wireless/intel/iwlwifi/dvm/rs.c b/drivers/net/wireless/intel/iwlwifi/dvm/rs.c
index 9187879..ba775f6 100644
--- a/drivers/net/wireless/intel/iwlwifi/dvm/rs.c
+++ b/drivers/net/wireless/intel/iwlwifi/dvm/rs.c
@@ -740,7 +740,10 @@ static u16 rs_get_adjacent_rate(struct iwl_priv *priv, u8 index, u16 rate_mask,
/* Find the previous rate that is in the rate mask */
i = index - 1;
- for (mask = (1 << i); i >= 0; i--, mask >>= 1) {
+ if (i >= 0)
+ mask = BIT(i);
+
+ for (; i >= 0; i--, mask >>= 1) {
if (rate_mask & mask) {
low = i;
break;
Closing the bug. Please reopen if needed. Patch has been submitted internally. |
I have the following warning in iwldvm: [ 52.782238] UBSAN: Undefined behaviour in drivers/net/wireless/intel/iwlwifi/dvm/rs.c:746:18 [ 52.783284] shift exponent -1 is negative [ 52.784254] CPU: 3 PID: 1897 Comm: irq/29-iwlwifi Not tainted 4.8.0-wt+ #52 [ 52.784255] Hardware name: LENOVO 3443CTO/3443CTO, BIOS G6ET23WW (1.02 ) 08/14/2012 [ 52.784256] ffffffffa1553958 ffff880104d67638 ffffffff815ddb33 000000000000001d [ 52.784259] ffff880104d67668 0000000000000001 ffff880104d67650 ffffffff8163cfbd [ 52.784262] ffffffffffffffff ffff880104d676e8 ffffffff8163d5cb 0000000000000c2b [ 52.784264] Call Trace: [ 52.784270] [<ffffffff815ddb33>] dump_stack+0x86/0xd3 [ 52.784272] [<ffffffff8163cfbd>] ubsan_epilogue+0xd/0x40 [ 52.784274] [<ffffffff8163d5cb>] __ubsan_handle_shift_out_of_bounds+0xeb/0x130 [ 52.784277] [<ffffffff81135ab2>] ? try_to_wake_up+0x52/0x930 [ 52.784282] [<ffffffffa1522452>] rs_get_adjacent_rate+0x262/0x2a0 [iwldvm] [ 52.784286] [<ffffffffa154569e>] rs_get_best_rate+0x112/0x1ce [iwldvm] [ 52.784290] [<ffffffffa15458a5>] rs_switch_to_siso+0x14b/0x19b [iwldvm] [ 52.784293] [<ffffffffa1526583>] rs_tx_status+0x33c3/0x4040 [iwldvm] [ 52.784296] [<ffffffff810ee69a>] ? __local_bh_enable_ip+0x7a/0x130 [ 52.784317] [<ffffffffa12c5a02>] ieee80211_tx_status+0x7d2/0x1710 [mac80211] [ 52.784329] [<ffffffffa12c565b>] ? ieee80211_tx_status+0x42b/0x1710 [mac80211] [ 52.784331] [<ffffffff811779fd>] ? mark_held_locks+0x8d/0x150 [ 52.784335] [<ffffffffa152f48c>] iwlagn_rx_reply_tx+0x32c/0xf60 [iwldvm] [ 52.784338] [<ffffffffa152f929>] ? iwlagn_rx_reply_tx+0x7c9/0xf60 [iwldvm] [ 52.784341] [<ffffffff816078de>] ? bsearch+0x5e/0x90 [ 52.784344] [<ffffffffa153cf2b>] iwl_rx_dispatch+0xab/0x130 [iwldvm] [ 52.784349] [<ffffffffa10511ab>] iwl_pcie_rx_handle+0x34b/0xe90 [iwlwifi] [ 52.784353] [<ffffffffa1053d04>] iwl_pcie_irq_handler+0x5c4/0xd30 [iwlwifi] [ 52.784357] [<ffffffffa1053745>] ? iwl_pcie_irq_handler+0x5/0xd30 [iwlwifi] [ 52.784359] [<ffffffff81199c55>] irq_thread_fn+0x25/0x90 [ 52.784362] [<ffffffff81aa3c88>] ? schedule+0x48/0x100 [ 52.784363] [<ffffffff811996c4>] irq_thread+0x1a4/0x330 [ 52.784365] [<ffffffff81199c30>] ? irq_thread_dtor+0x150/0x150 [ 52.784367] [<ffffffff81199ae0>] ? irq_forced_thread_fn+0xa0/0xa0 [ 52.784369] [<ffffffff81199520>] ? irq_thread_check_affinity+0xb0/0xb0 [ 52.784371] [<ffffffff81122703>] kthread+0x133/0x1a0 [ 52.784373] [<ffffffff81aacaaf>] ret_from_fork+0x1f/0x40 [ 52.784375] [<ffffffff811225d0>] ? kthread_create_on_node+0x240/0x240 [ 52.784376] ================================================================================ It looks like the relevant code is: /* Find the previous rate that is in the rate mask */ i = index - 1; for (mask = (1 << i); i >= 0; i--, mask >>= 1) { if (rate_mask & mask) { low = i; break; } } My guess is index comes into the function as 0 and the loop initializer then is "mask = (1 << -1)" which happens before the loop conditional checking that i is greater than zero and thus invokes undefined behavior.