Re: [PATCH 086/190] Revert "x86/PCI: Fix PCI IRQ routing table memory leak"

Bjorn Helgaas
2021-04-22T05:09:30Z

On Wed, Apr 21, 2021 at 02:59:21PM +0200, Greg Kroah-Hartman wrote:
> This reverts commit ea094d53580f40c2124cef3d072b73b2425e7bfd.
>
> Commits from @umn.edu addresses have been found to be submitted in "bad
> faith" to try to test the kernel community's ability to review "known
> malicious" changes. The result of these submissions can be found in a
> paper published at the 42nd IEEE Symposium on Security and Privacy
> entitled, "Open Source Insecurity: Stealthily Introducing
> Vulnerabilities via Hypocrite Commits" written by Qiushi Wu (University
> of Minnesota) and Kangjie Lu (University of Minnesota).
>
> Because of this, all submissions from this group must be reverted from
> the kernel tree and will need to be re-reviewed again to determine if
> they actually are a valid fix. Until that work is complete, remove this
> change to ensure that no problems are being introduced into the
> codebase.
>
> Cc: Wenwen Wang <wang6495@umn.edu>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

I would prefer that you not apply this revert.

Prior to ea094d53580f ("x86/PCI: Fix PCI IRQ routing table memory
leak"), we had essentially this:

pcibios_irq_init()
pirq_table = pcibios_get_irq_routing_table(); # kmallocs
if (pirq_table) {
if (io_apic_assign_pci_irqs)
pirq_table = NULL;
}

So if we called pcibios_get_irq_routing_table(), we kmalloced some
space and then (if io_apic_assign_pci_irqs) threw away the pointer,
which leaks the pointer as the commit log says.

After ea094d53580f, we have:

pcibios_irq_init()
rtable = NULL;
pirq_table = pcibios_get_irq_routing_table(); # kmallocs
rtable = pirq_table;
if (pirq_table) {
if (io_apic_assign_pci_irqs) {
kfree(rtable);
pirq_table = NULL;
}
}

which seems right to me.

Bjorn

> ---
> arch/x86/pci/irq.c | 10 ++--------
> 1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/pci/irq.c b/arch/x86/pci/irq.c
> index d3a73f9335e1..52e55108404e 100644
> --- a/arch/x86/pci/irq.c
> +++ b/arch/x86/pci/irq.c
> @@ -1119,8 +1119,6 @@ static const struct dmi_system_id pciirq_dmi_table[] __initconst = {
>
> void __init pcibios_irq_init(void)
> {
> - struct irq_routing_table *rtable = NULL;
> -
> DBG(KERN_DEBUG "PCI: IRQ init\n");
>
> if (raw_pci_ops == NULL)
> @@ -1131,10 +1129,8 @@ void __init pcibios_irq_init(void)
> pirq_table = pirq_find_routing_table();
>
> #ifdef CONFIG_PCI_BIOS
> - if (!pirq_table && (pci_probe & PCI_BIOS_IRQ_SCAN)) {
> + if (!pirq_table && (pci_probe & PCI_BIOS_IRQ_SCAN))
> pirq_table = pcibios_get_irq_routing_table();
> - rtable = pirq_table;
> - }
> #endif
> if (pirq_table) {
> pirq_peer_trick();
> @@ -1149,10 +1145,8 @@ void __init pcibios_irq_init(void)
> * If we're using the I/O APIC, avoid using the PCI IRQ
> * routing table
> */
> - if (io_apic_assign_pci_irqs) {
> - kfree(rtable);
> + if (io_apic_assign_pci_irqs)
> pirq_table = NULL;
> - }
> }
>
> x86_init.pci.fixup_irqs();
> --
> 2.31.1
>