Received-SPF: pass (google.com: domain of linux-kernel+bounces-163057-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
From: Thomas Gleixner <tglx@linutronix.de>
To: Mostafa Saleh <smostafa@google.com>, bhelgaas@google.com,
 linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org
Cc: ilpo.jarvinen@linux.intel.com, kernel-team@android.com, Mostafa Saleh
 <smostafa@google.com>
Subject: Re: [PATCH] PCI/MSI: Fix UAF in msi_capability_init
In-Reply-To: <20240429034112.140594-1-smostafa@google.com>
References: <20240429034112.140594-1-smostafa@google.com>
Date: Mon, 29 Apr 2024 23:04:39 +0200
Message-ID: <87zftbswwo.ffs@tglx>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

On Mon, Apr 29 2024 at 03:41, Mostafa Saleh wrote:
>  err:
> -	pci_msi_unmask(entry, msi_multi_mask(entry));
> +	/* Re-read the descriptor as it might have been freed */
> +	entry = msi_first_desc(&dev->dev, MSI_DESC_ALL);
> +	if (entry)
> +		pci_msi_unmask(entry, msi_multi_mask(entry));

What unmasks the entry in the NULL case?

The mask has to be undone. So you need something like the uncompiled
below.

Thanks,

        tglx
---

--- a/drivers/pci/msi/msi.c
+++ b/drivers/pci/msi/msi.c
@@ -349,7 +349,7 @@ static int msi_capability_init(struct pc
 			       struct irq_affinity *affd)
 {
 	struct irq_affinity_desc *masks = NULL;
-	struct msi_desc *entry;
+	struct msi_desc *entry, desc;
 	int ret;
 
 	/* Reject multi-MSI early on irq domain enabled architectures */
@@ -374,6 +374,12 @@ static int msi_capability_init(struct pc
 	/* All MSIs are unmasked by default; mask them all */
 	entry = msi_first_desc(&dev->dev, MSI_DESC_ALL);
 	pci_msi_mask(entry, msi_multi_mask(entry));
+	/*
+	 * Copy the MSI descriptor for the error path because
+	 * pci_msi_setup_msi_irqs() will free it for the hierarchical
+	 * interrupt domain case.
+	 */
+	memcpy(&desc, entry, sizeof(desc));
 
 	/* Configure MSI capability structure */
 	ret = pci_msi_setup_msi_irqs(dev, nvec, PCI_CAP_ID_MSI);
@@ -393,7 +399,7 @@ static int msi_capability_init(struct pc
 	goto unlock;
 
 err:
-	pci_msi_unmask(entry, msi_multi_mask(entry));
+	pci_msi_unmask(&desc, msi_multi_mask(&desc));
 	pci_free_msi_irqs(dev);
 fail:
 	dev->msi_enabled = 0;