awokd@danwin1210.me has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38466 )
Change subject: vc/amd/agesa/f16kb/Proc/CPU: Avoid out-of-bounds read ......................................................................
vc/amd/agesa/f16kb/Proc/CPU: Avoid out-of-bounds read
In an edge case where the cpuF16AddingMmioMap function is called with a fully populated MmioRange array, MmioPair could be incremented to its maximum value of 11, causing an out-of-bounds read in cpuF16MmioMap.c:228 with the comparison to MmioRange[MmioPair + i]. Add logic to avoid this case, and similar as i is incremented.
Change-Id: I781bee51a57602f3d73f43ad0ad8466a01ef8736 Signed-off-by: Joe Moore awokd@danwin1210.me Found-by: Coverity CID 1376955 --- M src/vendorcode/amd/agesa/f16kb/Proc/CPU/Family/0x16/cpuF16MmioMap.c 1 file changed, 20 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/38466/1
diff --git a/src/vendorcode/amd/agesa/f16kb/Proc/CPU/Family/0x16/cpuF16MmioMap.c b/src/vendorcode/amd/agesa/f16kb/Proc/CPU/Family/0x16/cpuF16MmioMap.c index 714f970..1023215 100644 --- a/src/vendorcode/amd/agesa/f16kb/Proc/CPU/Family/0x16/cpuF16MmioMap.c +++ b/src/vendorcode/amd/agesa/f16kb/Proc/CPU/Family/0x16/cpuF16MmioMap.c @@ -223,29 +223,36 @@ // | | // new base new limit
+ if (MmioPair >= (MMIO_REG_PAIR_NUM - 1)) { + IDS_HDT_CONSOLE (MAIN_FLOW, " [ERROR] Not enough MMIO register pairs to hold the request.\n"); + return AGESA_ERROR; + } MmioRange[MmioPair].Base = (MmioRange[MmioPair].Base <= NewMmioRange.Base) ? MmioRange[MmioPair].Base : NewMmioRange.Base; MmioRange[MmioPair].Modified = TRUE; - for (i = 1; NewMmioRange.Limit >= MmioRange[MmioPair + i].Base; i++) { - if ((NewMmioRange.Attribute.MmioPostedRange == MmioRange[MmioPair + i].Attribute.MmioPostedRange) && - (NewMmioRange.Attribute.MmioReadableRange == MmioRange[MmioPair + i].Attribute.MmioReadableRange) && - (NewMmioRange.Attribute.MmioWritableRange == MmioRange[MmioPair + i].Attribute.MmioWritableRange) && - (NewMmioRange.Attribute.MmioSecuredRange == MmioRange[MmioPair + i].Attribute.MmioSecuredRange)) { - MmioRange[MmioPair].Limit = MmioRange[MmioPair + i].Limit; - MmioRange[MmioPair + i].Base = 0; - MmioRange[MmioPair + i].Limit = F16_MMIO_ALIGN; - MmioRange[MmioPair + i].Attribute.MmioReadableRange = 0; - MmioRange[MmioPair + i].Attribute.MmioWritableRange = 0; - MmioRange[MmioPair + i].Modified = TRUE; + for (i = MmioPair + 1; i < (MMIO_REG_PAIR_NUM - 1); i++) { + if ((NewMmioRange.Attribute.MmioPostedRange == MmioRange[i].Attribute.MmioPostedRange) && + (NewMmioRange.Attribute.MmioReadableRange == MmioRange[i].Attribute.MmioReadableRange) && + (NewMmioRange.Attribute.MmioWritableRange == MmioRange[i].Attribute.MmioWritableRange) && + (NewMmioRange.Attribute.MmioSecuredRange == MmioRange[i].Attribute.MmioSecuredRange) && + (NewMmioRange.Limit >= MmioRange[i + 1].Base)) { + MmioRange[MmioPair].Limit = MmioRange[i].Limit; + MmioRange[i].Base = 0; + MmioRange[i].Limit = F16_MMIO_ALIGN; + MmioRange[i].Attribute.MmioReadableRange = 0; + MmioRange[i].Attribute.MmioWritableRange = 0; + MmioRange[i].Modified = TRUE; UnusedMmioPair++; - } else if (MmioRange[MmioPair + i].Attribute.MmioReadableRange != 0 || MmioRange[MmioPair + i].Attribute.MmioWritableRange != 0) { + } else if (MmioRange[i].Attribute.MmioReadableRange != 0 || MmioRange[i].Attribute.MmioWritableRange != 0) { // Overlapped MMIO regions with different attributes MmioRange[MmioPair].Limit = (MmioRange[MmioPair].Limit >= NewMmioRange.Limit) ? MmioRange[MmioPair].Limit : NewMmioRange.Limit; NewMmioIncluded = TRUE; Overlap = TRUE; break; + } else if (NewMmioRange.Limit < MmioRange[i + 1].Base) { + break; } } - MmioRange[MmioPair].Limit = (MmioRange[MmioPair + i - 1].Limit >= NewMmioRange.Limit) ? MmioRange[MmioPair + i - 1].Limit : NewMmioRange.Limit; + MmioRange[MmioPair].Limit = (MmioRange[i].Limit >= NewMmioRange.Limit) ? MmioRange[i].Limit : NewMmioRange.Limit; break; } else { // Overlapped MMIO regions with different attributes
awokd@danwin1210.me has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38466 )
Change subject: vc/amd/agesa/f16kb/Proc/CPU: Avoid out-of-bounds read ......................................................................
Patch Set 1:
The logic has been tested on a G505s which is a f15tn, but the hardware and code is similar. If this fix is acceptable, intent is to backport the bulk of this function from f16kb to f15tn (including the fix) because it has improved handling of overlapping MMIO ranges.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38466 )
Change subject: vc/amd/agesa/f16kb/Proc/CPU: Avoid out-of-bounds read ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38466/1/src/vendorcode/amd/agesa/f1... File src/vendorcode/amd/agesa/f16kb/Proc/CPU/Family/0x16/cpuF16MmioMap.c:
https://review.coreboot.org/c/coreboot/+/38466/1/src/vendorcode/amd/agesa/f1... PS1, Line 226: if (MmioPair >= (MMIO_REG_PAIR_NUM - 1)) { Why not: MmmioPair > MMIO_REG_PAIR_NUM
https://review.coreboot.org/c/coreboot/+/38466/1/src/vendorcode/amd/agesa/f1... PS1, Line 227: IDS_HDT_CONSOLE (MAIN_FLOW, " [ERROR] Not enough MMIO register pairs to hold the request.\n"); Please add the values to the error message.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38466
to look at the new patch set (#2).
Change subject: vc/amd/agesa/f16kb/Proc/CPU: Avoid out-of-bounds read ......................................................................
vc/amd/agesa/f16kb/Proc/CPU: Avoid out-of-bounds read
In an edge case where the cpuF16AddingMmioMap function is called with a fully populated MmioRange array, MmioPair could be incremented to its maximum value of 11, causing an out-of-bounds read in cpuF16MmioMap.c:228 with the comparison to MmioRange[MmioPair + i]. Add logic to avoid this case, and similar as i is incremented.
Change-Id: I781bee51a57602f3d73f43ad0ad8466a01ef8736 Signed-off-by: Joe Moore awokd@danwin1210.me Found-by: Coverity CID 1376955 --- M src/vendorcode/amd/agesa/f16kb/Proc/CPU/Family/0x16/cpuF16MmioMap.c 1 file changed, 22 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/38466/2
awokd@danwin1210.me has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38466 )
Change subject: vc/amd/agesa/f16kb/Proc/CPU: Avoid out-of-bounds read ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38466/1/src/vendorcode/amd/agesa/f1... File src/vendorcode/amd/agesa/f16kb/Proc/CPU/Family/0x16/cpuF16MmioMap.c:
https://review.coreboot.org/c/coreboot/+/38466/1/src/vendorcode/amd/agesa/f1... PS1, Line 226: if (MmioPair >= (MMIO_REG_PAIR_NUM - 1)) {
Why not: MmmioPair > MMIO_REG_PAIR_NUM
I moved the `i` init further up to hopefully make more clear. Checking here to avoid attempt to read MmioRange[i + 1] inside the FOR loop, if program flow makes it here with MmioPair == 11. The `>` is probably superfluous (there should be no way to get here with an MmioPair > 11), but left it in since we're making a comparison anyway.
https://review.coreboot.org/c/coreboot/+/38466/1/src/vendorcode/amd/agesa/f1... PS1, Line 227: IDS_HDT_CONSOLE (MAIN_FLOW, " [ERROR] Not enough MMIO register pairs to hold the request.\n");
Please add the values to the error message.
Done
Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/38466?usp=email )
Change subject: vc/amd/agesa/f16kb/Proc/CPU: Avoid out-of-bounds read ......................................................................
Abandoned
-
Name of user not set (Code Review) -
Paul Menzel (Code Review)
-
Stefan Reinauer (Code Review)