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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ