Re: [PATCH net] rtnetlink: fix struct net reference leak

On Fri, Dec 22, 2017 at 8:59 AM, Craig Gallek [off-list ref] wrote:

On Fri, Dec 22, 2017 at 3:11 AM, Nicolas Dichtel
[off-list ref] wrote:

quoted

Le 21/12/2017 à 23:18, Craig Gallek a écrit :

quoted

From: Craig Gallek <redacted>

The below referenced commit extended the RTM_GETLINK interface to
allow querying by netns id.  The netnsid property was previously
defined as a signed integer, but this patch assumes that the user
always passes a positive integer.  syzkaller discovered this problem
by setting a negative netnsid and then calling the get-link path
in a tight loop.  This surprisingly quickly overflows the reference
count on the associated struct net, potentially destroying it.  When the
default namespace is used, the machine crashes in strange and interesting
ways.

Unfortunately, this is not easy to reproduce with just the ip tool
as it enforces unsigned integer parsing despite the interface interpeting
the NETNSID attribute as signed.

I'm not sure why this attribute is signed in the first place, but
the first commit that introduced it (6621dd29eb9b) is in v4.15-rc4,
so I assume it's too late to change.

A valid (assigned) nsid is always >= 0.

quoted

This patch removes the positive netns id assumption, but adds another
assumption that the netns id 0 is always the 'self' identifying id (for
which an additional struct net reference is not necessary).

We cannot make this assumption, this is wrong. nsids may be automatically
allocated by the kernel, and it starts by 0.
The current netns can be identify by NETNSA_NSID_NOT_ASSIGNED, ie -1.

Thank you, I'll respin this with NETNSA_NSID_NOT_ASSIGNED as the sentinel value.

Looking at the netns id code more closely, there are several places
that assume ids will never be zero (short of the sentinel).  I think
the only simple fix here is to update the netlink interfaces to not
accept negative values as input.  I'm going to send that patch
instead...