netdev - Re: [PATCH net] net: neighbour: add neighbour dead check for neigh_timer

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]

Date:	Tue, 03 Dec 2013 11:37:37 -0500 (EST)
From:	David Miller <davem@...emloft.net>
To:	dingtianhong@...wei.com
Cc:	gaofeng@...fujitsu.com, yoshfuji@...ux-ipv6.org, joe@...ches.com,
	vfalico@...hat.com, netdev@...r.kernel.org
Subject: Re: [PATCH net] net: neighbour: add neighbour dead check for
 neigh_timer_handler()

From: Ding Tianhong <dingtianhong@...wei.com>
Date: Tue, 3 Dec 2013 21:48:45 +0800

> @@ -888,6 +888,11 @@ static void neigh_timer_handler(unsigned long arg)
>  
>  	write_lock(&neigh->lock);
>  
> +	if (neigh->dead) {
> +		pr_warn("neighbour is dead and should be destroyed\n");
> +		goto out;
> +	}
> +

I agree with Eric, if the neigh has been freed at this point you
cannot touch any member of it.

This means you cannot take neigh->lock.

This means you can't even test neigh->dead because it could be
arbitrary garbage, it's freed memory after all.

Worse yet, the neigh->timer is implicitly referenced by the timer
subsystem, the caller of neigh_timer_handler().  It is referencing
freed memory if it is touching neigh->timer members at all.

You can't avoid things by pretending that you can leave this timer
pending or potentially running asynchronously.

You must guarentee that all pending timer instances are complete
and no longer scheduled before you allow kfree(neigh) to execute.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html