Re: [PATCH net] net: openvswitch: fix flow mask use-after-free on flow deletion

[Aaron Conole <aconole@redhat.com>]

Ilya Maximets <i.maximets@ovn.org> writes:

> The commit in the Fixes tag below made so flow->mask free is scheduled
> via RCU right after it is removed from the flow table.  The pointer
> stays in the flow structure and it can be accessible while in the same
> RCU critical section.  This is done to avoid requiring ovs_mutex for
> the ovs_flow_free().
>
> However, while removing the flow during processing of CMD_DEL, we do
> not take RCU read lock before the removal, and ovs_flow_cmd_fill_info()
> uses the flow->mask pointer afterwards.  The RCU read lock is taken,
> but it's already late at that point.  The comment on that line
> acknowledges that the lock is cosmetic and doesn't serve a real purpose.
>
> This leads to use-after-free if the RCU grace period passes between
> removal and the filling.  It is a short race window, but it is there
> and can lead to a real crash in case memory allocation for the info
> takes a bit longer:
>
>  BUG: KASAN: slab-use-after-free in __ovs_nla_put_key
>              net/openvswitch/flow_netlink.c:1996
>  BUG: KASAN: slab-use-after-free in ovs_nla_put_key+0x2463/0x2e30
>              net/openvswitch/flow_netlink.c:2250
>  Read of size 4 at addr ffff88801ee89970 by task ovs_flow_del_ec/9487
>
>  Call Trace:
>   <TASK>
>   __ovs_nla_put_key net/openvswitch/flow_netlink.c:1996
>   ovs_nla_put_key+0x2463/0x2e30 net/openvswitch/flow_netlink.c:2250
>   ovs_flow_cmd_fill_info+0x420/0x9c0 net/openvswitch/datapath.c:930
>   ovs_flow_cmd_del+0x53a/0x970 net/openvswitch/datapath.c:1467
>   ...
>   netlink_rcv_skb+0x156/0x420 net/netlink/af_netlink.c:2556
>   </TASK>
>
>  Allocated by task 9487:
>   mask_alloc net/openvswitch/flow_table.c:967
>   flow_mask_insert net/openvswitch/flow_table.c:1012
>   ovs_flow_tbl_insert+0xea2/0x1a90 net/openvswitch/flow_table.c:1084
>   ovs_flow_cmd_new+0x7e3/0xd90 net/openvswitch/datapath.c:1086
>   ...
>   netlink_rcv_skb+0x156/0x420 net/netlink/af_netlink.c:2556
>
>  Freed by task 9485:
>   rcu_free_sheaf+0x1e/0x100 mm/slub.c:5978
>   rcu_do_batch kernel/rcu/tree.c:2645
>   rcu_core+0x59c/0x10c0 kernel/rcu/tree.c:2897
>   handle_softirqs+0x1e4/0x9a0 kernel/softirq.c:622
>   ...
>   instr_sysvec_apic_timer_interrupt arch/x86/kernel/apic/apic.c:1062
>
> ovs_flow_tbl_remove() must be called after the ovs_flow_cmd_fill_info()
> to avoid this race.  This also helps with cleaning up the forced cast
> and the cosmetic RCU read lock.  Before the commit in the Fixes tag the
> order did not matter as long as the flow object itself was not freed.
>
> A wider RCU critical section could be another option, but we have a
> GFP_KERNEL allocation in the way.
>
> Reported by Trend Micro's Zero Day Initiative as ZDI-CAN-32042.
>
> Fixes: 56c19868e115 ("openvswitch: Make flow mask removal symmetric.")
> Cc: stable@vger.kernel.org
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> ---

Reviewed-by: Aaron Conole <aconole@redhat.com>