[PATCH 0/1] Fix NULL pointer access in PowerPC MSI teardown code

[PATCH 0/1] Fix NULL pointer access in PowerPC MSI teardown code

[Radu Rendec]

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

[PATCH 1/1] Fix NULL pointer access in PowerPC MSI teardown code

[Radu Rendec]

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).

Signed-off-by: Radu Rendec <radu.rendec@gmail.com>
---
 arch/powerpc/kernel/msi.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/msi.c b/arch/powerpc/kernel/msi.c
index dab616a33b8d..83c2043cc685 100644
--- a/arch/powerpc/kernel/msi.c
+++ b/arch/powerpc/kernel/msi.c
@@ -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

Re: [1/1] Fix NULL pointer access in PowerPC MSI teardown code

[Michael Ellerman]

On Wed, 2018-11-28 at 03:20:48 UTC, Radu Rendec wrote:
> 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).
> 
> Signed-off-by: Radu Rendec <radu.rendec@gmail.com>

Applied to powerpc fixes, thanks.

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

cheers

Re: [PATCH 0/1] Fix NULL pointer access in PowerPC MSI teardown code

[Michael Ellerman]

Hi Radu,

Radu Rendec <radu.rendec@gmail.com> writes:
> 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.

What hardware are you on?

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

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

Re: [PATCH 0/1] Fix NULL pointer access in PowerPC MSI teardown code

[Radu Rendec]

Hi Michael,

On Wed, Nov 28, 2018 at 6:00 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> Radu Rendec <radu.rendec@gmail.com> writes:
> >
> > 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.
>
> What hardware are you on?

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).

> > I'm proposing the attached patch to fix the problem. It basically just
> > checks the pointer before the function call.
>
> 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?

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

Cheers,
Radu