Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33404
Change subject: nb/amd/amdfam10: die() on out of bounds reads ......................................................................
nb/amd/amdfam10: die() on out of bounds reads
These two functions try to access arrays of lengths 32 and 64 at indices of at most 259 and 71 (respectively). Something here is seriously wrong. This code was introduced in 2007, and aside from cosmetic changes, has had no modifications since then. I don't know what this code is supposed to do, and asking around on IRC, no one else did either. Until someone has the interest and time to work on it, let's at least add a die() to prevent the out of bounds access and alert the user that something is wrong.
Change-Id: I5fc15a50a9f0e97add31e3a40da82a15f7427358 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 12296{79-82} --- M src/northbridge/amd/amdfam10/ht_config.c 1 file changed, 8 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/33404/1
diff --git a/src/northbridge/amd/amdfam10/ht_config.c b/src/northbridge/amd/amdfam10/ht_config.c index 4810b99..1742fcd 100644 --- a/src/northbridge/amd/amdfam10/ht_config.c +++ b/src/northbridge/amd/amdfam10/ht_config.c @@ -127,6 +127,10 @@ u32 index;
for (index = 0; index < 256; index++) { + + if (index + 4 >= ARRAY_SIZE(sysconf.conf_io_addrx)) + die("Error! Out of bounds read in %s:%s\n", __FILE__, __func__); + if (sysconf.conf_io_addrx[index+4] == 0) { sysconf.conf_io_addr[index+4] = (nodeid & 0x3f); sysconf.conf_io_addrx[index+4] = 1 | ((linkn & 0x7)<<4); @@ -142,6 +146,10 @@ u32 index;
for (index = 0; index < 64; index++) { + + if (index + 8 >= ARRAY_SIZE(sysconf.conf_mmio_addrx)) + die("Error! Out of bounds read in %s:%s\n", __FILE__, __func__); + if (sysconf.conf_mmio_addrx[index+8] == 0) { sysconf.conf_mmio_addr[index+8] = (nodeid & 0x3f); sysconf.conf_mmio_addrx[index+8] = 1 | ((linkn & 0x7)<<4);
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33404 )
Change subject: nb/amd/amdfam10: die() on out of bounds reads ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/#/c/33404/1/src/northbridge/amd/amdfam10/ht_conf... File src/northbridge/amd/amdfam10/ht_config.c:
https://review.coreboot.org/#/c/33404/1/src/northbridge/amd/amdfam10/ht_conf... PS1, Line 132: die("Error! Out of bounds read in %s:%s\n", __FILE__, __func__); line over 80 characters
https://review.coreboot.org/#/c/33404/1/src/northbridge/amd/amdfam10/ht_conf... PS1, Line 151: die("Error! Out of bounds read in %s:%s\n", __FILE__, __func__); line over 80 characters
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33404
to look at the new patch set (#2).
Change subject: nb/amd/amdfam10: die() on out of bounds reads ......................................................................
nb/amd/amdfam10: die() on out of bounds reads
These two functions try to access arrays of lengths 32 and 64 at indices of at most 259 and 71 (respectively). Something here is seriously wrong. This code was introduced in 2007, and aside from cosmetic changes, has had no modifications since then. I don't know what this code is supposed to do, and asking around on IRC, no one else did either. Until someone has the interest and time to work on it, let's at least add a die() to prevent the out of bounds access and alert the user that something is wrong.
Change-Id: I5fc15a50a9f0e97add31e3a40da82a15f7427358 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 12296{79-82} --- M src/northbridge/amd/amdfam10/ht_config.c 1 file changed, 9 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/33404/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33404 )
Change subject: nb/amd/amdfam10: die() on out of bounds reads ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/#/c/33404/2/src/northbridge/amd/amdfam10/ht_conf... File src/northbridge/amd/amdfam10/ht_config.c:
https://review.coreboot.org/#/c/33404/2/src/northbridge/amd/amdfam10/ht_conf... PS2, Line 133: die("Error! Out of bounds read in %s:%s\n", __FILE__, __func__); line over 80 characters
https://review.coreboot.org/#/c/33404/2/src/northbridge/amd/amdfam10/ht_conf... PS2, Line 152: die("Error! Out of bounds read in %s:%s\n", __FILE__, __func__); line over 80 characters
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33404 )
Change subject: nb/amd/amdfam10: die() on out of bounds reads ......................................................................
Patch Set 2: Code-Review+2
Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33404 )
Change subject: nb/amd/amdfam10: die() on out of bounds reads ......................................................................
nb/amd/amdfam10: die() on out of bounds reads
These two functions try to access arrays of lengths 32 and 64 at indices of at most 259 and 71 (respectively). Something here is seriously wrong. This code was introduced in 2007, and aside from cosmetic changes, has had no modifications since then. I don't know what this code is supposed to do, and asking around on IRC, no one else did either. Until someone has the interest and time to work on it, let's at least add a die() to prevent the out of bounds access and alert the user that something is wrong.
Change-Id: I5fc15a50a9f0e97add31e3a40da82a15f7427358 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 12296{79-82} Reviewed-on: https://review.coreboot.org/c/coreboot/+/33404 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Arthur Heymans arthur@aheymans.xyz --- M src/northbridge/amd/amdfam10/ht_config.c 1 file changed, 9 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Arthur Heymans: Looks good to me, approved
diff --git a/src/northbridge/amd/amdfam10/ht_config.c b/src/northbridge/amd/amdfam10/ht_config.c index 4810b99..8499dbb 100644 --- a/src/northbridge/amd/amdfam10/ht_config.c +++ b/src/northbridge/amd/amdfam10/ht_config.c @@ -14,6 +14,7 @@ */
#include <stdint.h> +#include <console/console.h> #include <device/device.h> #include <device/pci_ops.h>
@@ -127,6 +128,10 @@ u32 index;
for (index = 0; index < 256; index++) { + + if (index + 4 >= ARRAY_SIZE(sysconf.conf_io_addrx)) + die("Error! Out of bounds read in %s:%s\n", __FILE__, __func__); + if (sysconf.conf_io_addrx[index+4] == 0) { sysconf.conf_io_addr[index+4] = (nodeid & 0x3f); sysconf.conf_io_addrx[index+4] = 1 | ((linkn & 0x7)<<4); @@ -142,6 +147,10 @@ u32 index;
for (index = 0; index < 64; index++) { + + if (index + 8 >= ARRAY_SIZE(sysconf.conf_mmio_addrx)) + die("Error! Out of bounds read in %s:%s\n", __FILE__, __func__); + if (sysconf.conf_mmio_addrx[index+8] == 0) { sysconf.conf_mmio_addr[index+8] = (nodeid & 0x3f); sysconf.conf_mmio_addrx[index+8] = 1 | ((linkn & 0x7)<<4);
-
Arthur Heymans (Code Review) -
build bot (Jenkins) (Code Review)
-
Jacob Garber (Code Review)
-
Martin Roth (Code Review)