Fix NULL pointer access in PowerPC MSI teardown code

Hi everyone,

It seems there's an unchecked access to a NULL pointer (to a function) in the PowerPC MSI teardown code. I found this on kernel 4.9, but the code looks identical in the latest 4.20-rc. I don't see any reason why this wouldn't happen on recent kernels too.

The PowerPC architecture specific MSI setup and teardown functions are in arch/powerpc/kernel/msi.c:

* arch_setup_msi_irqs() checks pointers for both the setup_msi_irqs and teardown_msi_irqs ops and returns -ENOSYS if either one is NULL.

* arch_teardown_msi_irqs() calls on the teardown_msi_irqs op pointer without checking it and assumes the function is never called unless arch_setup_msi_irqs() returns successfully.

The assumption in arch_teardown_msi_irqs() is wrong and results in a function call on a NULL pointer. An example of how this can happen is included in the actual patch header. In my case, it happens when the PCI hardware is configured during kernel start-up, because my controller doesn't support MSI and the ops are NULL.

I'm proposing the attached patch to fix the problem. It basically just checks the pointer before the function call.

The patch is against v4.20-rc4, but I only actually tested it on v4.9.115. On the other hand, the patch is trivial and I did check that the NULL pointer dereference scenario is still valid on v4.20-rc4.

Best regards,
Radu Rendec

Radu Rendec (1):
Fix NULL pointer access in PowerPC MSI teardown code

 arch/powerpc/kernel/msi.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

2.17.2

The arch_teardown_msi_irqs() function assumes that controller ops pointers were already checked in arch_setup_msi_irqs(), but this assumption is wrong: arch_teardown_msi_irqs() can be called even when arch_setup_msi_irqs() returns an error (-ENOSYS).

This can happen in the following scenario:

* msi_capability_init() calls pci_msi_setup_msi_irqs()
* pci_msi_setup_msi_irqs() returns -ENOSYS
* msi_capability_init() notices the error and calls free_msi_irqs()
* free_msi_irqs() calls pci_msi_teardown_msi_irqs()

This is easier to see when CONFIG_PCI_MSI_IRQ_DOMAIN is not set and pci_msi_setup_msi_irqs() and pci_msi_teardown_msi_irqs() are just aliases to arch_setup_msi_irqs() and arch_teardown_msi_irqs().

The call to free_msi_irqs() upon pci_msi_setup_msi_irqs() failure seems legit, as it does additional cleanup; e.g. list_del(&entry->list) and kfree(entry) inside free_msi_irqs() do happen (MSI descriptors are allocated before pci_msi_setup_msi_irqs() is called and need to be cleaned up if that fails).

---
 arch/powerpc/kernel/msi.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

index dab616a33b8d..83c2043cc685 100644
@@ -34,5 +34,12 @@ void arch_teardown_msi_irqs(struct pci_dev *dev)
 {
 	struct pci_controller *phb = pci_bus_to_host(dev->bus);
 
-	phb->controller_ops.teardown_msi_irqs(dev);
+	/*
+	 * We can be called even when arch_setup_msi_irqs() returns -ENOSYS,
+	 * so check the pointer again. Example: msi_capability_init() calls
+	 * pci_msi_setup_msi_irqs(), then free_msi_irqs(), which in turn calls
+	 * pci_msi_teardown_msi_irqs().
+	 */
+	if (phb->controller_ops.teardown_msi_irqs)
+		phb->controller_ops.teardown_msi_irqs(dev);
 }

2.17.2

Applied to powerpc fixes, thanks.

https://git.kernel.org/powerpc/c/78e7b15e17ac175e7eed9e21c6f92d

cheers

Hi Radu,

What hardware are you on?

Yeah that patch looks good to me.

I suspect this bug was introduced in:

6b2fd7efeb88 ("PCI/MSI/PPC: Remove arch_msi_check_device()")

Previously we had that check routine which would run before any of the MSI setup had been done, and so if there were no MSI ops then we bailed out early and didn't call teardown.

I guess since then (2014) we haven't tested an MSI capable device on a system that isn't MSI capable?

cheers

Hi Michael,

I'm on Freescale MPC8378 - old stuff, but still going strong :) The MSI capable device is a Broadcom PEX 8613 (a 3-port PCIe switch).

Thanks for looking into this. You're probably right, it really looks like that patch could have introduced this bug.

Cheers,
Radu