Change in coreboot[master]: cpu/x86: Initialize the argument value msr_index

[John Zhao (Code Review)]

Hello John Zhao,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/coreboot/+/34249

to review the following change.

Change subject: cpu/x86: Initialize the argument value msr_index
......................................................................

cpu/x86: Initialize the argument value msr_index

Clang Static Analyzer version 8.0.0 treats the uninitialized argument
value msr_index as logic error in a function call. Just set msr_index
with 0 initially.

Change-Id: I051b290998207a1129ada06bb455f44c2d291975
Signed-off-by: John Zhao 
---
M src/cpu/x86/mtrr/mtrr.c
1 file changed, 1 insertion(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/34249/1

diff --git a/src/cpu/x86/mtrr/mtrr.c b/src/cpu/x86/mtrr/mtrr.c
index 98449d5..4dc71d6 100644
--- a/src/cpu/x86/mtrr/mtrr.c
+++ b/src/cpu/x86/mtrr/mtrr.c
@@ -321,6 +321,7 @@

fixed_mtrrs_expose_amd_rwdram();

+	memset(&msr_index, 0, sizeof(msr_index));
    memset(&fixed_msrs, 0, sizeof(fixed_msrs));

msr_num = 0;

[Lance Zhao (Code Review)]

Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34249 )

Change subject: cpu/x86: Initialize the argument value msr_index
......................................................................

Patch Set 1: Code-Review+2

[Martin Roth (Code Review)]

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34249 )

Change subject: cpu/x86: Initialize the argument value msr_index
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34249/1/src/cpu/x86/mtrr/mtrr.c 
File src/cpu/x86/mtrr/mtrr.c:

https://review.coreboot.org/c/coreboot/+/34249/1/src/cpu/x86/mtrr/mtrr.c@361 
PS1, Line 361: ARRAY_SIZE(fixed_msrs)
I think another way to fix this would be to use msr_num here and in the loop below.  What the static analyzer is seeing, i believe, is that we might not go through the loop above the same number of times as the loop here and below, leading to the us to use the uninitialized variable.  Changing the value to 0 will cause us to write out to MSR 0 if that ever happened.

If you want to stick with the meset, maybe add this to each loop to make sure we don't do that here?

if (msr_index[i] == 0)
    next;

What do you think?

[John Zhao (Code Review)]

John Zhao has uploaded a new patch set (#2) to the change originally created by John Zhao. ( https://review.coreboot.org/c/coreboot/+/34249 )

Change subject: cpu/x86: Initialize the argument value msr_index
......................................................................

cpu/x86: Initialize the argument value msr_index

Clang Static Analyzer version 8.0.0 treats the uninitialized argument
value msr_index as logic error in a function call. Just set msr_index
with 0 initially.

Change-Id: I051b290998207a1129ada06bb455f44c2d291975
Signed-off-by: John Zhao 
---
M src/cpu/x86/mtrr/mtrr.c
1 file changed, 4 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/34249/2

[John Zhao (Code Review)]

John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34249 )

Change subject: cpu/x86: Initialize the argument value msr_index
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34249/1/src/cpu/x86/mtrr/mtrr.c 
File src/cpu/x86/mtrr/mtrr.c:

https://review.coreboot.org/c/coreboot/+/34249/1/src/cpu/x86/mtrr/mtrr.c@361 
PS1, Line 361: ARRAY_SIZE(fixed_msrs)
> I think another way to fix this would be to use msr_num here and in the loop below. […]

"ASSERT(msr_num == NUM_FIXED_MTRRS)" would ensure msr_num equal to NUM_FIXED_MTRRS. By fixed_msrs's definition, "msr_t fixed_msrs[NUM_FIXED_MTRRS]", ARRAY_SIZE(fixed_msrs) equals to NUM_FIXED_MTRRS. so msr_num would be the same value as ARRAY_SIZE(fixed_msrs).

Agreed with you that it would need to add check to prevent writing MRS 0 while we are with memset initially for msr_index. Just updated by adding "ASSERT (msr_index[i] != 0)" in the loop

[John Zhao (Code Review)]

John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34249 )

Change subject: cpu/x86: Initialize the argument value msr_index
......................................................................

Patch Set 2: Code-Review+2

[John Zhao (Code Review)]

John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34249 )

Change subject: cpu/x86: Initialize the argument value msr_index
......................................................................

Patch Set 3: -Code-Review

[Lance Zhao (Code Review)]

Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34249 )

Change subject: cpu/x86: Initialize the argument value msr_index
......................................................................

Patch Set 3: Code-Review+2

[Lance Zhao (Code Review)]

Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34249 )

Change subject: cpu/x86: Initialize the argument value msr_index
......................................................................

Patch Set 3: -Code-Review

[XS] Change in coreboot[master]: cpu/x86: Initialize the argument value msr_index

[Martin L Roth (Code Review)]

Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/34249?usp=email )

Change subject: cpu/x86: Initialize the argument value msr_index
......................................................................

Abandoned

This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.