Re: [PATCH ipsec] xfrm: policy: use hlist_del_init_rcu in xfrm_hash_rebuild to avoid bydst poison

[Florian Westphal <fw@strlen.de>]

Xiang Mei (Microsoft) <xmei5@asu.edu> wrote:
> xfrm_hash_rebuild() unlinks each policy from its bydst chain with
> hlist_del_rcu() and re-inserts it. For an inexact policy the re-insert goes
> through xfrm_policy_inexact_insert(), which can fail on a GFP_ATOMIC
> allocation; on failure the error path only WARN_ONCE()s and continues, so the
> policy is left with a poisoned bydst node (LIST_POISON2). The next rebuild
> calls hlist_del_rcu() on that node again, dereferences the poison, and takes a
> general protection fault.
> 
> Use hlist_del_init_rcu() instead, so a failed-reinsert node is left unhashed
> (pprev == NULL) rather than poisoned. The next rebuild's hlist_del_init_rcu()
> is then a no-op for it, and the non-failing case is unchanged.
> 
> The reinsert allocation is GFP_ATOMIC (it runs under xfrm_policy_lock), so in
> practice this is only reached under memory pressure; the crash below was
> reproduced deterministically by forcing that allocation to fail with fault
> injection (failslab).
> 
> Crash:
>   Oops: general protection fault, probably for non-canonical address
>   0xfbd59c0000000024: 0000 [#1] SMP KASAN NOPTI
>   KASAN: maybe wild-memory-access in range [0xdead000000000120-0xdead000000000127]
>   ...
>   Workqueue: events xfrm_hash_rebuild
>   RIP: 0010:xfrm_hash_rebuild+0x5b3/0x1190
>   RAX: dead000000000122   (LIST_POISON2 + offset)
>   ...
>   Call Trace:
>    hlist_del_rcu (include/linux/rculist.h:599)
>    xfrm_hash_rebuild (net/xfrm/xfrm_policy.c:1365)
>    process_one_work (kernel/workqueue.c:3322)
>    worker_thread (kernel/workqueue.c:3486)
>    kthread (kernel/kthread.c:436)
>    ret_from_fork (arch/x86/kernel/process.c:158)
>    ret_from_fork_asm (arch/x86/entry/entry_64.S:245)
>    ...
>   Kernel panic - not syncing: Fatal exception in interrupt
> 
> Fixes: 563d5ca93e88 ("xfrm: switch migrate to xfrm_policy_lookup_bytype")
> Reported-by: AutonomousCodeSecurity@microsoft.com
> Signed-off-by: Xiang Mei (Microsoft) <xmei5@asu.edu>
> ---
>  net/xfrm/xfrm_policy.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index 7ef861a0e823..2612a405542b 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -1362,7 +1362,7 @@ static void xfrm_hash_rebuild(struct work_struct *work)
>  		if (xfrm_policy_is_dead_or_sk(policy))
>  			continue;
>  
> -		hlist_del_rcu(&policy->bydst);
> +		hlist_del_init_rcu(&policy->bydst);

This patch is dubious.  I looks to me as if it papers over the
actual bug.

Why is there a memory allocation error?

The first loop -- before unlink -- is supposed to preallocate the new
bins and chain heads.

This is also why there is a WARN. No memory allocations are supposed to
occur after the hlist_del_rcu(), there is supposed to be a guarantee
that the insertion succeeds.