Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33383
Change subject: nb/intel/nehalem: Prevent out of bounds read ......................................................................
nb/intel/nehalem: Prevent out of bounds read
Check that clock_speed_index is non-zero before decrementing it, else we will get a very large out of bounds array access.
Change-Id: Ic8ed1293adfe0866781bd638323977abd110777e Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1229675 --- M src/northbridge/intel/nehalem/raminit.c 1 file changed, 2 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/33383/1
diff --git a/src/northbridge/intel/nehalem/raminit.c b/src/northbridge/intel/nehalem/raminit.c index 15d6abb..72a2c57 100644 --- a/src/northbridge/intel/nehalem/raminit.c +++ b/src/northbridge/intel/nehalem/raminit.c @@ -598,7 +598,8 @@ for (clock_speed_index = 0; clock_speed_index < 3; clock_speed_index++) { if (cycletime == min_cycletime[clock_speed_index]) break; - if (cycletime > min_cycletime[clock_speed_index]) { + if (cycletime > min_cycletime[clock_speed_index] && + clock_speed_index != 0) { clock_speed_index--; cycletime = min_cycletime[clock_speed_index]; break;
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33383 )
Change subject: nb/intel/nehalem: Prevent out of bounds read ......................................................................
Patch Set 1:
(1 comment)
Thx for finding this. Not sure if my suggestion makes the coverity happy. If you want to make that code a bit more readable you could also have a look at other raminit code. See for instance snb_normalize_tclk() in northbridge/intel/sandybridge/raminit_sandy.c
https://review.coreboot.org/#/c/33383/1/src/northbridge/intel/nehalem/ramini... File src/northbridge/intel/nehalem/raminit.c:
https://review.coreboot.org/#/c/33383/1/src/northbridge/intel/nehalem/ramini... PS1, Line 601: cycletime > min_cycletime[clock_speed_index] You want to die() if cycletime > min_cycletime[0], with a message like: "RAM init: Decoded SPD DRAM freq is slower than the controller minimum!"
You can add this before the loop.
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33383 )
Change subject: nb/intel/nehalem: Prevent out of bounds read ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33383/1/src/northbridge/intel/nehalem/ramini... File src/northbridge/intel/nehalem/raminit.c:
https://review.coreboot.org/#/c/33383/1/src/northbridge/intel/nehalem/ramini... PS1, Line 601: cycletime > min_cycletime[clock_speed_index]
You want to die() if cycletime > min_cycletime[0], with a message like: […]
I'll do that, thanks!
Hello Arthur Heymans, David Hendricks, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33383
to look at the new patch set (#2).
Change subject: nb/intel/nehalem: Prevent out of bounds read ......................................................................
nb/intel/nehalem: Prevent out of bounds read
If the decoded SPD DRAM frequency is slower than the controller minimum, then there will be an unsigned integer underflow in the following loop, which will lead to a very large out of bounds array access. Ensure this does not happen.
Change-Id: Ic8ed1293adfe0866781bd638323977abd110777e Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1229675 --- M src/northbridge/intel/nehalem/raminit.c 1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/33383/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33383 )
Change subject: nb/intel/nehalem: Prevent out of bounds read ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/33383/2/src/northbridge/intel/nehalem/ramini... File src/northbridge/intel/nehalem/raminit.c:
https://review.coreboot.org/#/c/33383/2/src/northbridge/intel/nehalem/ramini... PS2, Line 599: die("RAM init: Decoded SPD DRAM freq %u is slower than the controller minimum!", line over 80 characters
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33383 )
Change subject: nb/intel/nehalem: Prevent out of bounds read ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/33383/2/src/northbridge/intel/nehalem/ramini... File src/northbridge/intel/nehalem/raminit.c:
https://review.coreboot.org/#/c/33383/2/src/northbridge/intel/nehalem/ramini... PS2, Line 599: die("RAM init: Decoded SPD DRAM freq %u is slower than the controller minimum!",
line over 80 characters
Done
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33383 )
Change subject: nb/intel/nehalem: Prevent out of bounds read ......................................................................
Patch Set 2: Code-Review+1
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33383 )
Change subject: nb/intel/nehalem: Prevent out of bounds read ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/33383/2/src/northbridge/intel/nehalem/ramini... File src/northbridge/intel/nehalem/raminit.c:
https://review.coreboot.org/#/c/33383/2/src/northbridge/intel/nehalem/ramini... PS2, Line 599: freq %u freq = 1 / cycletime.
Hello HAOUAS Elyes, Arthur Heymans, David Hendricks, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33383
to look at the new patch set (#3).
Change subject: nb/intel/nehalem: Prevent out of bounds read ......................................................................
nb/intel/nehalem: Prevent out of bounds read
If the decoded SPD DRAM frequency is slower than the controller minimum, then there will be an unsigned integer underflow in the following loop, which will lead to a very large out of bounds array access. Ensure this does not happen.
Change-Id: Ic8ed1293adfe0866781bd638323977abd110777e Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1229675 --- M src/northbridge/intel/nehalem/raminit.c 1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/33383/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33383 )
Change subject: nb/intel/nehalem: Prevent out of bounds read ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/33383/3/src/northbridge/intel/nehalem/ramini... File src/northbridge/intel/nehalem/raminit.c:
https://review.coreboot.org/#/c/33383/3/src/northbridge/intel/nehalem/ramini... PS3, Line 599: die("RAM init: Decoded SPD DRAM freq is slower than the controller minimum!"); line over 80 characters
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33383 )
Change subject: nb/intel/nehalem: Prevent out of bounds read ......................................................................
Patch Set 3: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33383 )
Change subject: nb/intel/nehalem: Prevent out of bounds read ......................................................................
Patch Set 3: Code-Review+1
(1 comment)
https://review.coreboot.org/#/c/33383/3/src/northbridge/intel/nehalem/ramini... File src/northbridge/intel/nehalem/raminit.c:
https://review.coreboot.org/#/c/33383/3/src/northbridge/intel/nehalem/ramini... PS3, Line 599: die("RAM init: Decoded SPD DRAM freq is slower than the controller minimum!");
line over 80 characters
Ugh... Maybe omit "DRAM" and change "is slower" to "lower". Shouldn't impact readability too much.
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33383 )
Change subject: nb/intel/nehalem: Prevent out of bounds read ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/33383/3/src/northbridge/intel/nehalem/ramini... File src/northbridge/intel/nehalem/raminit.c:
https://review.coreboot.org/#/c/33383/3/src/northbridge/intel/nehalem/ramini... PS3, Line 599: die("RAM init: Decoded SPD DRAM freq is slower than the controller minimum!");
Ugh... Maybe omit "DRAM" and change "is slower" to "lower". Shouldn't impact readability too much.
The line length was recently increased to 96 anyway, so I've kinda just been ignoring these warnings as long as it fits under that.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33383 )
Change subject: nb/intel/nehalem: Prevent out of bounds read ......................................................................
Patch Set 3: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/33383/3/src/northbridge/intel/nehal... File src/northbridge/intel/nehalem/raminit.c:
https://review.coreboot.org/c/coreboot/+/33383/3/src/northbridge/intel/nehal... PS3, Line 599: die("RAM init: Decoded SPD DRAM freq is slower than the controller minimum!");
The line length was recently increased to 96 anyway, so I've kinda just been ignoring these warnings […]
Ack
Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33383 )
Change subject: nb/intel/nehalem: Prevent out of bounds read ......................................................................
nb/intel/nehalem: Prevent out of bounds read
If the decoded SPD DRAM frequency is slower than the controller minimum, then there will be an unsigned integer underflow in the following loop, which will lead to a very large out of bounds array access. Ensure this does not happen.
Change-Id: Ic8ed1293adfe0866781bd638323977abd110777e Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1229675 Reviewed-on: https://review.coreboot.org/c/coreboot/+/33383 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: HAOUAS Elyes ehaouas@noos.fr Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/intel/nehalem/raminit.c 1 file changed, 2 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified HAOUAS Elyes: Looks good to me, but someone else must approve Angel Pons: Looks good to me, approved
diff --git a/src/northbridge/intel/nehalem/raminit.c b/src/northbridge/intel/nehalem/raminit.c index b4ff85c..fadf0e0 100644 --- a/src/northbridge/intel/nehalem/raminit.c +++ b/src/northbridge/intel/nehalem/raminit.c @@ -595,6 +595,8 @@ info-> spd[channel][slot][CAS_LATENCY_TIME]); } + if (cycletime > min_cycletime[0]) + die("RAM init: Decoded SPD DRAM freq is slower than the controller minimum!"); for (clock_speed_index = 0; clock_speed_index < 3; clock_speed_index++) { if (cycletime == min_cycletime[clock_speed_index]) break;
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
build bot (Jenkins) (Code Review)
-
HAOUAS Elyes (Code Review)
-
Jacob Garber (Code Review)
-
Martin Roth (Code Review)