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 <[e-mail]>
---
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;

-- 
To view, visit https://review.coreboot.org/c/coreboot/+/34249
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I051b290998207a1129ada06bb455f44c2d291975
Gerrit-Change-Number: 34249
Gerrit-PatchSet: 1
Gerrit-Owner: John Zhao <[e-mail]>
Gerrit-Reviewer: John Zhao <[e-mail]>
Gerrit-MessageType: newchange

[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


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/34249
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I051b290998207a1129ada06bb455f44c2d291975
Gerrit-Change-Number: 34249
Gerrit-PatchSet: 1
Gerrit-Owner: John Zhao <[e-mail]>
Gerrit-Reviewer: John Zhao <[e-mail]>
Gerrit-Reviewer: John Zhao <[e-mail]>
Gerrit-Reviewer: Lance Zhao <[e-mail]>
Gerrit-Reviewer: Lijian Zhao <[e-mail]>
Gerrit-Reviewer: Martin Roth <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Comment-Date: Fri, 12 Jul 2019 02:46:30 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[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?



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/34249
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I051b290998207a1129ada06bb455f44c2d291975
Gerrit-Change-Number: 34249
Gerrit-PatchSet: 1
Gerrit-Owner: John Zhao <[e-mail]>
Gerrit-Reviewer: John Zhao <[e-mail]>
Gerrit-Reviewer: John Zhao <[e-mail]>
Gerrit-Reviewer: Lance Zhao <[e-mail]>
Gerrit-Reviewer: Lijian Zhao <[e-mail]>
Gerrit-Reviewer: Martin Roth <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Comment-Date: Sat, 13 Jul 2019 18:19:11 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[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 <[e-mail]>
---
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
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/34249
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I051b290998207a1129ada06bb455f44c2d291975
Gerrit-Change-Number: 34249
Gerrit-PatchSet: 2
Gerrit-Owner: John Zhao <[e-mail]>
Gerrit-Reviewer: John Zhao <[e-mail]>
Gerrit-Reviewer: John Zhao <[e-mail]>
Gerrit-Reviewer: Lance Zhao <[e-mail]>
Gerrit-Reviewer: Lijian Zhao <[e-mail]>
Gerrit-Reviewer: Martin Roth <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-MessageType: newpatchset

[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



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/34249
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I051b290998207a1129ada06bb455f44c2d291975
Gerrit-Change-Number: 34249
Gerrit-PatchSet: 2
Gerrit-Owner: John Zhao <[e-mail]>
Gerrit-Reviewer: John Zhao <[e-mail]>
Gerrit-Reviewer: John Zhao <[e-mail]>
Gerrit-Reviewer: Lance Zhao <[e-mail]>
Gerrit-Reviewer: Lijian Zhao <[e-mail]>
Gerrit-Reviewer: Martin Roth <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Comment-Date: Tue, 16 Jul 2019 01:33:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Martin Roth <[e-mail]>
Gerrit-MessageType: comment

[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


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/34249
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I051b290998207a1129ada06bb455f44c2d291975
Gerrit-Change-Number: 34249
Gerrit-PatchSet: 2
Gerrit-Owner: John Zhao <[e-mail]>
Gerrit-Reviewer: John Zhao <[e-mail]>
Gerrit-Reviewer: John Zhao <[e-mail]>
Gerrit-Reviewer: Lance Zhao <[e-mail]>
Gerrit-Reviewer: Lijian Zhao <[e-mail]>
Gerrit-Reviewer: Martin Roth <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Comment-Date: Wed, 20 May 2020 16:48:46 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[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


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/34249
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I051b290998207a1129ada06bb455f44c2d291975
Gerrit-Change-Number: 34249
Gerrit-PatchSet: 3
Gerrit-Owner: John Zhao <[e-mail]>
Gerrit-Reviewer: John Zhao <[e-mail]>
Gerrit-Reviewer: John Zhao <[e-mail]>
Gerrit-Reviewer: Lance Zhao <[e-mail]>
Gerrit-Reviewer: Lijian Zhao <[e-mail]>
Gerrit-Reviewer: Martin Roth <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Comment-Date: Wed, 20 May 2020 16:51:49 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[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


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/34249
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I051b290998207a1129ada06bb455f44c2d291975
Gerrit-Change-Number: 34249
Gerrit-PatchSet: 3
Gerrit-Owner: John Zhao <[e-mail]>
Gerrit-Reviewer: John Zhao <[e-mail]>
Gerrit-Reviewer: John Zhao <[e-mail]>
Gerrit-Reviewer: Lance Zhao <[e-mail]>
Gerrit-Reviewer: Lijian Zhao <[e-mail]>
Gerrit-Reviewer: Martin Roth <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Thu, 21 May 2020 04:20:39 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[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


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/34249
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I051b290998207a1129ada06bb455f44c2d291975
Gerrit-Change-Number: 34249
Gerrit-PatchSet: 3
Gerrit-Owner: John Zhao <[e-mail]>
Gerrit-Reviewer: John Zhao <[e-mail]>
Gerrit-Reviewer: John Zhao <[e-mail]>
Gerrit-Reviewer: Lance Zhao <[e-mail]>
Gerrit-Reviewer: Lijian Zhao <[e-mail]>
Gerrit-Reviewer: Martin Roth <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Thu, 21 May 2020 06:13:42 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[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.
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/34249?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I051b290998207a1129ada06bb455f44c2d291975
Gerrit-Change-Number: 34249
Gerrit-PatchSet: 3
Gerrit-Owner: John Zhao <[e-mail]>
Gerrit-Reviewer: John Zhao <[e-mail]>
Gerrit-Reviewer: John Zhao <[e-mail]>
Gerrit-Reviewer: Lance Zhao <[e-mail]>
Gerrit-Reviewer: Lijian Zhao <[e-mail]>
Gerrit-Reviewer: Martin L Roth <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-MessageType: abandon