netdev - Re: NULL pointer dereference in veth_stats

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:	Fri, 4 Jan 2013 20:25:12 +0000
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	Eric Dumazet <erdnetdev@...il.com>
CC:	Tom Parkin <tparkin@...alix.com>,
	David Miller <davem@...emloft.net>,
	netdev <netdev@...r.kernel.org>
Subject: Re: NULL pointer dereference in veth_stats_one

On Fri, 2013-01-04 at 11:23 -0800, Eric Dumazet wrote:
> On Fri, 2013-01-04 at 18:17 +0000, Ben Hutchings wrote:
> 
> > This possibly needs some memory barriers to properly synchronise with
> > veth_newlink().  But can you not move initialisation of the peer
> > pointers before registration of the devices in veth_newlink(), so that
> > veth_get_stats64() cannot be called before they are initialised?
> 
> The ->peer pointer cannot change once set. ( its never cleared )

We may still need an explicit barrier for data-dependency.

> So the problem would not be in veth_newlink(), but might be in
> veth_dellink()

A lot of things are done in between the unregister_netdevice_queue() and
the actual deletion which are probably sufficient to flush out any calls
to dev_get_stats().  But to make sure, I think we would need some small
amount of shared state that isn't freed until both devices are.

> It seems we would have a problem in veth_get_ethtool_stats() already...

That should be OK because both ethtool operations and the whole process
of interface deletion are serialised by the RTNL lock.

> More generally, what prevents a get_stats() being called while a
> dellink() (-> veth_dev_free() -> free_percpu()) is done ?

Anything calling dev_get_stats() must have a counted or RCU reference to
the device, and netdev_run_todo() waits for those to go away.  For
mutually referencing devices we want a kind of weak reference and we
have no good way to implement those.

Ben.

> (Same thing is done for tunnel/dummy stats percpu data)

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--
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