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] ================================================================================
On Wed, 2016-01-27 at 15:46 +0000, Chris Bainbridge wrote:
> Use kzalloc instead of kmalloc for struct tid_ampdu_rx. Fixes:
>
Applied, thanks.
johannes
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
Link: https://lkml.org/lkml/2016/1/26/230
Signed-off-by: Chris Bainbridge <[email protected]>
---
net/mac80211/agg-rx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
index 10ad4ac1fa0b..bde3344cbdd0 100644
--- a/net/mac80211/agg-rx.c
+++ b/net/mac80211/agg-rx.c
@@ -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;
--
2.1.4
On Thu, 2016-01-28 at 15:30 +0300, Dan Carpenter wrote:
> 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
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.
regards,
dan carpenter
On Thu, 28 Jan 2016, Julian Calaby wrote:
> Hi Johannes,
>
> On Thu, Jan 28, 2016 at 8:48 PM, Johannes Berg
> <[email protected]> wrote:
> > On Thu, 2016-01-28 at 10:27 +1100, Julian Calaby wrote:
> >> 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.
>
> >> 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.
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
Hi Johannes,
On Thu, Jan 28, 2016 at 8:48 PM, Johannes Berg
<[email protected]> wrote:
> On Thu, 2016-01-28 at 10:27 +1100, Julian Calaby wrote:
>> 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.
>> 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,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
On Thu, 2016-01-28 at 10:27 +1100, Julian Calaby wrote:
>
> This looks like a "big hammer" solution to this problem.
It is, in a way, but it also avoids future errors like it.
> 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.
> 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
Hi Chris,
On Thu, Jan 28, 2016 at 2:46 AM, Chris Bainbridge
<[email protected]> wrote:
> 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
>
> Link: https://lkml.org/lkml/2016/1/26/230
> Signed-off-by: Chris Bainbridge <[email protected]>
> ---
> net/mac80211/agg-rx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
> index 10ad4ac1fa0b..bde3344cbdd0 100644
> --- a/net/mac80211/agg-rx.c
> +++ b/net/mac80211/agg-rx.c
> @@ -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.
Reviewed-by: Julian Calaby <[email protected]>
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,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/