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

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

[Greg Kroah-Hartman]

Description
===========

In the Linux kernel, the following vulnerability has been resolved:

PCI: Avoid potential out-of-bounds read in pci_dev_for_each_resource()

Coverity complains that pointer in the pci_dev_for_each_resource() may be
wrong, i.e., might be used for the out-of-bounds read.

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.

As Jonas pointed out "It probably makes the code slightly less performant
as res will now be checked for being not NULL (which will always be true),
but I doubt it will be significant (or in any hot paths)."

The Linux kernel CVE team has assigned CVE-2023-52466 to this issue.


Affected and fixed versions
===========================

	Issue introduced in 6.4 with commit 09cc90063240 and fixed in 6.6.14 with commit 5b3e25efe16e
	Issue introduced in 6.4 with commit 09cc90063240 and fixed in 6.7.2 with commit bd26159dcaaa
	Issue introduced in 6.4 with commit 09cc90063240 and fixed in 6.8-rc1 with commit 3171e46d677a

Please see https://www.kernel.org or a full list of currently supported
kernel versions by the kernel community.

Unaffected versions might change over time as fixes are backported to
older supported kernel versions.  The official CVE entry at
	https://cve.org/CVERecord/?id=CVE-2023-52466
will be updated if fixes are backported, please check that for the most
up to date information about this issue.


Affected files
==============

The file(s) affected by this issue are:
	include/linux/pci.h


Mitigation
==========

The Linux kernel CVE team recommends that you update to the latest
stable kernel version for this, and many other bugfixes.  Individual
changes are never tested alone, but rather are part of a larger kernel
release.  Cherry-picking individual commits is not recommended or
supported by the Linux kernel community at all.  If however, updating to
the latest release is impossible, the individual changes to resolve this
issue can be found at these commits:
	https://git.kernel.org/stable/c/5b3e25efe16e06779a9a7c7610217c1b921ec179
	https://git.kernel.org/stable/c/bd26159dcaaa3e9a927070efd348e7ce7e5ee933
	https://git.kernel.org/stable/c/3171e46d677a668eed3086da78671f1e4f5b8405

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

[Carlos López]

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

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

[Greg Kroah-Hartman]

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

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

[Bjorn Helgaas]

[+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

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

[Greg Kroah-Hartman]

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

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

[Bjorn Helgaas]

On Tue, Feb 27, 2024 at 06:24:38PM +0100, Greg Kroah-Hartman wrote:
> 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.

Sounds good, I'm happy to have it as a CVE.  Thanks for the
speculation details; I'm certainly not an expert on that.

Bjorn

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

[Jiri Kosina]

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

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