Re: UAF during boot on MTL based devices with attached dock

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4383 bytes --]

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.