UBSAN: Undefined behaviour in net/mac80211/rx.c:924:18
Subsystems (4)
4.5.0-rc1 another UBSAN error:
[ 4845.229441] ================================================================================ [ 4845.229454] UBSAN: Undefined behaviour in net/mac80211/rx.c:924:18 [ 4845.229458] load of value 2 is not a valid value for type '_Bool' [ 4845.229464] CPU: 1 PID: 6266 Comm: kworker/u16:8 Not tainted 4.5.0-rc1 #252 [ 4845.229468] Hardware name: Apple Inc. MacBookPro10,2/Mac-AFD8A9D944EA4843, BIOS MBP102.88Z.0106.B0A.1509130955 09/13/2015 [ 4845.229491] Workqueue: phy2 rt2x00usb_work_rxdone [ 4845.229493] 0000000000000000 0000000000000000 0000000000000002 ffff8801b13c39f8 [ 4845.229496] ffffffff81b2e7d9 0000000000000007 ffff8801b13c3a20 ffff8801b13c3a10 [ 4845.229498] ffffffff81bcb87d ffffffff85016890 ffff8801b13c3a60 ffffffff81bcc279 [ 4845.229501] Call Trace: [ 4845.229506] [<ffffffff81b2e7d9>] dump_stack+0x45/0x6c [ 4845.229510] [<ffffffff81bcb87d>] ubsan_epilogue+0xd/0x40 [ 4845.229513] [<ffffffff81bcc279>] __ubsan_handle_load_invalid_value+0x69/0x80 [ 4845.229517] [<ffffffff82280032>] ? xhci_setup_addressable_virt_dev+0xeb2/0x13b0 [ 4845.229520] [<ffffffff8121230b>] ? pick_next_entity+0xcb/0x280 [ 4845.229524] [<ffffffff82a7bce3>] ieee80211_sta_reorder_release.isra.15+0x7e3/0xad0 [ 4845.229527] [<ffffffff82a86837>] ieee80211_prepare_and_rx_handle+0x11a7/0x2ab0 [ 4845.229530] [<ffffffff82272694>] ? xhci_urb_enqueue+0x394/0x1140 [ 4845.229533] [<ffffffff82205d8f>] ? usb_hcd_map_urb_for_dma+0x94f/0x1140 [ 4845.229537] [<ffffffff82552927>] ? skb_release_data+0x117/0x2f0 [ 4845.229539] [<ffffffff82a883aa>] __ieee80211_rx_handle_packet+0x26a/0x9a0 [ 4845.229542] [<ffffffff8254ef8c>] ? __kmalloc_reserve.isra.11+0x2c/0x80 [ 4845.229545] [<ffffffff82a89131>] ieee80211_rx_napi+0x651/0x12b0 [ 4845.229549] [<ffffffff82188972>] rt2x00lib_rxdone+0x402/0x1120 [ 4845.229552] [<ffffffff8121b8ff>] ? dequeue_task_fair+0x97f/0x41d0 [ 4845.229554] [<ffffffff8219d19c>] rt2x00usb_work_rxdone+0xac/0x1f0 [ 4845.229558] [<ffffffff82b37cbd>] ? __schedule+0x5cd/0x1770 [ 4845.229561] [<ffffffff811dc3b6>] process_one_work+0x266/0xc00 [ 4845.229563] [<ffffffff811dce56>] worker_thread+0x96/0xd40 [ 4845.229565] [<ffffffff811dcdc0>] ? process_scheduled_works+0x70/0x70 [ 4845.229568] [<ffffffff811e91d8>] kthread+0x108/0x180 [ 4845.229571] [<ffffffff811e90d0>] ? kthread_create_on_node+0x210/0x210 [ 4845.229573] [<ffffffff82b40d9f>] ret_from_fork+0x3f/0x70 [ 4845.229576] [<ffffffff811e90d0>] ? kthread_create_on_node+0x210/0x210 [ 4845.229577] ================================================================================
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 1/1 net/mac80211/agg-rx.c: fix use of uninitialised values
Use kzalloc instead of kmalloc for struct tid_ampdu_rx. Fixes:
[ 7.976605] UBSAN: Undefined behaviour in net/mac80211/rx.c:932:29 [ 7.976608] load of value 2 is not a valid value for type '_Bool' [ 7.976611] CPU: 3 PID: 1134 Comm: kworker/u16:7 Not tainted 4.5.0-rc1+ #265 [ 7.976613] Hardware name: Apple Inc. MacBookPro10,2/Mac-AFD8A9D944EA4843, BIOS MBP102.88Z.0106.B0A.1509130955 09/13/2015 [ 7.976616] Workqueue: phy0 rt2x00usb_work_rxdone [ 7.976619] 0000000000000004 ffff880254a7ba50 ffffffff8181d866 0000000000000007 [ 7.976622] ffff880254a7ba78 ffff880254a7ba68 ffffffff8188422d ffffffff8379b500 [ 7.976626] ffff880254a7bab8 ffffffff81884747 0000000000000202 0000000348620032 [ 7.976629] Call Trace: [ 7.976633] [<ffffffff8181d866>] dump_stack+0x45/0x5f [ 7.976637] [<ffffffff8188422d>] ubsan_epilogue+0xd/0x40 [ 7.976642] [<ffffffff81884747>] __ubsan_handle_load_invalid_value+0x67/0x70 [ 7.976646] [<ffffffff82227b4d>] ieee80211_sta_reorder_release.isra.16+0x5ed/0x730 [ 7.976650] [<ffffffff8222ca14>] ieee80211_prepare_and_rx_handle+0xd04/0x1c00 [ 7.976654] [<ffffffff81cb27ce>] ? usb_hcd_map_urb_for_dma+0x65e/0x960 [ 7.976659] [<ffffffff8222db03>] __ieee80211_rx_handle_packet+0x1f3/0x750 [ 7.976663] [<ffffffff8222e4a7>] ieee80211_rx_napi+0x447/0x990 [ 7.976667] [<ffffffff81c5fb85>] rt2x00lib_rxdone+0x305/0xbd0 [ 7.976670] [<ffffffff811ac23f>] ? dequeue_task_fair+0x64f/0x1de0 [ 7.976674] [<ffffffff811a1516>] ? sched_clock_cpu+0xe6/0x150 [ 7.976678] [<ffffffff81c6c45c>] rt2x00usb_work_rxdone+0x7c/0x140 [ 7.976682] [<ffffffff8117aef6>] process_one_work+0x226/0x860 [ 7.976686] [<ffffffff8117b58c>] worker_thread+0x5c/0x680 [ 7.976690] [<ffffffff8117b530>] ? process_one_work+0x860/0x860 [ 7.976693] [<ffffffff81184f86>] kthread+0xf6/0x150 [ 7.976697] [<ffffffff81184e90>] ? kthread_worker_fn+0x310/0x310 [ 7.976700] [<ffffffff822a94df>] ret_from_fork+0x3f/0x70 [ 7.976703] [<ffffffff81184e90>] ? kthread_worker_fn+0x310/0x310
--- net/mac80211/agg-rx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
net/mac80211/agg-rx.c +1 -1
@@ -291,7 +291,7 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta, }/* prepare A-MPDU MLME for Rx aggregation */ - tid_agg_rx =kmalloc (sizeof (struct tid_ampdu_rx ),GFP_KERNEL ); + tid_agg_rx =kzalloc (sizeof (struct tid_ampdu_rx ),GFP_KERNEL );if (!tid_agg_rx) goto end;
Signature · 1 line
2.1.4
Re: [PATCH] net/mac80211/agg-rx.c: fix use of uninitialised values
Hi Chris,
Quoted 42 lines · Chris Bainbridge
Use kzalloc instead of kmalloc for struct
tid_ampdu_rx. Fixes:[ 7.976605] UBSAN: Undefined behaviour in net/mac80211/rx.c:932:29 [ 7.976608] load of value 2 is not a valid value for type '_Bool' [ 7.976611] CPU: 3 PID: 1134 Comm: kworker/u16:7 Not tainted 4.5.0-rc1+ #265 [ 7.976613] Hardware name: Apple Inc. MacBookPro10,2/Mac-AFD8A9D944EA4843, BIOS MBP102.88Z.0106.B0A.1509130955 09/13/2015 [ 7.976616] Workqueue: phy0 rt2x00usb_work_rxdone [ 7.976619] 0000000000000004 ffff880254a7ba50 ffffffff8181d866 0000000000000007 [ 7.976622] ffff880254a7ba78 ffff880254a7ba68 ffffffff8188422d ffffffff8379b500 [ 7.976626] ffff880254a7bab8 ffffffff81884747 0000000000000202 0000000348620032 [ 7.976629] Call Trace: [ 7.976633] [<ffffffff8181d866>] dump_stack+0x45/0x5f [ 7.976637] [<ffffffff8188422d>] ubsan_epilogue+0xd/0x40 [ 7.976642] [<ffffffff81884747>] __ubsan_handle_load_invalid_value+0x67/0x70 [ 7.976646] [<ffffffff82227b4d>] ieee80211_sta_reorder_release.isra.16+0x5ed/0x730 [ 7.976650] [<ffffffff8222ca14>] ieee80211_prepare_and_rx_handle+0xd04/0x1c00 [ 7.976654] [<ffffffff81cb27ce>] ? usb_hcd_map_urb_for_dma+0x65e/0x960 [ 7.976659] [<ffffffff8222db03>] __ieee80211_rx_handle_packet+0x1f3/0x750 [ 7.976663] [<ffffffff8222e4a7>] ieee80211_rx_napi+0x447/0x990 [ 7.976667] [<ffffffff81c5fb85>] rt2x00lib_rxdone+0x305/0xbd0 [ 7.976670] [<ffffffff811ac23f>] ? dequeue_task_fair+0x64f/0x1de0 [ 7.976674] [<ffffffff811a1516>] ? sched_clock_cpu+0xe6/0x150 [ 7.976678] [<ffffffff81c6c45c>] rt2x00usb_work_rxdone+0x7c/0x140 [ 7.976682] [<ffffffff8117aef6>] process_one_work+0x226/0x860 [ 7.976686] [<ffffffff8117b58c>] worker_thread+0x5c/0x680 [ 7.976690] [<ffffffff8117b530>] ? process_one_work+0x860/0x860 [ 7.976693] [<ffffffff81184f86>] kthread+0xf6/0x150 [ 7.976697] [<ffffffff81184e90>] ? kthread_worker_fn+0x310/0x310 [ 7.976700] [<ffffffff822a94df>] ret_from_fork+0x3f/0x70 [ 7.976703] [<ffffffff81184e90>] ? kthread_worker_fn+0x310/0x310Signed-off-by Chris Bainbridge <[email protected]>--- net/mac80211/agg-rx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)net/mac80211/agg-rx.c +0
-0
net/mac80211/agg-rx.c void __ieee80211_start_rx_ba_session(struct sta_info *sta, · line 294
Quoted 5 lines
net/mac80211/agg-rx.c +1
-1@@ -291,7 +291,7 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta, }/* prepare A-MPDU MLME for Rx aggregation */ - tid_agg_rx =kmalloc (sizeof (struct tid_ampdu_rx ),GFP_KERNEL ); + tid_agg_rx =kzalloc (sizeof (struct tid_ampdu_rx ),GFP_KERNEL );
This looks like a "big hammer" solution to this problem.
My reading of this is that the events leading up to UBSAN's warning are:
1. In __ieee80211_start_rx_ba_session() tid_agg_rx is kmalloc'd and has "random" data
2. All of tid_ampdu_rx's fields except the boolean "removed" are initialised
3. Stuff happens
4. We get to "set_release_timer" in ieee80211_sta_reorder_release
5. the random data means that "removed" has an incorrect value for a boolean
6. UBSAN barfs as above
I believe that false is stored as 0 internally (and true as 1) so kzalloc is a correct solution, but it's the heavy weight solution as all but one of the fields in the struct are initialised immediately after, so we're essentially zeroing the entire struct to ensure that one field is set to zero.
I'd prefer to just set ->removed to false right after we set ->auto_seq as that should be faster, however I don't know if __ieee80211_start_rx_ba_session() is a fast path so I don't know if this is saving anything.
Either way, this looks sane to me.
On another note, this is an error that should be pretty easy to spot. Could any of the automated tools find cases where a struct containing a bool variable is kmalloc'd and returned without assigning all the bools?
Thanks,
Signature · 4 lines
Julian Calaby Email: [email protected] Profile: http://www.google.com/profiles/julian.calaby/
Johannes Berg
maintainerRe: [PATCH] net/mac80211/agg-rx.c: fix use of uninitialised values
Quoted 2 lines · Julian Calaby
Â
This looks like a "big hammer" solution to this problem.
It is, in a way, but it also avoids future errors like it.
Quoted 4 lines
I'd prefer to just set ->removed to false right after we set ->auto_seq as that should be faster, however I don't know if
__ieee80211_start_rx_ba_session()is a fast path so I don't know if this is saving anything.
It's not supposed to be called frequently, no.
Quoted 4 lines
On another note, this is an error that should be pretty easy to spot. Could any of the automated tools find cases where a struct containing a bool variable is kmalloc'd and returned without assigning all the bools?
I think you'd quickly drown in false positives, since "return" isn't necessarily something that means it needs to have been fully initialized.
johannes
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net/mac80211/agg-rx.c: fix use of uninitialised values
Hi Johannes,
Quoted 7 lines · Johannes Berg
Quoted 4 lines · Julian Calaby
I'd prefer to just set ->removed to false right after we set ->auto_seq as that should be faster, however I don't know if
__ieee80211_start_rx_ba_session()is a fast path so I don't know if this is saving anything.It's not supposed to be called frequently, no.
Then most of my commentary is moot.
I guess the argument comes down to do we zero everything or initialise everything, and if speed isn't an issue, the former is better.
Quoted 8 lines
Quoted 4 lines
On another note, this is an error that should be pretty easy to spot. Could any of the automated tools find cases where a struct containing a bool variable is kmalloc'd and returned without assigning all the bools?
I think you'd quickly drown in false positives, since "return" isn't necessarily something that means it needs to have been fully initialized.
True.
Either way, I'm guessing that UBSAN will pick up a lot of similar bugs and the output of that is probably a much smaller haystack to dig through than just "every" kmalloc() call.
Thanks,
Signature · 4 lines
Julian Calaby Email: [email protected] Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH] net/mac80211/agg-rx.c: fix use of uninitialised values
Quoted 26 lines · Julian Calaby
Hi Johannes,
Quoted 7 lines · Johannes Berg
Quoted 4 lines · Julian Calaby
I'd prefer to just set ->removed to false right after we set ->auto_seq as that should be faster, however I don't know if
__ieee80211_start_rx_ba_session()is a fast path so I don't know if this is saving anything.It's not supposed to be called frequently, no.
Then most of my commentary is moot.
I guess the argument comes down to do we zero everything or initialise everything, and if speed isn't an issue, the former is better.
Quoted 8 lines
Quoted 4 lines
On another note, this is an error that should be pretty easy to spot. Could any of the automated tools find cases where a struct containing a bool variable is kmalloc'd and returned without assigning all the bools?
I think you'd quickly drown in false positives, since "return" isn't necessarily something that means it needs to have been fully initialized.
Quoted 5 lines
True.
Either way, I'm guessing that UBSAN will pick up a lot of similar bugs and the output of that is probably a much smaller haystack to dig through than just "every"
kmalloc()call.
It could be possible to find the cases where most of the fields are initialized, but one is left out. This could be done for all the fields of a given type, or in general.
julia
Re: [PATCH] net/mac80211/agg-rx.c: fix use of uninitialised values
In reply to Julian CalabyIt's not the return where we should trigger the warning it's at the
rcu_assign_pointer(sta->ampdu_mlme.tid_rx[tid], tid_agg_rx);
line. That's for correctness, but also it should be slightly easier. Or it should cut down on false positives if we ignored returns and only looked global scope type assignements.
regards,
dan carpenter
Johannes Berg
maintainerRe: [PATCH] net/mac80211/agg-rx.c: fix use of uninitialised values
Quoted 7 lines · Dan Carpenter
It's not the return where we should trigger the warning it's at the
rcu_assign_pointer(sta->ampdu_mlme.tid_rx[tid], tid_agg_rx);line. That's for correctness, but also it should be slightly easier. Or it should cut down on false positives if we ignored returns and only looked global scope type assignements.
That's a good idea! But even that will probably get you a lot of false positives. For example, in this structure, the rcu_head is never initialized until we need it for kfree_rcu() or call_rcu(). I'm sure there are other places like it.
johannes
Re: [PATCH] net/mac80211/agg-rx.c: fix use of uninitialised values
In reply to Chris BainbridgeQuoted 2 lines · Chris Bainbridge
Use kzalloc instead of kmalloc for struct
tid_ampdu_rx. Fixes:
Applied, thanks.
johannes