Hi, On Thu, 2024-09-26 at 16:58 +0300, Ilpo Järvinen wrote: > 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()? Ok, yes. You are right. I was blinded by dev_dbg 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). >
