Re: [PATCH RESEND] PCI: s390: Fix use-after-free of PCI bus resources with s390 per-function hotplug

[Bjorn Helgaas <helgaas@kernel.org>]

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();