Bug 177341 - iwlwifi: dvm/mvm: UBSAN warning in rs.c - WIFILNX-119
Summary: iwlwifi: dvm/mvm: UBSAN warning in rs.c - WIFILNX-119
Status: CLOSED CODE_FIX
Alias: None
Product: Drivers
Classification: Unclassified
Component: network-wireless (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: DO NOT USE - assign "network-wireless-intel" component instead
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-10-13 12:42 UTC by Bob Copeland
Modified: 2017-01-08 09:17 UTC (History)
1 user (show)

See Also:
Kernel Version: 4.8
Subsystem:
Regression: No
Bisected commit-id:


Attachments

Description Bob Copeland 2016-10-13 12:42:25 UTC
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.
Comment 1 Bob Copeland 2016-10-13 12:43:24 UTC
Also Johannes pointed out mvm might have the same kind of issue.
Comment 2 Luca Coelho 2016-10-13 12:45:48 UTC
Thanks for reporting! We'll take a look into this.
Comment 3 Emmanuel Grumbach 2017-01-07 17:55:47 UTC
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;
Comment 4 Emmanuel Grumbach 2017-01-07 17:56:30 UTC
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;
Comment 5 Emmanuel Grumbach 2017-01-08 09:16:54 UTC
Closing the bug. Please reopen if needed.

Patch has been submitted internally.

Note You need to log in before you can comment on or make changes to this bug.