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

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 22 Feb 2023 16:02:20 -0600
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Niklas Schnelle <schnelle@...ux.ibm.com>
Cc:     Gerd Bayer <gbayer@...ux.ibm.com>,
        Gerald Schaefer <gerald.schaefer@...ux.ibm.com>,
        Heiko Carstens <hca@...ux.ibm.com>,
        Vasily Gorbik <gor@...ux.ibm.com>,
        Alexander Gordeev <agordeev@...ux.ibm.com>,
        Christian Borntraeger <borntraeger@...ux.ibm.com>,
        Sven Schnelle <svens@...ux.ibm.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Pierre Morel <pmorel@...ux.ibm.com>,
        Matthew Rosato <mjrosato@...ux.ibm.com>,
        linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-pci@...r.kernel.org
Subject: Re: [PATCH RESEND] PCI: s390: Fix use-after-free of PCI bus
 resources with s390 per-function hotplug

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