netdev - Re: [PATCH] xfrm: use list_for_each_entry_safe in xfrm_policy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190701090345.fkd7lrecicrewpnt@breakpoint.cc>
Date:   Mon, 1 Jul 2019 11:03:45 +0200
From:   Florian Westphal <fw@...len.de>
To:     Li RongQing <lirongqing@...du.com>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH] xfrm: use list_for_each_entry_safe in xfrm_policy_flush

Li RongQing <lirongqing@...du.com> wrote:
> The iterated pol maybe be freed since it is not protected
> by RCU or spinlock when put it, lead to UAF, so use _safe
> function to iterate over it against removal
> 
> Signed-off-by: Li RongQing <lirongqing@...du.com>
> ---
>  net/xfrm/xfrm_policy.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index 3235562f6588..87d770dab1f5 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -1772,7 +1772,7 @@ xfrm_policy_flush_secctx_check(struct net *net, u8 type, bool task_valid)
>  int xfrm_policy_flush(struct net *net, u8 type, bool task_valid)
>  {
>  	int dir, err = 0, cnt = 0;
> -	struct xfrm_policy *pol;
> +	struct xfrm_policy *pol, *tmp;
>  
>  	spin_lock_bh(&net->xfrm.xfrm_policy_lock);
>  
> @@ -1781,7 +1781,7 @@ int xfrm_policy_flush(struct net *net, u8 type, bool task_valid)
>  		goto out;
>  
>  again:
> -	list_for_each_entry(pol, &net->xfrm.policy_all, walk.all) {
> +	list_for_each_entry_safe(pol, tmp, &net->xfrm.policy_all, walk.all) {
>  		dir = xfrm_policy_id2dir(pol->index);
>  		if (pol->walk.dead ||
>  		    dir >= XFRM_POLICY_MAX ||

This function drops the lock, but after re-acquire jumps to the 'again'
label, so I do not see the UAF as the entire loop gets restarted.