On Wed, 25 Sep 2024, Wassenberg, Dennis wrote: > On Tue, 2024-09-24 at 13:51 +0300, Ilpo Järvinen wrote: > > On Mon, 23 Sep 2024, Wassenberg, Dennis wrote: > > > > > Hi together, > > > > > > we did some further analysis on this: > > > > > > Because we are working on kernel 6.8.12, I will use some logs from this kernel version, just for demonstration. The > > > initial report was based on 6.11. > > > > > > After we tried a KASAN build (dmesg-ramoops-kasan) it looks like it is exactly the same pciehp flow which leads to > > > the > > > UAF. > > > Both going through pciehp_ist -> pciehp_disable_slot -> pciehp_unconfigure_device -> pci_remove_bus_device -> ... > > > This means there are two consecutive interrupts, running on CPU 12 and both will execute the same flow. > > > At the latest the pci_lock_rescan_remove should be taken in pciehp_unconfigure_device to prevent accessing the > > > pci/bus > > > structures in parallel. > > > > > > I had a look if there are shared data structures accessed in this code path: > > > For me the access to "*parent = ctrl->pcie->port->subordinate;" looks fishy in pciehp_unconfigure_device. The parent > > > ptr > > > will be obtained before getting the lock (pci_lock_rescan_remove). Now, if there are two concurrent/consecutive > > > flows > > > come into this function, both will get the pointer to the parent bridge/subordinate. One thread will enter the lock > > > and > > > the other one is waiting until the lock is gone. The thread which enters the lock at first will completely remove > > > the > > > bridge and the subordinate: pciehp_unconfigure_device -> pci_stop_and_remove_bus_device -> pci_remove_bus_device -> > > > pci_destroy_dev: This will destroy the pci_dev and the subordinate is a part the this structure as well. Now > > > everything > > > is gone below this pci_bus (childs included). In pci_remove_bus_device there is a loop which iterates over all child > > > devices and call pci_remove_bus_device again. This means even the child bridges of the current bridge will be > > > deleted. > > > In the end: everything is gone below the bridge which is regarded here at first. > > > > Doesn't that end up removing portdrv/hotplug too so pciehp_remove() does > > release ctrl? I'm not sure if ctrl can be safely accessed even if the > > lock is taken first? > > Yes, it looks like it ends up in removing portdrv/hotplug too. I am not sure if this can be safely accessed. For testing > I added "set_service_data(dev, NULL);" at the end of pciehp_remove. This should make sure that it is not possible to > access freed ctrl. If there is a flow which accesses this, it should result in a null-ptr instead of UAF. I did some > runs with this change but I always ran into the UAF. Okay, perhaps it doesn't occur for some reason. I suppose the reason is that the concurrent pciehp_ist() waits for the lock in pciehp_unconfigure_device() and since it has not yet returned, free_irq() is what keeps the hotplug & ctrl getting removed. So it seems to me your change is fine. > For me it looks more related to the slot object. If I compare two runs (one with dyndbg enabled for pci and one without) > it will access the failing address in the __dynamic_dev_dbg portion at pci_destroy_slot in case of the dyndbg enabled > run. In case of the non dyndbg run it will fail while accessing > "kobject_put(&slot->kobj);" in pci_destroy_slot. The first error is <3>[ 10.244423] BUG: KASAN: slab-use-after-free in pci_slot_release+0x36e/0x3e0 so how you inferred it occurs in pci_destroy_slot()? > Unfortunately I have currently no clue about how can this slot object > ever been destroyed prematurely. There are dev_dbg()s on the paths that lead to destruction of the slot object. I don't see any of those lines in your logs so I don't believe that has occurred here. > I attach the logs of both runs. I know, one is based on an other kernel version but there it is more easy to reproduce > with KASAN enabled. What in these logs indicate to you it would be slot access which fails? To me it looks in both cases access to ->bus is the culprit (it also explains why dyndbg on/off matters because pci_destroy_slot() will not access ->bus otherwise so it can get all the way into pci_slot_release() before blowing up). -- i.
