netdev - Re: [PATCH] neighbour: Prevent a dead entry from updating gc

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: <20210125135924.6baa37c9@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Mon, 25 Jan 2021 13:59:24 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Chinmay Agarwal <chinagar@...eaurora.org>
Cc:     netdev@...r.kernel.org, xiyou.wangcong@...il.com,
        sharathv@...eaurora.org
Subject: Re: [PATCH] neighbour: Prevent a dead entry from updating gc_list

On Tue, 26 Jan 2021 01:29:37 +0530 Chinmay Agarwal wrote:
> Following race condition was detected:
> <CPU A, t0> - neigh_flush_dev() is under execution and calls neigh_mark_dead(n),
> marking the neighbour entry 'n' as dead.
> 
> <CPU B, t1> - Executing: __netif_receive_skb() -> __netif_receive_skb_core()
> -> arp_rcv() -> arp_process().arp_process() calls __neigh_lookup() which takes  
> a reference on neighbour entry 'n'.
> 
> <CPU A, t2> - Moves further along neigh_flush_dev() and calls
> neigh_cleanup_and_release(n), but since reference count increased in t2,
> 'n' couldn't be destroyed.
> 
> <CPU B, t3> - Moves further along, arp_process() and calls
> neigh_update()-> __neigh_update() -> neigh_update_gc_list(), which adds
> the neighbour entry back in gc_list(neigh_mark_dead(), removed it
> earlier in t0 from gc_list)
> 
> <CPU B, t4> - arp_process() finally calls neigh_release(n), destroying
> the neighbour entry.
> 
> This leads to 'n' still being part of gc_list, but the actual
> neighbour structure has been freed.
> 
> The situation can be prevented from happening if we disallow a dead
> entry to have any possibility of updating gc_list. This is what the
> patch intends to achieve.
> 
> Signed-off-by: Chinmay Agarwal <chinagar@...eaurora.org>
> ---
>  net/core/neighbour.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index ff07358..cf8e3076 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -1244,13 +1244,14 @@ static int __neigh_update(struct neighbour *neigh, const u8 *lladdr,
>  	old    = neigh->nud_state;
>  	err    = -EPERM;
>  
> -	if (!(flags & NEIGH_UPDATE_F_ADMIN) &&
> -	    (old & (NUD_NOARP | NUD_PERMANENT)))
> -		goto out;
>  	if (neigh->dead) {
>  		NL_SET_ERR_MSG(extack, "Neighbor entry is now dead");
> +		new=old;
>  		goto out;
>  	}
> +	if (!(flags & NEIGH_UPDATE_F_ADMIN) &&
> +	    (old & (NUD_NOARP | NUD_PERMANENT)))
> +		goto out;
>  
>  	ext_learn_change = neigh_update_ext_learned(neigh, flags, &notify);
>  

Please run checkpatch on your patches:

ERROR: spaces required around that '=' (ctx:VxV)
#52: FILE: net/core/neighbour.c:1249:
+		new=old;
 		   ^