awokd@danwin1210.me has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36192 )
Change subject: vc/amd/agesa: Fix out of bounds read ......................................................................
Patch Set 4:
(5 comments)
https://review.coreboot.org/c/coreboot/+/36192/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36192/2//COMMIT_MSG@9 PS2, Line 9: ByteLane is incorrectly used unitialized from prior for statement.
Might be bad coding style, not incorrect per-se. […]
Done
https://review.coreboot.org/c/coreboot/+/36192/2//COMMIT_MSG@11 PS2, Line 11: PassTestRxEnDly at that index, so appears safe to delete.
Rephrase perhaps: PassTestRxEnDly[MaxByteLanes] never appears as rvalue, all for loops have ByteLane […]
Done
https://review.coreboot.org/c/coreboot/+/36192/2/src/vendorcode/amd/agesa/f1... File src/vendorcode/amd/agesa/f12/Proc/Mem/Tech/mtthrcSeedTrain.c:
https://review.coreboot.org/c/coreboot/+/36192/2/src/vendorcode/amd/agesa/f1... PS2, Line 329: if ((RxEn >= NBPtr->MinRxEnSeedGross) && (RxEn <= NBPtr->MaxRxEnSeedTotal)) {
For coverity scan, this might evaluate as a static true statement.
Coverity seemed to attempt to evaluate all branches, but I'm definitely unfamiliar with it. Why would it think this to be always true?
https://review.coreboot.org/c/coreboot/+/36192/2/src/vendorcode/amd/agesa/f1... PS2, Line 337: OutOfRange[ByteLane] = TRUE;
... so this path of not setting PassTestRxEndDly might not ever be reached.
This path is why I was wondering if the intent with 'PassTestRxEnDly[ByteLane] = RxOrig[ByteLane];' was to initialize the PassTestRxEnDly array with RxOrig's contents (which are "Original RxEn Dly based on PRE results"). If so, that line would work better here. I found "2.9.5.9.3 DQS Receiver Enable Cycle Training" in the BKDG for AMD 15h. It was not immediately illuminating, but I will keep studying.
https://review.coreboot.org/c/coreboot/+/36192/2/src/vendorcode/amd/agesa/f1... PS2, Line 359: MemClkCycle = (UINT8) (PassTestRxEnDly[ByteLane] >> 5);
PassTestRxEnDly could still be unset here?
Since you point it out, I'm surprised Coverity didn't flag this or even the use in original line #343 as potentially uninitialized.