Re: [PATCH] net/mac80211/agg-rx.c: fix use of uninitialised values

Re: [PATCH] net/mac80211/agg-rx.c: fix use of uninitialised values

[Julian Calaby]

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,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [PATCH] net/mac80211/agg-rx.c: fix use of uninitialised values

[Johannes Berg]

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 kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] net/mac80211/agg-rx.c: fix use of uninitialised values

[Julian Calaby]

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,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [PATCH] net/mac80211/agg-rx.c: fix use of uninitialised values

[Julia Lawall]

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

Re: [PATCH] net/mac80211/agg-rx.c: fix use of uninitialised values

[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.

regards,
dan carpenter

Re: [PATCH] net/mac80211/agg-rx.c: fix use of uninitialised values

[Johannes Berg]

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