linux-kernel - Re: [PATCH v2 04/10] PCI: Destroy pci dev only once

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: <20131208035047.GB17629@kroah.com>
Date:	Sat, 7 Dec 2013 19:50:47 -0800
From:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:	Yinghai Lu <yinghai@...nel.org>
Cc:	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
	Gu Zheng <guz.fnst@...fujitsu.com>,
	Guo Chao <yan@...ux.vnet.ibm.com>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Mika Westerberg <mika.westerberg@...ux.intel.com>,
	Myron Stowe <myron.stowe@...il.com>
Subject: Re: [PATCH v2 04/10] PCI: Destroy pci dev only once

On Sat, Dec 07, 2013 at 07:31:21PM -0800, Yinghai Lu wrote:
> [+ GregKH]
> 
> On Fri, Dec 6, 2013 at 5:27 PM, Rafael J. Wysocki <rjw@...ysocki.net> wrote:
> > On Thursday, December 05, 2013 10:52:36 PM Yinghai Lu wrote:
> >> On Mon, Dec 2, 2013 at 6:49 AM, Rafael J. Wysocki <rjw@...ysocki.net> wrote:
> >> >
> >> > Scenario 5: pci_stop_and_remove_bus_device() is run concurrently
> >> >   for a device and its parent bridge via remove_callback().
> >> >
> >> >   In that case both code paths attempt to acquire
> >> >   pci_remove_rescan_mutex.  If the child device removal acquires
> >> >   it first, there will be no problems.  However, if the parent
> >> >   bridge removal acquires it first, it will eventually execute
> >> >   pci_destroy_dev() for the child device, but that device will
> >> >   not be freed yet due to the reference held by the concurrent
> >> >   child removal.  Consequently, both pci_stop_bus_device() and
> >> >   pci_remove_bus_device() will be executed for that device
> >> >   unnecessarily and pci_destroy_dev() will see a corrupted list
> >> >   head in that object.  Moreover, an excess put_device() will
> >> >   be executed for that device in that case which may lead to a
> >> >   use-after-free in the final kobject_put() done by
> >> >   sysfs_schedule_callback_work().
> >> >
> >> > Index: linux-pm/include/linux/pci.h
> >> > ===================================================================
> >> > --- linux-pm.orig/include/linux/pci.h
> >> > +++ linux-pm/include/linux/pci.h
> >> > @@ -321,6 +321,7 @@ struct pci_dev {
> >> >         unsigned int    multifunction:1;/* Part of multi-function device */
> >> >         /* keep track of device state */
> >> >         unsigned int    is_added:1;
> >> > +       unsigned int    is_gone:1;
> >> >         unsigned int    is_busmaster:1; /* device is busmaster */
> >> >         unsigned int    no_msi:1;       /* device may not use msi */
> >> >         unsigned int    block_cfg_access:1;     /* config space access is blocked */
> >> > Index: linux-pm/drivers/pci/remove.c
> >> > ===================================================================
> >> > --- linux-pm.orig/drivers/pci/remove.c
> >> > +++ linux-pm/drivers/pci/remove.c
> >> > @@ -34,6 +34,7 @@ static void pci_stop_dev(struct pci_dev
> >> >
> >> >  static void pci_destroy_dev(struct pci_dev *dev)
> >> >  {
> >> > +       dev->is_gone = 1;
> >> >         device_del(&dev->dev);
> >> >
> >> >         down_write(&pci_bus_sem);
> >> > @@ -109,8 +110,10 @@ static void pci_remove_bus_device(struct
> >> >   */
> >> >  void pci_stop_and_remove_bus_device(struct pci_dev *dev)
> >> >  {
> >> > -       pci_stop_bus_device(dev);
> >> > -       pci_remove_bus_device(dev);
> >> > +       if (!dev->is_gone) {
> >> > +               pci_stop_bus_device(dev);
> >> > +               pci_remove_bus_device(dev);
> >> > +       }
> >> >  }
> >> >  EXPORT_SYMBOL(pci_stop_and_remove_bus_device);
> >> >
> >>
> >> Yes, above change should address sys double remove problem.
> >
> > I've just realized that we don't need a new flag for that, though.
> >
> > It looks like we only need to check dev->dev.kobj.parent and return if that is
> > NULL, because that means pci_destroy_dev() has run for that device already
> > (I'm wondering why device_del() doesn't clear dev->parent, BTW, it looks like
> > it should do that?).
> >
> > Of course, that still is going to be racy if we don't hold
> > pci_remove_rescan_mutex around pci_stop_and_remove_bus_device() in every code
> > path using it (or use another similar synchronization mechanism).
> 
> Wonder if we can have safe way to check if device_del() is called already.

Nope.

> And those access_after_free should be addressed by driver core instead
> of pci code?

Nope, it's up to the bus to handle this.  It shouldn't be hard, you
shouldn't actually care about this, if you do, something is wrong.

How is this PCI code so hard to get right?  Look at USB for devices that
disappear from anywhere at anytime as an example for how to handle
this.  PCI should be doing the same thing, no need for this "is_gone"
stuff.

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/