Change in coreboot[master]: soc/intel/common/block: Fix Klocwork issue

[Subrata Banik (Code Review)]

Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45411 )

Change subject: soc/intel/common/block: Fix Klocwork issue
......................................................................

soc/intel/common/block: Fix Klocwork issue

List of changes:
1. Add NULL check for pointer ctx
2. Initialize pointer soc_config with NULL

Signed-off-by: Subrata Banik 
Change-Id: I16015b538112e0b125b4a5e145c26263c456953c
---
M src/soc/intel/common/block/chip/chip.c
M src/soc/intel/common/block/cse/cse_lite.c
2 files changed, 4 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/45411/1

diff --git a/src/soc/intel/common/block/chip/chip.c b/src/soc/intel/common/block/chip/chip.c
index 09f2c47..47bee7c 100644
--- a/src/soc/intel/common/block/chip/chip.c
+++ b/src/soc/intel/common/block/chip/chip.c
@@ -5,7 +5,7 @@

const struct soc_intel_common_config *chip_get_common_soc_structure(void)
 {
-	const struct soc_intel_common_config *soc_config;
+	const struct soc_intel_common_config *soc_config = NULL;
    const config_t *config;

config = config_of_soc();
diff --git a/src/soc/intel/common/block/cse/cse_lite.c b/src/soc/intel/common/block/cse/cse_lite.c
index edb08da..17e8ef6 100644
--- a/src/soc/intel/common/block/cse/cse_lite.c
+++ b/src/soc/intel/common/block/cse/cse_lite.c
@@ -169,8 +169,9 @@
    /* Log CSE Firmware Status Registers to help debugging */
    cse_log_status_registers();
    if (CONFIG(VBOOT)) {
-		struct vb2_context *ctx;
-		ctx = vboot_get_context();
+		struct vb2_context *ctx = vboot_get_context();
+		if (ctx == NULL)
+			return;
    	vb2api_fail(ctx, VB2_RECOVERY_INTEL_CSE_LITE_SKU, rec_sub_code);
    	vboot_save_data(ctx);
    	vboot_reboot();

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45411 )

Change subject: soc/intel/common/block: Fix Klocwork issue
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/45411/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/45411/1//COMMIT_MSG@7 
PS1, Line 7: soc/intel/common/block: Fix Klocwork issue
It’d be great to describe the action in the git commit message summary.

https://review.coreboot.org/c/coreboot/+/45411/1//COMMIT_MSG@11 
PS1, Line 11: 2. Initialize pointer soc_config with NULL
It should be easiest to make one commit per change, and add `Found-by:` tags.

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45411 )

Change subject: soc/intel/common/block: Fix Klocwork issue
......................................................................

Patch Set 1:

(3 comments)

https://review.coreboot.org/c/coreboot/+/45411/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/45411/1//COMMIT_MSG@11 
PS1, Line 11: 2. Initialize pointer soc_config with NULL
> It should be easiest to make one commit per change, and add `Found-by:` tags.

I'd prefer one commit per change, too.

https://review.coreboot.org/c/coreboot/+/45411/1/src/soc/intel/common/block/... 
File src/soc/intel/common/block/chip/chip.c:

https://review.coreboot.org/c/coreboot/+/45411/1/src/soc/intel/common/block/... 
PS1, Line 8: const struct soc_intel_common_config *soc_config = NULL;
           : 	const config_t *config;
           : 
           : 	config = config_of_soc();
           : 	soc_config = &config->common_soc_config;
           : 
           : 	return soc_config;
This could just be `return &config_of_soc()->common_soc_config;`

https://review.coreboot.org/c/coreboot/+/45411/1/src/soc/intel/common/block/... 
File src/soc/intel/common/block/cse/cse_lite.c:

https://review.coreboot.org/c/coreboot/+/45411/1/src/soc/intel/common/block/... 
PS1, Line 174: return
I'd just skip over the `vb2api_fail` and `vboot_save_data` calls. Otherwise, there won't be a reboot.

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45411 )

Change subject: soc/intel/common/block: Fix Klocwork issue
......................................................................

Patch Set 1:

(4 comments)

https://review.coreboot.org/c/coreboot/+/45411/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/45411/1//COMMIT_MSG@7 
PS1, Line 7: soc/intel/common/block: Fix Klocwork issue
> It’d be great to describe the action in the git commit message summary.

Ack

https://review.coreboot.org/c/coreboot/+/45411/1//COMMIT_MSG@11 
PS1, Line 11: 2. Initialize pointer soc_config with NULL
> I'd prefer one commit per change, too.

Ack

https://review.coreboot.org/c/coreboot/+/45411/1/src/soc/intel/common/block/... 
File src/soc/intel/common/block/chip/chip.c:

https://review.coreboot.org/c/coreboot/+/45411/1/src/soc/intel/common/block/... 
PS1, Line 8: const struct soc_intel_common_config *soc_config = NULL;
           : 	const config_t *config;
           : 
           : 	config = config_of_soc();
           : 	soc_config = &config->common_soc_config;
           : 
           : 	return soc_config;
> This could just be `return &config_of_soc()->common_soc_config;`

Ack

https://review.coreboot.org/c/coreboot/+/45411/1/src/soc/intel/common/block/... 
File src/soc/intel/common/block/cse/cse_lite.c:

https://review.coreboot.org/c/coreboot/+/45411/1/src/soc/intel/common/block/... 
PS1, Line 174: return
> I'd just skip over the `vb2api_fail` and `vboot_save_data` calls. […]

Thanks a lot.

Change in coreboot[master]: soc/intel/common/block: Skip saving VBNV data if ctx is NULL

[Subrata Banik (Code Review)]

Hello build bot (Jenkins), Tim Wawrzynczak, Angel Pons, Patrick Rudolph, HAOUAS Elyes,

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#2).

Change subject: soc/intel/common/block: Skip saving VBNV data if ctx is NULL
......................................................................

soc/intel/common/block: Skip saving VBNV data if ctx is NULL

Found-by: Klocwork, Avoid NULL pointer (ctx) deference.

Signed-off-by: Subrata Banik 
Change-Id: I16015b538112e0b125b4a5e145c26263c456953c
---
M src/soc/intel/common/block/cse/cse_lite.c
1 file changed, 5 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/45411/2

Change in coreboot[master]: soc/intel/common/block: Skip saving VBNV data if ctx is NULL

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45411 )

Change subject: soc/intel/common/block: Skip saving VBNV data if ctx is NULL
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45411/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/45411/1//COMMIT_MSG@11 
PS1, Line 11: 2. Initialize pointer soc_config with NULL
> Ack

https://review.coreboot.org/c/coreboot/+/45416/1  other CL

Change in coreboot[master]: soc/intel/common/block: Skip saving VBNV data if ctx is NULL

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45411 )

Change subject: soc/intel/common/block: Skip saving VBNV data if ctx is NULL
......................................................................

Patch Set 2: Code-Review+1

Change in coreboot[master]: soc/intel/common/block: Skip saving VBNV data if ctx is NULL

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45411 )

Change subject: soc/intel/common/block: Skip saving VBNV data if ctx is NULL
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45411/1/src/soc/intel/common/block/... 
File src/soc/intel/common/block/chip/chip.c:

https://review.coreboot.org/c/coreboot/+/45411/1/src/soc/intel/common/block/... 
PS1, Line 8: const struct soc_intel_common_config *soc_config = NULL;
           : 	const config_t *config;
           : 
           : 	config = config_of_soc();
           : 	soc_config = &config->common_soc_config;
           : 
           : 	return soc_config;
> Ack

getting compilation error hence
https://review.coreboot.org/c/coreboot/+/45416/2/src/soc/intel/common/block/...

Change in coreboot[master]: soc/intel/common/block: Skip saving VBNV data if ctx is NULL

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45411 )

Change subject: soc/intel/common/block: Skip saving VBNV data if ctx is NULL
......................................................................

Patch Set 2:

Since we just use `vboot_get_context()` all over the place, often even doing `vboot_get_context()->flags` or other, maybe it would be more useful for static verification to just add an assert(vboot_ctx) in `src/security/vboot/common.c` at the end of `vboot_get_context()` just before the final return? Then we shouldn't need to add it everywhere to convince static analyzers it's non-NULL

Change in coreboot[master]: soc/intel/common/block: Skip saving VBNV data if ctx is NULL

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45411 )

Change subject: soc/intel/common/block: Skip saving VBNV data if ctx is NULL
......................................................................

Patch Set 2:
> Patch Set 2:
>
> Since we just use `vboot_get_context()` all over the place, often even doing `vboot_get_context()->flags` or other, maybe it would be more useful for static verification to just add an assert(vboot_ctx) in `src/security/vboot/common.c` at the end of `vboot_get_context()` just before the final return? Then we shouldn't need to add it everywhere to convince static analyzers it's non-NULL

Why are there vb2 calls which clearly have a side-effect inside assert statements?

Change in coreboot[master]: soc/intel/common/block: Skip saving VBNV data if ctx is NULL

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45411 )

Change subject: soc/intel/common/block: Skip saving VBNV data if ctx is NULL
......................................................................

Patch Set 2:
> Patch Set 2:
> > Patch Set 2:
> >
> > Since we just use `vboot_get_context()` all over the place, often even doing `vboot_get_context()->flags` or other, maybe it would be more useful for static verification to just add an assert(vboot_ctx) in `src/security/vboot/common.c` at the end of `vboot_get_context()` just before the final return? Then we shouldn't need to add it everywhere to convince static analyzers it's non-NULL
>
> Why are there vb2 calls which clearly have a side-effect inside assert statements?

It does look fishy, but technically, only one copy of the argument appears in the preprocessed source

Change in coreboot[master]: soc/intel/common/block: Skip saving VBNV data if ctx is NULL

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45411 )

Change subject: soc/intel/common/block: Skip saving VBNV data if ctx is NULL
......................................................................

Patch Set 2:
> Patch Set 2:
> > Patch Set 2:
> > > Patch Set 2:
> > >
> > > Since we just use `vboot_get_context()` all over the place, often even doing `vboot_get_context()->flags` or other, maybe it would be more useful for static verification to just add an assert(vboot_ctx) in `src/security/vboot/common.c` at the end of `vboot_get_context()` just before the final return? Then we shouldn't need to add it everywhere to convince static analyzers it's non-NULL
> >
> > Why are there vb2 calls which clearly have a side-effect inside assert statements?
>
> It does look fishy, but technically, only one copy of the argument appears in the preprocessed source

Well 2 appear, but GCC will eliminate one at build-time

Change in coreboot[master]: soc/intel/common/block: Skip saving VBNV data if ctx is NULL

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45411 )

Change subject: soc/intel/common/block: Skip saving VBNV data if ctx is NULL
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45411/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/45411/2//COMMIT_MSG@9 
PS2, Line 9: deference
dereference

Change in coreboot[master]: soc/intel/common/block: Skip saving VBNV data if ctx is NULL

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45411 )

Change subject: soc/intel/common/block: Skip saving VBNV data if ctx is NULL
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45411/2/src/soc/intel/common/block/... 
File src/soc/intel/common/block/cse/cse_lite.c:

https://review.coreboot.org/c/coreboot/+/45411/2/src/soc/intel/common/block/... 
PS2, Line 177: vboot_reboot
I think this should happen only when we are able to save context and trigger recovery mode. If for any reason that fails, we should drop down to the die below.

Ideally, the condition that vboot context is not available should not happen and if it does, then we probably have some other critical hardware issue which probably wouldn't be fixed by rebooting. So, instead of getting stuck in a reboot loop, we can just die here. I think we might see this mostly during development/testing if it ever happens.

Change in coreboot[master]: soc/intel/common/block: Add NULL check for 'ctx' pointer

[Subrata Banik (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Angel Pons, Aaron Durbin, Patrick Rudolph, HAOUAS Elyes,

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#3).

Change subject: soc/intel/common/block: Add NULL check for 'ctx' pointer
......................................................................

soc/intel/common/block: Add NULL check for 'ctx' pointer

Found-by: Klocwork, Avoid NULL pointer (ctx) dereference.

Signed-off-by: Subrata Banik 
Change-Id: I16015b538112e0b125b4a5e145c26263c456953c
---
M src/soc/intel/common/block/cse/cse_lite.c
1 file changed, 4 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/45411/3

Change in coreboot[master]: soc/intel/common/block: Add NULL check for 'ctx' pointer

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45411 )

Change subject: soc/intel/common/block: Add NULL check for 'ctx' pointer
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/c/coreboot/+/45411/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/45411/2//COMMIT_MSG@9 
PS2, Line 9: deference
> dereference

Ack

https://review.coreboot.org/c/coreboot/+/45411/2/src/soc/intel/common/block/... 
File src/soc/intel/common/block/cse/cse_lite.c:

https://review.coreboot.org/c/coreboot/+/45411/2/src/soc/intel/common/block/... 
PS2, Line 177: vboot_reboot
> I think this should happen only when we are able to save context and trigger recovery mode. […]

Ack

Change in coreboot[master]: soc/intel/common/block: Add NULL check for 'ctx' pointer

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45411 )

Change subject: soc/intel/common/block: Add NULL check for 'ctx' pointer
......................................................................

Patch Set 3: Code-Review+2

Change in coreboot[master]: soc/intel/common/block: Add NULL check for 'ctx' pointer

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45411 )

Change subject: soc/intel/common/block: Add NULL check for 'ctx' pointer
......................................................................

Patch Set 3: Code-Review+2

Change in coreboot[master]: soc/intel/common/block: Add NULL check for 'ctx' pointer

[Subrata Banik (Code Review)]

Subrata Banik has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45411 )

Change subject: soc/intel/common/block: Add NULL check for 'ctx' pointer
......................................................................

soc/intel/common/block: Add NULL check for 'ctx' pointer

Found-by: Klocwork, Avoid NULL pointer (ctx) dereference.

Signed-off-by: Subrata Banik 
Change-Id: I16015b538112e0b125b4a5e145c26263c456953c
Reviewed-on: https://review.coreboot.org/c/coreboot/+/45411
Reviewed-by: Furquan Shaikh 
Reviewed-by: Angel Pons 
Tested-by: build bot (Jenkins) 
---
M src/soc/intel/common/block/cse/cse_lite.c
1 file changed, 4 insertions(+), 3 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: Looks good to me, approved
  Angel Pons: Looks good to me, approved

diff --git a/src/soc/intel/common/block/cse/cse_lite.c b/src/soc/intel/common/block/cse/cse_lite.c
index edb08da..7daa35e 100644
--- a/src/soc/intel/common/block/cse/cse_lite.c
+++ b/src/soc/intel/common/block/cse/cse_lite.c
@@ -169,13 +169,14 @@
    /* Log CSE Firmware Status Registers to help debugging */
    cse_log_status_registers();
    if (CONFIG(VBOOT)) {
-		struct vb2_context *ctx;
-		ctx = vboot_get_context();
+		struct vb2_context *ctx = vboot_get_context();
+		if (ctx == NULL)
+			goto failure;
    	vb2api_fail(ctx, VB2_RECOVERY_INTEL_CSE_LITE_SKU, rec_sub_code);
    	vboot_save_data(ctx);
    	vboot_reboot();
    }
-
+failure:
    die("cse_lite: Failed to trigger recovery mode(recovery subcode:%d)\n", rec_sub_code);
 }

Change in coreboot[master]: soc/intel/common/block: Add NULL check for 'ctx' pointer

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45411 )

Change subject: soc/intel/common/block: Add NULL check for 'ctx' pointer
......................................................................

Patch Set 4:

Automatic boot test returned (PASS/FAIL/TOTAL): 8/1/9
"QEMU x86 q35/ich9" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/19686
"QEMU x86 q35/ich9" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19685
"QEMU x86 i440fx/piix4" (x86_64) using payload SeaBIOS : FAIL : https://lava.9esec.io/r/19684
"QEMU x86 i440fx/piix4" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19683
"QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/19682
"HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/19690
"HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/19689
"HP Compaq 8200 Elite SFF PC" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/19688
"HP Compaq 8200 Elite SFF PC" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19687

Please note: This test is under development and might not be accurate at all!