Date:   Wed, 22 Feb 2023 16:02:20 -0600
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     Niklas Schnelle <schnelle@linux.ibm.com>
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
Subject: Re: [PATCH RESEND] PCI: s390: Fix use-after-free of PCI bus
 resources with s390 per-function hotplug
Message-ID: <20230222220220.GA3804275@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1a621a2b836d81d12b6f265f47d93b827e0a82df.camel@linux.ibm.com>
Precedence: bulk

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

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

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.

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?

> > >  static void zpci_cleanup_bus_resources(struct zpci_dev *zdev)
> > >  {
> > > +	struct resource *res;
> > >  	int i;
> > >  
> > > +	pci_lock_rescan_remove();
> > 
> > What exactly is this protecting?  This doesn't seem like quite the
> > right place since we're not adding/removing a pci_dev here.  Is this
> > to protect the bus->resources list in pci_bus_remove_resource()?
> 
> Yes I did not find a lock that is specifically for bus->resources but
> it seemed to me that changes to resources would only affect things
> running under the rescan/remove lock.

Yeah, OK.

> > >  	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> > > -		if (!zdev->bars[i].size || !zdev->bars[i].res)
> > > +		res = zdev->bars[i].res;
> > > +		if (!res)
> > >  			continue;
> > >  
> > > +		release_resource(res);
> > > +		pci_bus_remove_resource(zdev->zbus->bus, res);
> > >  		zpci_free_iomap(zdev, zdev->bars[i].map_idx);
> > > -		release_resource(zdev->bars[i].res);
> > > -		kfree(zdev->bars[i].res);
> > > +		zdev->bars[i].res = NULL;
> > > +		kfree(res);
> > >  	}
> > >  	zdev->has_resources = 0;
> > > +	pci_unlock_rescan_remove();