linux-kernel - Re: revert bab65e48cb064 PCI/MSI Sanitize MSI-X checks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <878rf3tm48.ffs@tglx>
Date:   Fri, 07 Apr 2023 23:31:51 +0200
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Linus Torvalds <torvalds@...ux-foundation.org>,
        David Laight <David.Laight@...lab.com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Jason Gunthorpe <jgg@...dia.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Christoph Hellwig <hch@...radead.org>
Subject: Re: revert bab65e48cb064 PCI/MSI Sanitize MSI-X checks

Linus!

On Fri, Apr 07 2023 at 12:26, Linus Torvalds wrote:
> On Fri, Apr 7, 2023 at 5:25 AM David Laight <David.Laight@...lab.com> wrote:
>>
>> I think it depends on why the driver is asking for more
>> interrupts than the hardware supports.
>> Potentially the driver could do:
>>         for (i = 0; i < 8; i++)
>>                 msix_tbl[i].entry = 2 * i;
>> if the hardware supports 8 interrupts perhaps it
>> should return 4?
>
> Hmm.
>
> Something like this might get that case right too. Again - entirely
> untested, but looks superficially sane to me.
>
> It just returns how many of the msix_entry[] entries can be used (or
> negative for error). So then the (only) caller can just say "is that
> still enough for what we required". Seems reasonable, and would get
> that non-contiguous case right, I think.

Probably, but the point is that a driver should not hand in invalid data
in the first place. Even if the hardware does not provide enough space
for the requested maximum range then the handed in entries array should
not contain any invalid data, right?

Ergo the hwsize check in that function is bogus. No idea what I was
thinking there.

So the simple and IMO correct solution is to drop that hwsize check from
the validation function and validate up to the

The point is that for a range allocation with and entries array, _all_
entries up to max_vec must be correct independent of the actual hardware
size.

So the fix is simply removing the hardware size check from the
validation function. The hardware size checking happens afterwards
anyway.

Let me write up a proper changelog for that.

Thanks,

	tglx
---
 drivers/pci/msi/msi.c |    9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

--- a/drivers/pci/msi/msi.c
+++ b/drivers/pci/msi/msi.c
@@ -750,8 +750,7 @@ static int msix_capability_init(struct p
	return ret;
 }

-static bool pci_msix_validate_entries(struct pci_dev *dev, struct msix_entry *entries,
-				      int nvec, int hwsize)
+static bool pci_msix_validate_entries(struct pci_dev *dev, struct msix_entry *entries, int nvev)
 {
	bool nogap;
	int i, j;
@@ -762,10 +761,6 @@ static bool pci_msix_validate_entries(st
	nogap = pci_msi_domain_supports(dev, MSI_FLAG_MSIX_CONTIGUOUS, DENY_LEGACY);

	for (i = 0; i < nvec; i++) {
-		/* Entry within hardware limit? */
-		if (entries[i].entry >= hwsize)
-			return false;
-
		/* Check for duplicate entries */
		for (j = i + 1; j < nvec; j++) {
			if (entries[i].entry == entries[j].entry)
@@ -805,7 +800,7 @@ int __pci_enable_msix_range(struct pci_d
	if (hwsize < 0)
		return hwsize;

-	if (!pci_msix_validate_entries(dev, entries, nvec, hwsize))
+	if (!pci_msix_validate_entries(dev, entries, nvec))
		return -EINVAL;

	if (hwsize < nvec) {