Message-ID: <3a41947fe19bfd655cecd7f6573ce3c3b4cb2a56.camel@linux.ibm.com>
Subject: Re: [PATCH RESEND] PCI: s390: Fix use-after-free of PCI bus
 resources with s390 per-function hotplug
From:   Niklas Schnelle <schnelle@linux.ibm.com>
To:     Bjorn Helgaas <helgaas@kernel.org>
Cc:     Gerd Bayer <gbayer@linux.ibm.com>,
        Gerald Schaefer <gerald.schaefer@linux.ibm.com>,
        Heiko Carstens <hca@linux.ibm.com>,
        Vasily Gorbik <gor@linux.ibm.com>,
        Alexander Gordeev <agordeev@linux.ibm.com>,
        Christian Borntraeger <borntraeger@linux.ibm.com>,
        Sven Schnelle <svens@linux.ibm.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Pierre Morel <pmorel@linux.ibm.com>,
        Matthew Rosato <mjrosato@linux.ibm.com>,
        linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-pci@vger.kernel.org
Date:   Thu, 23 Feb 2023 12:22:06 +0100
In-Reply-To: <20230222220220.GA3804275@bhelgaas>
References: <20230222220220.GA3804275@bhelgaas>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4 (3.46.4-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

On Wed, 2023-02-22 at 16:02 -0600, Bjorn Helgaas wrote:
> On Mon, Feb 20, 2023 at 01:53:34PM +0100, Niklas Schnelle wrote:
> > On Fri, 2023-02-17 at 17:15 -0600, Bjorn Helgaas wrote:
> > > On Tue, Feb 14, 2023 at 10:49:10AM +0100, Niklas Schnelle wrote:
> > > > ...
>=20
> > >     What happens when zpci_bus_release() calls
> > >     pci_free_resource_list() on &zbus->resources?  It looks like that
> > >     ultimately calls kfree(), which is OK for the
> > >     zpci_setup_bus_resources() stuff, but what about the
> > >     zbus->bus_resource that was not kalloc'ed?
> >=20
> > As far as I can see pci_free_resource_list() only calls kfree() on the
> > entry not on entry->res. The resources set up in
> > zpci_setup_bus_resources() are freed in zpci_cleanup_bus_resources()
> > explicitly.
>=20
> So I guess the zbus->resources are allocated in zpci_bus_scan_device()
> where zpci_setup_bus_resources() adds a zbus resource for every
> zpci_dev BAR, and freed in zpci_bus_release() when the last zpci_dev
> is unregistered.

Only the zbus->resources list itself is freed in zpci_bus_release() the
resources for the BARs of each zpci_dev are freed in
zpci_cleanup_bus_resources() when the individual zpci_dev is removed.

>=20
> Does that mean that if you add device A, add device B, and remove A,
> the zbus retains A's resources even though A is gone?  What if you
> then add device C whose resources partially overlap A's?
>=20
> > >=20

No. Prior to the proposed patch pci_bus::resources/pci_bus::resource[]
and zpci_bus::resources would retain dangling pointers to the BAR
resources of A while the BAR resource itself was already freed when A
is removed. This is the use-after-free this patch is trying to fix.

The BAR resource freeing happens when zpci_cleanup_bus_resources() is
called in zpci_release_device() once the reference count for the struct
zpci_dev hits 0. With this patch this stays the same but we're a)
removing the resource pointer from pci_bus::resources /
pci_bus::resource[] and never adding it to zpci_bus::resources. Meaning
with this patch zpci_bus::resources doesn't store BAR resources at all
and is only used for the IORESOURCE_BUS the BAR resources are instead
only referenced by zpci_dev::bars[bar_num].res and pci_bus::resources /
pci_bus::resource[i].

I don't think we can get overlapping resources but this way the
resource structs are freed when the device (PCI function) goes away
which is also when they become inaccessible for our PCI load/store
instructions.