Re: CVE-2023-52466: PCI: Avoid potential out-of-bounds read in pci_dev_for_each_resource()

[Greg Kroah-Hartman]

On Wed, Feb 28, 2024 at 10:20:13AM +0100, Jiri Kosina wrote:
> On Tue, 27 Feb 2024, Greg Kroah-Hartman wrote:
> 
> > > 09cc90063240 ("PCI: Introduce pci_dev_for_each_resource()") added
> > > pci_dev_for_each_resource(), which expands to:
> > > 
> > >   for (...; res = (&(dev)->resource[(bar)]), bar < PCI_NUM_RESOURCES; ...)
> > > 
> > > We compute "res" before the bounds-check of "bar", so the pointer may
> > > be out-of-bounds, but the body of the pci_dev_for_each_resource() loop
> > > is never executed with that out-of-bounds value.
> > > 
> > > So I don't think this is a security issue, no matter how
> > > pci_dev_for_each_resource() is used, unless the mere presence of an
> > > invalid address in a register is an issue.
> > 
> > Ah, yeah, now I remember, stuff like this was fixed up in other loops as
> > just reading off into the wild can be a speculation issue and so we had
> > to fix up a bunch of places in the kernel where we did have "invalid
> > data" in a register.  The code didn't use that, but the processor would
> > fetch from there, and boom, speculation mess.  There's a whole research
> > paper published on this type of thing somewhere...
> 
> Greg, could you please elaborate on this?
> 
> Where in this whole construct do you see a potential for *_uncached_* (!) 
> memory access that'd cause CPU to speculate into the wild? I just don't 
> see it.

Ok, finally got the time to look at this, and you are right, and I was
right (in a different way), but you are right more :)

What the change does is hopefully NOT allow the extra resource[bar]
read to happen (that is IFF the && change always comes first, but
processors do not guarantee it).  But if/when that additional
read-off-the-end-of-the-array happens, before the check, we are reading
ONLY a chunk of memory that we already allocated, it's the middle of the
pci device structure itself.

So the uncached read can still happen, but the read is of a location
that we still allow to be read (i.e. the next field in the structure).

So the "read off the end of the array" can still happen with this
change, that didn't really get fixed, but the read is "safe" as it's a
memory chunk we control.  All this change did is make the static checker
happier, and on cpus without a lot of speculation, not do the read where
it shouldn't have, but on modern cpus, nothing really changed at all.

I'll go mark this CVE rejected now, thanks all for the reviews!

greg k-h