Leon Romanovsky <leon@xxxxxxxxxx> writes: > On Tue, Jan 24, 2023 at 01:52:37PM +0200, Alexander Shishkin wrote: >> Leon Romanovsky <leon@xxxxxxxxxx> writes: >> >> > 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. A malicious device is what the fuzzing is aiming to simulate. The fact of fuzzing process itself didn't seem relevant to the patch, so I didn't include it, going instead for the problem statement and proposed solution. Will the commit message benefit from mentioning fuzzing? > Do you see "gazillion bugs" for devices which don't change their MSI-X > table size under the hood, which is main kernel assumption? Not so far. > If yes, you should fix these bugs. That's absolutely the intention. >> > 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(). Well, since you stated that the reverse will cause a kernel crash, I had to ask how. I'll include some version of the above paragraph in the commit message to indicate that we reverse situation has been considered. Regards, -- Alex
