Re: [PATCH net v2] net/sched: Fix UAF when resolving a clash

[Michal Kubiak <michal.kubiak@intel.com>]

On Fri, Jul 05, 2024 at 10:50:56AM +0800, Chengen Du wrote:
> KASAN reports the following UAF:
> 
>  BUG: KASAN: slab-use-after-free in tcf_ct_flow_table_process_conn+0x12b/0x380 [act_ct]
>  Read of size 1 at addr ffff888c07603600 by task handler130/6469
> 
>  Call Trace:
>   <IRQ>
>   dump_stack_lvl+0x48/0x70
>   print_address_description.constprop.0+0x33/0x3d0
>   print_report+0xc0/0x2b0
>   kasan_report+0xd0/0x120
>   __asan_load1+0x6c/0x80
>   tcf_ct_flow_table_process_conn+0x12b/0x380 [act_ct]
>   tcf_ct_act+0x886/0x1350 [act_ct]
>   tcf_action_exec+0xf8/0x1f0
>   fl_classify+0x355/0x360 [cls_flower]
>   __tcf_classify+0x1fd/0x330
>   tcf_classify+0x21c/0x3c0
>   sch_handle_ingress.constprop.0+0x2c5/0x500
>   __netif_receive_skb_core.constprop.0+0xb25/0x1510
>   __netif_receive_skb_list_core+0x220/0x4c0
>   netif_receive_skb_list_internal+0x446/0x620
>   napi_complete_done+0x157/0x3d0
>   gro_cell_poll+0xcf/0x100
>   __napi_poll+0x65/0x310
>   net_rx_action+0x30c/0x5c0
>   __do_softirq+0x14f/0x491
>   __irq_exit_rcu+0x82/0xc0
>   irq_exit_rcu+0xe/0x20
>   common_interrupt+0xa1/0xb0
>   </IRQ>
>   <TASK>
>   asm_common_interrupt+0x27/0x40
> 
>  Allocated by task 6469:
>   kasan_save_stack+0x38/0x70
>   kasan_set_track+0x25/0x40
>   kasan_save_alloc_info+0x1e/0x40
>   __kasan_krealloc+0x133/0x190
>   krealloc+0xaa/0x130
>   nf_ct_ext_add+0xed/0x230 [nf_conntrack]
>   tcf_ct_act+0x1095/0x1350 [act_ct]
>   tcf_action_exec+0xf8/0x1f0
>   fl_classify+0x355/0x360 [cls_flower]
>   __tcf_classify+0x1fd/0x330
>   tcf_classify+0x21c/0x3c0
>   sch_handle_ingress.constprop.0+0x2c5/0x500
>   __netif_receive_skb_core.constprop.0+0xb25/0x1510
>   __netif_receive_skb_list_core+0x220/0x4c0
>   netif_receive_skb_list_internal+0x446/0x620
>   napi_complete_done+0x157/0x3d0
>   gro_cell_poll+0xcf/0x100
>   __napi_poll+0x65/0x310
>   net_rx_action+0x30c/0x5c0
>   __do_softirq+0x14f/0x491
> 
>  Freed by task 6469:
>   kasan_save_stack+0x38/0x70
>   kasan_set_track+0x25/0x40
>   kasan_save_free_info+0x2b/0x60
>   ____kasan_slab_free+0x180/0x1f0
>   __kasan_slab_free+0x12/0x30
>   slab_free_freelist_hook+0xd2/0x1a0
>   __kmem_cache_free+0x1a2/0x2f0
>   kfree+0x78/0x120
>   nf_conntrack_free+0x74/0x130 [nf_conntrack]
>   nf_ct_destroy+0xb2/0x140 [nf_conntrack]
>   __nf_ct_resolve_clash+0x529/0x5d0 [nf_conntrack]
>   nf_ct_resolve_clash+0xf6/0x490 [nf_conntrack]
>   __nf_conntrack_confirm+0x2c6/0x770 [nf_conntrack]
>   tcf_ct_act+0x12ad/0x1350 [act_ct]
>   tcf_action_exec+0xf8/0x1f0
>   fl_classify+0x355/0x360 [cls_flower]
>   __tcf_classify+0x1fd/0x330
>   tcf_classify+0x21c/0x3c0
>   sch_handle_ingress.constprop.0+0x2c5/0x500
>   __netif_receive_skb_core.constprop.0+0xb25/0x1510
>   __netif_receive_skb_list_core+0x220/0x4c0
>   netif_receive_skb_list_internal+0x446/0x620
>   napi_complete_done+0x157/0x3d0
>   gro_cell_poll+0xcf/0x100
>   __napi_poll+0x65/0x310
>   net_rx_action+0x30c/0x5c0
>   __do_softirq+0x14f/0x491
> 
> The ct may be dropped if a clash has been resolved but is still passed to
> the tcf_ct_flow_table_process_conn function for further usage. This issue
> can be fixed by retrieving ct from skb again after confirming conntrack.
> 
> Fixes: 0cc254e5aa37 ("net/sched: act_ct: Offload connections with commit action")
> Co-developed-by: Gerald Yang <gerald.yang@canonical.com>
> Signed-off-by: Gerald Yang <gerald.yang@canonical.com>
> Signed-off-by: Chengen Du <chengen.du@canonical.com>
> ---
>  net/sched/act_ct.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> index 2a96d9c1db65..6f41796115e3 100644
> --- a/net/sched/act_ct.c
> +++ b/net/sched/act_ct.c
> @@ -1077,6 +1077,14 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
>  		 */
>  		if (nf_conntrack_confirm(skb) != NF_ACCEPT)
>  			goto drop;
> +
> +		/* The ct may be dropped if a clash has been resolved,
> +		 * so it's necessary to retrieve it from skb again to
> +		 * prevent UAF.
> +		 */
> +		ct = nf_ct_get(skb, &ctinfo);
> +		if (!ct)
> +			goto drop;

After taking a closer look at this change, I have a question: Why do we
need to change an action returned by "nf_conntrack_confirm()"
(NF_ACCEPT) and actually perform the flow for NF_DROP?

From the commit message I understand that you only want to prevent
calling "tcf_ct_flow_table_process_conn()". But for such reason we have
a bool variable: "skip_add".
Shouldn't we just set "skip_add" to true to prevent the UAF?
Would the following example code make sense in this case?

	ct = nf_ct_get(skb, &ctinfo);
	if (!ct)
		skip_add = true;



>  	}
>  
>  	if (!skip_add)
> -- 
> 2.43.0
> 
> 

Thanks,
Michal