| Message ID | 20160127154618.GA5717@localhost (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Johannes Berg |
| Headers | show |
Hi Chris, On Thu, Jan 28, 2016 at 2:46 AM, Chris Bainbridge <chris.bainbridge@gmail.com> 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 <chris.bainbridge@gmail.com> > --- > 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 <julian.calaby@gmail.com> 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,
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Johannes, On Thu, Jan 28, 2016 at 8:48 PM, Johannes Berg <johannes@sipsolutions.net> 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,
On Thu, 28 Jan 2016, Julian Calaby wrote: > Hi Johannes, > > On Thu, Jan 28, 2016 at 8:48 PM, Johannes Berg > <johannes@sipsolutions.net> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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 <chris.bainbridge@gmail.com> --- net/mac80211/agg-rx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)