Hi,
On 25/2/24 9:16, Greg Kroah-Hartman wrote:
> There is no actual issue right now because we have another check afterwards
> and the out-of-bounds read is not being performed. In any case it's better
> code with this fixed, hence the proposed change.
Given that there is no actual security issue this looks more like a
hardening, and thus not deserving of a CVE, no?
Best,
Carlos
--
Carlos López
Security Engineer
SUSE Software Solutions
On Tue, Feb 27, 2024 at 02:18:51PM +0100, Carlos L?pez wrote:
>
> Hi,
>
> On 25/2/24 9:16, Greg Kroah-Hartman wrote:
> > There is no actual issue right now because we have another check afterwards
> > and the out-of-bounds read is not being performed. In any case it's better
> > code with this fixed, hence the proposed change.
>
> Given that there is no actual security issue this looks more like a
> hardening, and thus not deserving of a CVE, no?
This was a tricky one, I think it's needed as we do not know how people
are really using these macros, right? If the PCI maintainer agrees (on
the cc:), I'll be glad to revoke it, it's their call.
And thanks for the review, much appreciated!
greg k-h
[+cc Mika, author of 09cc90063240]
On Tue, Feb 27, 2024 at 02:26:26PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Feb 27, 2024 at 02:18:51PM +0100, Carlos López wrote:
> > On 25/2/24 9:16, Greg Kroah-Hartman wrote:
> > > There is no actual issue right now because we have another check
> > > afterwards and the out-of-bounds read is not being performed. In
> > > any case it's better code with this fixed, hence the proposed
> > > change.
> >
> > Given that there is no actual security issue this looks more like a
> > hardening, and thus not deserving of a CVE, no?
>
> This was a tricky one, I think it's needed as we do not know how people
> are really using these macros, right? If the PCI maintainer agrees (on
> the cc:), I'll be glad to revoke it, it's their call.
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.
The same address computation is used for "pci_resource_start(dev,
bar)", which is used in hundreds of places where drivers supply the
BAR index, and the index is not checked.
We could consider adding a bounds check in pci_resource_n() to turn a
potential out-of-bounds reference into a NULL pointer dereference,
e.g.,
#define pci_resource_n(dev, bar) (bar < PCI_NUM_RESOURCES ?
&(dev)->resource[(bar)] : NULL)
But of course, there's nothing stopping drivers from computing
"&dev->resource[junk]" themselves.
Bjorn
On Tue, Feb 27, 2024 at 09:07:44AM -0600, Bjorn Helgaas wrote:
> [+cc Mika, author of 09cc90063240]
>
> On Tue, Feb 27, 2024 at 02:26:26PM +0100, Greg Kroah-Hartman wrote:
> > On Tue, Feb 27, 2024 at 02:18:51PM +0100, Carlos L?pez wrote:
> > > On 25/2/24 9:16, Greg Kroah-Hartman wrote:
> > > > There is no actual issue right now because we have another check
> > > > afterwards and the out-of-bounds read is not being performed. In
> > > > any case it's better code with this fixed, hence the proposed
> > > > change.
> > >
> > > Given that there is no actual security issue this looks more like a
> > > hardening, and thus not deserving of a CVE, no?
> >
> > This was a tricky one, I think it's needed as we do not know how people
> > are really using these macros, right? If the PCI maintainer agrees (on
> > the cc:), I'll be glad to revoke it, it's their call.
>
> 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...
So let's keep this as a CVE unless someone really doesn't want it marked
as such. Again, it is a "weakness that is fixed" in the kernel, and
because of that, a CVE can be allocated for it.
thanks,
greg k-h
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.
Thanks,
--
Jiri Kosina
SUSE Labs
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