netdev - Re: [PATCH] net: frag, fix race conditions in LRU list maintenance

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: <20130506093318.679f5557@redhat.com>
Date:	Mon, 6 May 2013 09:33:18 +0200
From:	Jesper Dangaard Brouer <brouer@...hat.com>
To:	Konstantin Khlebnikov <khlebnikov@...nvz.org>
Cc:	netdev@...r.kernel.org, Eric Dumazet <edumazet@...gle.com>,
	"David S. Miller" <davem@...emloft.net>,
	Florian Westphal <fw@...len.de>, linux-kernel@...r.kernel.org,
	Jesper Dangaard Brouer <brouer@...hat.com>
Subject: Re: [PATCH] net: frag, fix race conditions in LRU list maintenance

On Sun, 05 May 2013 18:56:22 +0400
Konstantin Khlebnikov <khlebnikov@...nvz.org> wrote:

> This patch fixes race between inet_frag_lru_move() and
> inet_frag_lru_add() which was introduced in commit
> 3ef0eb0db4bf92c6d2510fe5c4dc51852746f206 ("net: frag, move LRU list
> maintenance outside of rwlock")
> 
> One cpu already added new fragment queue into hash but not into LRU.
> Other cpu found it in hash and tries to move it to the end of LRU.
> This leads to NULL pointer dereference inside of list_move_tail().

I tried to address this in:
http://article.gmane.org/gmane.linux.network/266324 But it was never
applied, as we disagreed on other parts of the patchset and didn't think
this would happen with curr LRU scheme (which you have shown it can)


> Another possible race condition is between inet_frag_lru_move() and
> inet_frag_lru_del(): move can happens after deletion.

This should be very difficult to hit, because its protected with
the INET_FRAG_COMPLETE bit (checked in ip_frag_queue() before the LRU
move), which is set after removing it from LRU (all while protected
under the q->lock).


> This patch initializes LRU list head before adding fragment into hash
> and inet_frag_lru_move() doesn't touches it if it's empty.

This a good fix, and better than my earlier attempt at fixing this.

Thanks!

Signed-off-by: Jesper Dangaard Brouer <brouer@...hat.com>
--
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