linux-kernel - Re: [PATCH 1/2] PCI/MSI: Cache the MSIX table size

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: <Y8/Kyzh+stow83lQ@unreal>
Date:   Tue, 24 Jan 2023 14:10:51 +0200
From:   Leon Romanovsky <leon@...nel.org>
To:     Alexander Shishkin <alexander.shishkin@...ux.intel.com>
Cc:     Bjorn Helgaas <bhelgaas@...gle.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
        Marc Zyngier <maz@...nel.org>, darwi@...utronix.de,
        elena.reshetova@...el.com, kirill.shutemov@...ux.intel.com,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        stable@...r.kernel.org
Subject: Re: [PATCH 1/2] PCI/MSI: Cache the MSIX table size

On Tue, Jan 24, 2023 at 01:52:37PM +0200, Alexander Shishkin wrote:
> Leon Romanovsky <leon@...nel.org> writes:
> 
> > On Thu, Jan 19, 2023 at 07:06:32PM +0200, Alexander Shishkin wrote:
> >> A malicious device can change its MSIX table size between the table
> >> ioremap() and subsequent accesses, resulting in a kernel page fault in
> >> pci_write_msg_msix().
> >> 
> >> To avoid this, cache the table size observed at the moment of table
> >> ioremap() and use the cached value. This, however, does not help drivers
> >> that peek at the PCIE_MSIX_FLAGS register directly.
> >> 
> >> Signed-off-by: Alexander Shishkin <alexander.shishkin@...ux.intel.com>
> >> Reviewed-by: Mika Westerberg <mika.westerberg@...ux.intel.com>
> >> Cc: stable@...r.kernel.org
> >> ---
> >>  drivers/pci/msi/api.c | 7 ++++++-
> >>  drivers/pci/msi/msi.c | 2 +-
> >>  include/linux/pci.h   | 1 +
> >>  3 files changed, 8 insertions(+), 2 deletions(-)
> >
> > I'm not security expert here, but not sure that this protects from anything.
> > 1. Kernel relies on working and not-malicious HW. There are gazillion ways
> > to cause crashes other than changing MSI-X.
> 
> This particular bug was preventing our fuzzing from going deeper into
> the code and reaching some more of the aforementioned gazillion bugs.

Your commit message says nothing about fuzzing, but talks about
malicious device. 
Do you see "gazillion bugs" for devices which don't change their MSI-X
table size under the hood, which is main kernel assumption?

If yes, you should fix these bugs.

> 
> > 2. Device can report large table size, kernel will cache it and
> > malicious device will reduce it back. It is not handled and will cause
> > to kernel crash too.
> 
> How would that happen? If the device decides to have fewer vectors,
> they'll all still fit in the ioremapped MSIX table. The worst thing that
> can happen is 0xffffffff reads from the mmio space, which a device can
> do anyway. But that shouldn't trigger a page fault or otherwise
> crash. Or am I missing something?

Like I said, I'm no expert. You should tell me if it safe for all
callers of pci_msix_vec_count().

Thanks

> 
> Thanks,
> --
> Alex