netdev - Re: [PATCH] fix kernel crash in the macvlan driver

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] [day] [month] [year] [list]

Message-ID: <alpine.OSX.2.00.1206111840260.6713@animac.local>
Date:	Mon, 11 Jun 2012 18:50:40 -0700 (PDT)
From:	Ani Sinha <ani@...stanetworks.com>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
cc:	netdev@...r.kernel.org,
	Francesco Ruggeri <fruggeri@...stanetworks.com>
Subject: Re: [PATCH] fix kernel crash in the macvlan driver

Hi Eric :

I have found the reason for the crash :

On Thu, 7 Jun 2012, Eric W. Biederman wrote:

> >
> > The logic of my change is as follows :
> >
> > As far as I can see, macvlan_newlink() pairs with macvlan_dellink(). If
> > you are incrementing the reference count in newlink(), the corresponding
> > decrement should be, in my opinion in dellink(). If you are derementing
> > the count in uninit(), you are asuming that for every dellink() call,
> > there is a corresponding uninit() call. I am not sure if this assumption
> > is correct. Perhaps you can shed some more lights on this.
>
> Yes.  Look at net/core/dev.c
> dellink calls unregister_netdevice_queue.
> The active part of unregister_netdevice_queue rollback_registered_many
> calls dev->ndo_stop() and then ndo_uninit.

You are correct with respect the current upstream code. However, please
take a look at the commit : 0696c3a8acd3b7c3186dd231d65d97e05a75189f

Before this change was committed, a failure in dev_get_valid_name() would
cause netdev_ops->ndo_uninit() to get called as a part of the clneaup
code. This in turn will mess up (decrement) the port->count values causing
an unbalanced reference cound decrement. This was the reason why the port
was getting freed and a use after free resulted in the crash. I have
verified this was indeed the case using printks. After applying the fix in
0696c3a8acd3b7c3186dd231d65d97e05a75189f  along with your original
ref-count on dev->port fix, I can no longer reproduce the crash.

When moved the counter decrement from uninit() to dellink(), this fixed
the issue since although we had an extra unbalanced uninit() call, it
wasn't modifying the counter values. However ...

>
> We might still be using rcu hash lookups until ndo_close is called and
> so we really don't want to move the decrement before then.

I agree with you on this one. Hence, we should keep the reference counter
modification code where it currently is.

So there was a window when you had applied your patch (port reference
counting) and Peter's fix did not go in. In that period, the upstream
kernel was broken as well. Fortunately, it's all fixed now :)

Cheers,
Ani

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