Change in coreboot[master]: security/vboot: relocate vb2ex_abort and vb2ex_printf

List overview All Threads
Download

newer

older

Change in coreboot[master]:...

Joel Kitching (Code Review)

4 Feb 2020 4 Feb '20

10:44 a.m.

Joel Kitching has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38706 )

Change subject: security/vboot: relocate vb2ex_abort and vb2ex_printf ......................................................................

security/vboot: relocate vb2ex_abort and vb2ex_printf

Enabling an assertion in vb2_member_of() results in coreboot linking vb2ex_abort() and vb2ex_printf() in ramstage.

Move these two functions from vboot_logic.c to vboot_common.c, which is linked into ramstage.

Relevant vboot_reference commit: CL:2037263.

BUG=b:124141368, chromium:1005700 TEST=make clean && make test-abuild BRANCH=none

Change-Id: Ica0103c5684b3d50ba7dc1b4c39559cb192efa81 Signed-off-by: Joel Kitching kitching@google.com --- M src/security/vboot/vboot_common.c M src/security/vboot/vboot_logic.c 2 files changed, 23 insertions(+), 21 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/38706/1

diff --git a/src/security/vboot/vboot_common.c b/src/security/vboot/vboot_common.c index 3342524..73aefe2 100644 --- a/src/security/vboot/vboot_common.c +++ b/src/security/vboot/vboot_common.c @@ -16,6 +16,8 @@ #include <boot_device.h> #include <cbmem.h> #include <console/cbmem_console.h> +#include <console/console.h> +#include <console/vtxprintf.h> #include <fmap.h> #include <reset.h> #include <stddef.h> @@ -56,3 +58,24 @@ vboot_platform_prepare_reboot(); board_reset(); } + +/* exports */ + +void vb2ex_printf(const char *func, const char *fmt, ...) +{ + va_list args; + + if (func) + printk(BIOS_INFO, "VB2:%s() ", func); + + va_start(args, fmt); + vprintk(BIOS_INFO, fmt, args); + va_end(args); + + return; +} + +void vb2ex_abort(void) +{ + die("vboot has aborted execution; exit\n"); +} diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c index 1d17a17..182128c 100644 --- a/src/security/vboot/vboot_logic.c +++ b/src/security/vboot/vboot_logic.c @@ -17,8 +17,6 @@ #include <assert.h> #include <bootmode.h> #include <cbmem.h> -#include <console/console.h> -#include <console/vtxprintf.h> #include <fmap.h> #include <string.h> #include <timestamp.h> @@ -37,20 +35,6 @@

/* exports */

-void vb2ex_printf(const char *func, const char *fmt, ...) -{ - va_list args; - - if (func) - printk(BIOS_INFO, "VB2:%s() ", func); - - va_start(args, fmt); - vprintk(BIOS_INFO, fmt, args); - va_end(args); - - return; -} - vb2_error_t vb2ex_read_resource(struct vb2_context *ctx, enum vb2_resource_index index, uint32_t offset, @@ -83,11 +67,6 @@ return VB2_SUCCESS; }

Attachments:

attachment.htm (text/html — 6.9 KB)

Show replies by date

build bot (Jenkins) (Code Review)

4 Feb 4 Feb

10:44 a.m.

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38706 )

Change subject: security/vboot: relocate vb2ex_abort and vb2ex_printf ......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38706/1/src/security/vboot/vboot_co... File src/security/vboot/vboot_common.c:

https://review.coreboot.org/c/coreboot/+/38706/1/src/security/vboot/vboot_co... PS1, Line 76: } void function return statements are not generally useful

Joel Kitching (Code Review)

11:30 a.m.

Hello Aaron Durbin, Julius Werner, build bot (Jenkins),

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

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

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

Change subject: security/vboot: relocate vb2ex_abort and vb2ex_printf ......................................................................

security/vboot: relocate vb2ex_abort and vb2ex_printf

Enabling an assertion in vb2_member_of() results in coreboot linking vb2ex_abort() and vb2ex_printf() in ramstage.

Move these two functions from vboot_logic.c to vboot_common.c, which is linked into ramstage.

Relevant vboot_reference commit: CL:2037263.

BUG=b:124141368, chromium:1005700 TEST=make clean && make test-abuild BRANCH=none

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/38706/2

Joel Kitching (Code Review)

11:33 a.m.

Hello Aaron Durbin, Julius Werner, build bot (Jenkins),

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

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

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

Change subject: security/vboot: relocate vb2ex_abort and vb2ex_printf ......................................................................

security/vboot: relocate vb2ex_abort and vb2ex_printf

Enabling an assertion in vb2_member_of() results in coreboot linking vb2ex_abort() and vb2ex_printf() in ramstage.

Move these two functions from vboot_logic.c to vboot_common.c, which is linked into ramstage.

Relevant vboot_reference commit: CL:2037263.

BUG=b:124141368, chromium:1005700 TEST=make clean && make test-abuild BRANCH=none

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/38706/3

Joel Kitching (Code Review)

11:33 a.m.

Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38706 )

Change subject: security/vboot: relocate vb2ex_abort and vb2ex_printf ......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38706/1/src/security/vboot/vboot_co... File src/security/vboot/vboot_common.c:

https://review.coreboot.org/c/coreboot/+/38706/1/src/security/vboot/vboot_co... PS1, Line 76: }

...

void function return statements are not generally useful

Good point. Let's just remove that.

-- To view, visit https://review.coreboot.org/c/coreboot/+/38706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ica0103c5684b3d50ba7dc1b4c39559cb192efa81 Gerrit-Change-Number: 38706 Gerrit-PatchSet: 3 Gerrit-Owner: Joel Kitching kitching@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Tue, 04 Feb 2020 10:33:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: build bot (Jenkins) no-reply@coreboot.org Gerrit-MessageType: comment

Frans Hendriks (Code Review)

4:01 p.m.

Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38706 )

Change subject: security/vboot: relocate vb2ex_abort and vb2ex_printf ......................................................................

Patch Set 3:

Would adding vb2ex_abort() and vb2ex_printf() to vboot library be the preferred solution?

-- To view, visit https://review.coreboot.org/c/coreboot/+/38706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ica0103c5684b3d50ba7dc1b4c39559cb192efa81 Gerrit-Change-Number: 38706 Gerrit-PatchSet: 3 Gerrit-Owner: Joel Kitching kitching@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Frans Hendriks fhendriks@eltan.com Gerrit-Comment-Date: Tue, 04 Feb 2020 15:01:24 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Joel Kitching (Code Review)

5:28 p.m.

Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38706 )

Change subject: security/vboot: relocate vb2ex_abort and vb2ex_printf ......................................................................

Patch Set 3:

...

Patch Set 3:

Would adding vb2ex_abort() and vb2ex_printf() to vboot library be the preferred solution?

By design, the "vb2ex" functions are supposed to be implemented by the caller. These are mostly lower-level functions interacting with drivers or hardware in some way. This also allows implementation to differ across the firmware being linked, i.e. coreboot or depthcharge.

-- To view, visit https://review.coreboot.org/c/coreboot/+/38706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ica0103c5684b3d50ba7dc1b4c39559cb192efa81 Gerrit-Change-Number: 38706 Gerrit-PatchSet: 3 Gerrit-Owner: Joel Kitching kitching@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Frans Hendriks fhendriks@eltan.com Gerrit-Comment-Date: Tue, 04 Feb 2020 16:28:40 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Joel Kitching (Code Review)

5:45 p.m.

Hello Aaron Durbin, Julius Werner, build bot (Jenkins), Patrick Georgi, Martin Roth,

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

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

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

Change subject: security/vboot: relocate vb2ex_abort and vb2ex_printf ......................................................................

security/vboot: relocate vb2ex_abort and vb2ex_printf

Enabling an assertion in vb2_member_of() results in coreboot linking vb2ex_abort() and vb2ex_printf() in ramstage.

Move these two functions from vboot_logic.c to vboot_common.c, which is linked into ramstage.

Relevant vboot_reference commit: CL:2037263.

BUG=b:124141368, chromium:1005700 TEST=make clean && make test-abuild BRANCH=none

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/38706/4

-- To view, visit https://review.coreboot.org/c/coreboot/+/38706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ica0103c5684b3d50ba7dc1b4c39559cb192efa81 Gerrit-Change-Number: 38706 Gerrit-PatchSet: 4 Gerrit-Owner: Joel Kitching kitching@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Frans Hendriks fhendriks@eltan.com Gerrit-MessageType: newpatchset

Aaron Durbin (Code Review)

7:04 p.m.

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38706 )

Change subject: security/vboot: relocate vb2ex_abort and vb2ex_printf ......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38706/4/src/security/vboot/vboot_co... File src/security/vboot/vboot_common.c:

https://review.coreboot.org/c/coreboot/+/38706/4/src/security/vboot/vboot_co... PS4, Line 29: #if CONFIG(VBOOT) Why does this need to be guarded? If this function isn't used it will get dropped at link time.

-- To view, visit https://review.coreboot.org/c/coreboot/+/38706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ica0103c5684b3d50ba7dc1b4c39559cb192efa81 Gerrit-Change-Number: 38706 Gerrit-PatchSet: 4 Gerrit-Owner: Joel Kitching kitching@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Frans Hendriks fhendriks@eltan.com Gerrit-Comment-Date: Tue, 04 Feb 2020 18:04:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Julius Werner (Code Review)

9:38 p.m.

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38706 )

Change subject: security/vboot: relocate vb2ex_abort and vb2ex_printf ......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38706/4//COMMIT_MSG Commit Message:

https://review.coreboot.org/c/coreboot/+/38706/4//COMMIT_MSG@13 PS4, Line 13: which is linked into ramstage. Maybe go even further and change compilation of vboot_common.c to be guarded by CONFIG_VBOOT_LIB, not CONFIG_VBOOT? These two callbacks are so generic that they may eventually come up in use cases of the vboot library that aren't tied to verification.

-- To view, visit https://review.coreboot.org/c/coreboot/+/38706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ica0103c5684b3d50ba7dc1b4c39559cb192efa81 Gerrit-Change-Number: 38706 Gerrit-PatchSet: 4 Gerrit-Owner: Joel Kitching kitching@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Frans Hendriks fhendriks@eltan.com Gerrit-Comment-Date: Tue, 04 Feb 2020 20:38:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Joel Kitching (Code Review)

11:23 p.m.

Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38706 )

Change subject: security/vboot: relocate vb2ex_abort and vb2ex_printf ......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/c/coreboot/+/38706/4//COMMIT_MSG Commit Message:

https://review.coreboot.org/c/coreboot/+/38706/4//COMMIT_MSG@13 PS4, Line 13: which is linked into ramstage.

...

Maybe go even further and change compilation of vboot_common. […]

I agree -- but perhaps we should hold off on this until we have cleaned up/merged security/vboot/common.c and security/vboot/vboot_common.c?

Or perhaps we can have one file for "vboot common" and one for "vboot lib common"?

https://review.coreboot.org/c/coreboot/+/38706/4/src/security/vboot/vboot_co... File src/security/vboot/vboot_common.c:

https://review.coreboot.org/c/coreboot/+/38706/4/src/security/vboot/vboot_co... PS4, Line 29: #if CONFIG(VBOOT)

...

Why does this need to be guarded? If this function isn't used it will get dropped at link time.

This is to match the CONFIG(VBOOT) conditional in vboot_common.h. Unfortunately the set of functions defined in the conditional are spread all over the place. The "vboot utility code" in coreboot could probably use some shuffling around.

https://qa.coreboot.org/job/coreboot-gerrit/115915/ failure:

src/security/vboot/vboot_common.c:30:5: error: redefinition of 'vboot_can_enable_udc' int vboot_can_enable_udc(void) ^~~~~~~~~~~~~~~~~~~~ In file included from src/security/vboot/misc.h:20, from src/security/vboot/vboot_common.c:24: src/security/vboot/vboot_common.h:80:19: note: previous definition of 'vboot_can_enable_udc' was here static inline int vboot_can_enable_udc(void) { return 1; }

-- To view, visit https://review.coreboot.org/c/coreboot/+/38706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ica0103c5684b3d50ba7dc1b4c39559cb192efa81 Gerrit-Change-Number: 38706 Gerrit-PatchSet: 4 Gerrit-Owner: Joel Kitching kitching@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Frans Hendriks fhendriks@eltan.com Gerrit-Comment-Date: Tue, 04 Feb 2020 22:23:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aaron Durbin adurbin@chromium.org Comment-In-Reply-To: Julius Werner jwerner@chromium.org Gerrit-MessageType: comment

Aaron Durbin (Code Review)

11:33 p.m.

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38706 )

Change subject: security/vboot: relocate vb2ex_abort and vb2ex_printf ......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38706/4/src/security/vboot/vboot_co... File src/security/vboot/vboot_common.c:

https://review.coreboot.org/c/coreboot/+/38706/4/src/security/vboot/vboot_co... PS4, Line 29: #if CONFIG(VBOOT)

...

This is to match the CONFIG(VBOOT) conditional in vboot_common.h. […]

Oh. Because of the different definitions and guards. I see. I just assumed if it wasn't used it wouldn't be linked in. nm.

-- To view, visit https://review.coreboot.org/c/coreboot/+/38706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ica0103c5684b3d50ba7dc1b4c39559cb192efa81 Gerrit-Change-Number: 38706 Gerrit-PatchSet: 4 Gerrit-Owner: Joel Kitching kitching@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Frans Hendriks fhendriks@eltan.com Gerrit-Comment-Date: Tue, 04 Feb 2020 22:33:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aaron Durbin adurbin@chromium.org Comment-In-Reply-To: Joel Kitching kitching@google.com Gerrit-MessageType: comment

Julius Werner (Code Review)

11:42 p.m.

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38706 )

Change subject: security/vboot: relocate vb2ex_abort and vb2ex_printf ......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38706/4/src/security/vboot/vboot_co... File src/security/vboot/vboot_common.c:

https://review.coreboot.org/c/coreboot/+/38706/4/src/security/vboot/vboot_co... PS4, Line 29: #if CONFIG(VBOOT)

...

Oh. Because of the different definitions and guards. I see. […]

Well, then this is probably just not the right file to add this? Maybe just make a new one (vboot_lib.c or whatever) for these for now and make it conditional on CONFIG_VBOOT_LIB. I agree that we should still do more consolidation and clarification of what belongs where at some point, but can leave that to a later patch. I think I'd rather have too many files in the short term than random #ifdefs spread around.

-- To view, visit https://review.coreboot.org/c/coreboot/+/38706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ica0103c5684b3d50ba7dc1b4c39559cb192efa81 Gerrit-Change-Number: 38706 Gerrit-PatchSet: 4 Gerrit-Owner: Joel Kitching kitching@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Frans Hendriks fhendriks@eltan.com Gerrit-Comment-Date: Tue, 04 Feb 2020 22:42:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aaron Durbin adurbin@chromium.org Comment-In-Reply-To: Joel Kitching kitching@google.com Gerrit-MessageType: comment

Joel Kitching (Code Review)

5 Feb 5 Feb

10:25 a.m.

Hello Aaron Durbin, Julius Werner, build bot (Jenkins), Patrick Georgi, Martin Roth,

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

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

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

Change subject: security/vboot: relocate vb2ex_abort and vb2ex_printf ......................................................................

security/vboot: relocate vb2ex_abort and vb2ex_printf

Enabling an assertion in vb2_member_of() results in coreboot linking vb2ex_abort() and vb2ex_printf() in ramstage.

Move these two functions from vboot_logic.c to vboot_lib.c, which is should be enabled in all stages if CONFIG_VBOOT_LIB is enabled. Note that CONFIG_VBOOT_LIB is implied by CONFIG_VBOOT.

Relevant vboot_reference commit: CL:2037263.

BUG=b:124141368, chromium:1005700 TEST=make clean && make test-abuild BRANCH=none

Change-Id: Ica0103c5684b3d50ba7dc1b4c39559cb192efa81 Signed-off-by: Joel Kitching kitching@google.com --- M src/security/vboot/Makefile.inc A src/security/vboot/vboot_lib.c M src/security/vboot/vboot_logic.c M src/vendorcode/eltan/security/lib/Makefile.inc 4 files changed, 46 insertions(+), 22 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/38706/5

-- To view, visit https://review.coreboot.org/c/coreboot/+/38706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ica0103c5684b3d50ba7dc1b4c39559cb192efa81 Gerrit-Change-Number: 38706 Gerrit-PatchSet: 5 Gerrit-Owner: Joel Kitching kitching@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Frans Hendriks fhendriks@eltan.com Gerrit-MessageType: newpatchset

Joel Kitching (Code Review)

10:25 a.m.

Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38706 )

Change subject: security/vboot: relocate vb2ex_abort and vb2ex_printf ......................................................................

Patch Set 5:

(2 comments)

https://review.coreboot.org/c/coreboot/+/38706/4//COMMIT_MSG Commit Message:

https://review.coreboot.org/c/coreboot/+/38706/4//COMMIT_MSG@13 PS4, Line 13: which is linked into ramstage.

...

I agree -- but perhaps we should hold off on this until we have cleaned up/merged security/vboot/com […]

Adding vboot_lib.c as suggested in other comment.

https://review.coreboot.org/c/coreboot/+/38706/4/src/security/vboot/vboot_co... File src/security/vboot/vboot_common.c:

https://review.coreboot.org/c/coreboot/+/38706/4/src/security/vboot/vboot_co... PS4, Line 29: #if CONFIG(VBOOT)

...

Well, then this is probably just not the right file to add this? Maybe just make a new one (vboot_li […]

Sure -- let's do that.

-- To view, visit https://review.coreboot.org/c/coreboot/+/38706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ica0103c5684b3d50ba7dc1b4c39559cb192efa81 Gerrit-Change-Number: 38706 Gerrit-PatchSet: 5 Gerrit-Owner: Joel Kitching kitching@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Frans Hendriks fhendriks@eltan.com Gerrit-Comment-Date: Wed, 05 Feb 2020 09:25:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aaron Durbin adurbin@chromium.org Comment-In-Reply-To: Julius Werner jwerner@chromium.org Comment-In-Reply-To: Joel Kitching kitching@google.com Gerrit-MessageType: comment

Frans Hendriks (Code Review)

11:17 a.m.

Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38706 )

Change subject: security/vboot: relocate vb2ex_abort and vb2ex_printf ......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38706/5/src/vendorcode/eltan/securi... File src/vendorcode/eltan/security/lib/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/38706/5/src/vendorcode/eltan/securi... PS5, Line 45: bootblock-y += ../../../../security/vboot/vboot_lib.c vboot_lib.c should replace vboot_logic.c. Can vboot_logic.c be removed?

-- To view, visit https://review.coreboot.org/c/coreboot/+/38706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ica0103c5684b3d50ba7dc1b4c39559cb192efa81 Gerrit-Change-Number: 38706 Gerrit-PatchSet: 5 Gerrit-Owner: Joel Kitching kitching@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Frans Hendriks fhendriks@eltan.com Gerrit-Comment-Date: Wed, 05 Feb 2020 10:17:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Joel Kitching (Code Review)

11:24 a.m.

Hello Aaron Durbin, Julius Werner, build bot (Jenkins), Patrick Georgi, Martin Roth,

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

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

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

Change subject: security/vboot: relocate vb2ex_abort and vb2ex_printf ......................................................................

security/vboot: relocate vb2ex_abort and vb2ex_printf

Enabling an assertion in vb2_member_of() results in coreboot linking vb2ex_abort() and vb2ex_printf() in ramstage.

Move these two functions from vboot_logic.c to vboot_lib.c, which is should be enabled in all stages if CONFIG_VBOOT_LIB is enabled. Note that CONFIG_VBOOT_LIB is implied by CONFIG_VBOOT.

Relevant vboot_reference commit: CL:2037263.

BUG=b:124141368, chromium:1005700 TEST=make clean && make test-abuild BRANCH=none

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/38706/6

-- To view, visit https://review.coreboot.org/c/coreboot/+/38706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ica0103c5684b3d50ba7dc1b4c39559cb192efa81 Gerrit-Change-Number: 38706 Gerrit-PatchSet: 6 Gerrit-Owner: Joel Kitching kitching@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Frans Hendriks fhendriks@eltan.com Gerrit-MessageType: newpatchset

Joel Kitching (Code Review)

11:25 a.m.

Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38706 )

Change subject: security/vboot: relocate vb2ex_abort and vb2ex_printf ......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38706/5/src/vendorcode/eltan/securi... File src/vendorcode/eltan/security/lib/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/38706/5/src/vendorcode/eltan/securi... PS5, Line 45: bootblock-y += ../../../../security/vboot/vboot_lib.c

...

vboot_lib.c should replace vboot_logic.c. […]

Yes, it seems like all Eltan needs here to be happy is vb2ex_printf.

-- To view, visit https://review.coreboot.org/c/coreboot/+/38706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ica0103c5684b3d50ba7dc1b4c39559cb192efa81 Gerrit-Change-Number: 38706 Gerrit-PatchSet: 6 Gerrit-Owner: Joel Kitching kitching@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Frans Hendriks fhendriks@eltan.com Gerrit-Comment-Date: Wed, 05 Feb 2020 10:25:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Frans Hendriks fhendriks@eltan.com Gerrit-MessageType: comment

Aaron Durbin (Code Review)

5:06 p.m.

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38706 )

Change subject: security/vboot: relocate vb2ex_abort and vb2ex_printf ......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38706/6/src/security/vboot/vboot_li... File src/security/vboot/vboot_lib.c:

https://review.coreboot.org/c/coreboot/+/38706/6/src/security/vboot/vboot_li... PS6, Line 4: * Copyright 2020 The Chromium OS Authors. All rights reserved. we should move to the spdx headers for copyright. Grep for SPDX-License-Identifer for an example.

-- To view, visit https://review.coreboot.org/c/coreboot/+/38706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ica0103c5684b3d50ba7dc1b4c39559cb192efa81 Gerrit-Change-Number: 38706 Gerrit-PatchSet: 6 Gerrit-Owner: Joel Kitching kitching@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Frans Hendriks fhendriks@eltan.com Gerrit-Comment-Date: Wed, 05 Feb 2020 16:06:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Julius Werner (Code Review)

10:03 p.m.

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38706 )

Change subject: security/vboot: relocate vb2ex_abort and vb2ex_printf ......................................................................

Patch Set 6: Code-Review+2

(3 comments)

https://review.coreboot.org/c/coreboot/+/38706/6/src/security/vboot/Makefile... File src/security/vboot/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/38706/6/src/security/vboot/Makefile... PS6, Line 61: verstage-y += vboot_lib.c nit: There's no need to put this down here, the verstage class won't be used by any rules when not building for vboot but it doesn't hurt to still add files to it. I think it's more easy to read if you always put the lines of all classes for the same file together. (In fact, I've frequently thought we should add an 'all' class as a shorthand for all that library code that we want available everywhere, but that's one of those things you never actually end up finding time for.)

https://review.coreboot.org/c/coreboot/+/38706/6/src/security/vboot/vboot_li... File src/security/vboot/vboot_lib.c:

...

we should move to the spdx headers for copyright. Grep for SPDX-License-Identifer for an example.

I'm somewhat confused what's going on there actually, I thought the plan was to migrate whole directories at a time (so not every patch author needs to worry about it until their directory was migrated and the linter starts reminding them to), but it seems like nothing is happening anymore... (same as with the 96 character thing).

https://review.coreboot.org/c/coreboot/+/38706/6/src/security/vboot/vboot_li... PS6, Line 20: /* vboot_reference callbacks implemented by coreboot. */ nit: maybe be a bit more specific what separates these callbacks from the one implemented in vboot_logic.c?

-- To view, visit https://review.coreboot.org/c/coreboot/+/38706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ica0103c5684b3d50ba7dc1b4c39559cb192efa81 Gerrit-Change-Number: 38706 Gerrit-PatchSet: 6 Gerrit-Owner: Joel Kitching kitching@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Frans Hendriks fhendriks@eltan.com Gerrit-Comment-Date: Wed, 05 Feb 2020 21:03:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Aaron Durbin adurbin@chromium.org Gerrit-MessageType: comment

Patrick Georgi (Code Review)

10:13 p.m.

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38706 )

Change subject: security/vboot: relocate vb2ex_abort and vb2ex_printf ......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38706/6/src/security/vboot/vboot_li... File src/security/vboot/vboot_lib.c:

...

I'm somewhat confused what's going on there actually, I thought the plan was to migrate whole direct […]

So yes, this is still planned, but people are busy with other things. As for the license header, we can accept both right now. Things will work through the transition.

-- To view, visit https://review.coreboot.org/c/coreboot/+/38706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ica0103c5684b3d50ba7dc1b4c39559cb192efa81 Gerrit-Change-Number: 38706 Gerrit-PatchSet: 6 Gerrit-Owner: Joel Kitching kitching@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Frans Hendriks fhendriks@eltan.com Gerrit-Comment-Date: Wed, 05 Feb 2020 21:13:11 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aaron Durbin adurbin@chromium.org Comment-In-Reply-To: Julius Werner jwerner@chromium.org Gerrit-MessageType: comment

Joel Kitching (Code Review)

6 Feb 6 Feb

1:27 a.m.

Hello Aaron Durbin, Julius Werner, build bot (Jenkins), Patrick Georgi, Martin Roth,

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

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

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

Change subject: security/vboot: relocate vb2ex_abort and vb2ex_printf ......................................................................

security/vboot: relocate vb2ex_abort and vb2ex_printf

Enabling an assertion in vb2_member_of() results in coreboot linking vb2ex_abort() and vb2ex_printf() in ramstage.

Move these two functions from vboot_logic.c to vboot_lib.c, which is should be enabled in all stages if CONFIG_VBOOT_LIB is enabled. Note that CONFIG_VBOOT_LIB is implied by CONFIG_VBOOT.

Relevant vboot_reference commit: CL:2037263.

BUG=b:124141368, chromium:1005700 TEST=make clean && make test-abuild BRANCH=none

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/38706/7

-- To view, visit https://review.coreboot.org/c/coreboot/+/38706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ica0103c5684b3d50ba7dc1b4c39559cb192efa81 Gerrit-Change-Number: 38706 Gerrit-PatchSet: 7 Gerrit-Owner: Joel Kitching kitching@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Frans Hendriks fhendriks@eltan.com Gerrit-MessageType: newpatchset

Joel Kitching (Code Review)

1:28 a.m.

Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38706 )

Change subject: security/vboot: relocate vb2ex_abort and vb2ex_printf ......................................................................

Patch Set 7:

(2 comments)

https://review.coreboot.org/c/coreboot/+/38706/6/src/security/vboot/Makefile... File src/security/vboot/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/38706/6/src/security/vboot/Makefile... PS6, Line 61: verstage-y += vboot_lib.c

...

nit: There's no need to put this down here, the verstage class won't be used by any rules when not b […]

Done. Now the question becomes, what order should they be listed (=

https://review.coreboot.org/c/coreboot/+/38706/6/src/security/vboot/vboot_li... File src/security/vboot/vboot_lib.c:

...

So yes, this is still planned, but people are busy with other things. […]

Done

-- To view, visit https://review.coreboot.org/c/coreboot/+/38706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ica0103c5684b3d50ba7dc1b4c39559cb192efa81 Gerrit-Change-Number: 38706 Gerrit-PatchSet: 7 Gerrit-Owner: Joel Kitching kitching@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Frans Hendriks fhendriks@eltan.com Gerrit-Comment-Date: Thu, 06 Feb 2020 00:28:11 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aaron Durbin adurbin@chromium.org Comment-In-Reply-To: Julius Werner jwerner@chromium.org Comment-In-Reply-To: Patrick Georgi pgeorgi@google.com Gerrit-MessageType: comment

Julius Werner (Code Review)

1:43 a.m.

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38706 )

Change subject: security/vboot: relocate vb2ex_abort and vb2ex_printf ......................................................................

Patch Set 7: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/38706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ica0103c5684b3d50ba7dc1b4c39559cb192efa81 Gerrit-Change-Number: 38706 Gerrit-PatchSet: 7 Gerrit-Owner: Joel Kitching kitching@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Frans Hendriks fhendriks@eltan.com Gerrit-Comment-Date: Thu, 06 Feb 2020 00:43:29 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Frans Hendriks (Code Review)

8:04 a.m.

Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38706 )

Change subject: security/vboot: relocate vb2ex_abort and vb2ex_printf ......................................................................

Patch Set 7: Code-Review+1

-- To view, visit https://review.coreboot.org/c/coreboot/+/38706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ica0103c5684b3d50ba7dc1b4c39559cb192efa81 Gerrit-Change-Number: 38706 Gerrit-PatchSet: 7 Gerrit-Owner: Joel Kitching kitching@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Frans Hendriks fhendriks@eltan.com Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Thu, 06 Feb 2020 07:04:01 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Joel Kitching (Code Review)

7 Feb 7 Feb

4:21 a.m.

Hello Aaron Durbin, Julius Werner, Frans Hendriks, build bot (Jenkins), Patrick Georgi, Martin Roth,

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

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

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

Change subject: security/vboot: relocate vb2ex_abort and vb2ex_printf ......................................................................

security/vboot: relocate vb2ex_abort and vb2ex_printf

Enabling an assertion in vb2_member_of() results in coreboot linking vb2ex_abort() and vb2ex_printf() in ramstage.

Move these two functions from vboot_logic.c to vboot_lib.c, which is should be enabled in all stages if CONFIG_VBOOT_LIB is enabled. Note that CONFIG_VBOOT_LIB is implied by CONFIG_VBOOT.

Relevant vboot_reference commit: CL:2037263.

BUG=b:124141368, chromium:1005700 TEST=make clean && make test-abuild BRANCH=none

Change-Id: Ica0103c5684b3d50ba7dc1b4c39559cb192efa81 Signed-off-by: Joel Kitching kitching@google.com --- M src/security/vboot/Makefile.inc A src/security/vboot/vboot_lib.c M src/security/vboot/vboot_logic.c M src/vendorcode/eltan/security/verified_boot/Makefile.inc 4 files changed, 36 insertions(+), 22 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/38706/8

-- To view, visit https://review.coreboot.org/c/coreboot/+/38706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ica0103c5684b3d50ba7dc1b4c39559cb192efa81 Gerrit-Change-Number: 38706 Gerrit-PatchSet: 8 Gerrit-Owner: Joel Kitching kitching@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Frans Hendriks fhendriks@eltan.com Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-CC: Wim Vervoorn wvervoorn@eltan.com Gerrit-MessageType: newpatchset

Joel Kitching (Code Review)

4:22 a.m.

Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38706 )

Change subject: security/vboot: relocate vb2ex_abort and vb2ex_printf ......................................................................

Patch Set 8:

Unfortunately had to rebase on Eltan's change -- everything is the same, except moving the change from one Makefile.inc to another.

-- To view, visit https://review.coreboot.org/c/coreboot/+/38706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ica0103c5684b3d50ba7dc1b4c39559cb192efa81 Gerrit-Change-Number: 38706 Gerrit-PatchSet: 8 Gerrit-Owner: Joel Kitching kitching@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Frans Hendriks fhendriks@eltan.com Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-CC: Wim Vervoorn wvervoorn@eltan.com Gerrit-Comment-Date: Fri, 07 Feb 2020 03:22:39 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Martin Roth (Code Review)

4:56 a.m.

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

Change subject: security/vboot: relocate vb2ex_abort and vb2ex_printf ......................................................................

Patch Set 8: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/38706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ica0103c5684b3d50ba7dc1b4c39559cb192efa81 Gerrit-Change-Number: 38706 Gerrit-PatchSet: 8 Gerrit-Owner: Joel Kitching kitching@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Frans Hendriks fhendriks@eltan.com Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-CC: Wim Vervoorn wvervoorn@eltan.com Gerrit-Comment-Date: Fri, 07 Feb 2020 03:56:18 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Martin Roth (Code Review)

4:56 a.m.

Martin Roth has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38706 )

Change subject: security/vboot: relocate vb2ex_abort and vb2ex_printf ......................................................................

security/vboot: relocate vb2ex_abort and vb2ex_printf

Enabling an assertion in vb2_member_of() results in coreboot linking vb2ex_abort() and vb2ex_printf() in ramstage.

Move these two functions from vboot_logic.c to vboot_lib.c, which is should be enabled in all stages if CONFIG_VBOOT_LIB is enabled. Note that CONFIG_VBOOT_LIB is implied by CONFIG_VBOOT.

Relevant vboot_reference commit: CL:2037263.

BUG=b:124141368, chromium:1005700 TEST=make clean && make test-abuild BRANCH=none

Change-Id: Ica0103c5684b3d50ba7dc1b4c39559cb192efa81 Signed-off-by: Joel Kitching kitching@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/38706 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Martin Roth martinroth@google.com --- M src/security/vboot/Makefile.inc A src/security/vboot/vboot_lib.c M src/security/vboot/vboot_logic.c M src/vendorcode/eltan/security/verified_boot/Makefile.inc 4 files changed, 36 insertions(+), 22 deletions(-)

Approvals: build bot (Jenkins): Verified Martin Roth: Looks good to me, approved

diff --git a/src/security/vboot/Makefile.inc b/src/security/vboot/Makefile.inc index a700e00..2fe2d92 100644 --- a/src/security/vboot/Makefile.inc +++ b/src/security/vboot/Makefile.inc @@ -16,6 +16,12 @@

ifeq ($(CONFIG_VBOOT_LIB),y)

+bootblock-y += vboot_lib.c +verstage-y += vboot_lib.c +romstage-y += vboot_lib.c +ramstage-y += vboot_lib.c +postcar-y += vboot_lib.c + vboot-fixup-includes = $(patsubst -I%,-I$(top)/%,\ $(patsubst $(src)/%.h,$(top)/$(src)/%.h,\ $(filter-out -I$(obj),$(1)))) diff --git a/src/security/vboot/vboot_lib.c b/src/security/vboot/vboot_lib.c new file mode 100644 index 0000000..b2303c0 --- /dev/null +++ b/src/security/vboot/vboot_lib.c @@ -0,0 +1,29 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* This file is part of the coreboot project. */ + +#include <console/console.h> +#include <console/vtxprintf.h> +#include <vb2_api.h> + +/* + * vboot callbacks implemented by coreboot -- necessary for making general API + * calls when CONFIG_VBOOT_LIB is enabled. For callbacks specific to verstage + * (CONFIG_VBOOT), please see vboot_logic.c. + */ + +void vb2ex_printf(const char *func, const char *fmt, ...) +{ + va_list args; + + if (func) + printk(BIOS_INFO, "VB2:%s() ", func); + + va_start(args, fmt); + vprintk(BIOS_INFO, fmt, args); + va_end(args); +} + +void vb2ex_abort(void) +{ + die("vboot has aborted execution; exit\n"); +} diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c index 1d17a17..182128c 100644 --- a/src/security/vboot/vboot_logic.c +++ b/src/security/vboot/vboot_logic.c @@ -17,8 +17,6 @@ #include <assert.h> #include <bootmode.h> #include <cbmem.h> -#include <console/console.h> -#include <console/vtxprintf.h> #include <fmap.h> #include <string.h> #include <timestamp.h> @@ -37,20 +35,6 @@

/* exports */

-void vb2ex_abort(void) -{ - die("vboot has aborted execution; exit\n"); -} - /* No-op stubs that can be overridden by SoCs with hardware crypto support. */ __weak vb2_error_t vb2ex_hwcrypto_digest_init(enum vb2_hash_algorithm hash_alg, uint32_t data_size) diff --git a/src/vendorcode/eltan/security/verified_boot/Makefile.inc b/src/vendorcode/eltan/security/verified_boot/Makefile.inc index 827535b..2acad84 100644 --- a/src/vendorcode/eltan/security/verified_boot/Makefile.inc +++ b/src/vendorcode/eltan/security/verified_boot/Makefile.inc @@ -17,7 +17,7 @@

CPPFLAGS_common += -I$(src)/security/vboot

-bootblock-y += ../../../../security/vboot/vboot_logic.c +bootblock-y += ../../../../security/vboot/vboot_lib.c bootblock-y += vboot_check.c postcar-y += vboot_check.c romstage-y += vboot_check.c

-- To view, visit https://review.coreboot.org/c/coreboot/+/38706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ica0103c5684b3d50ba7dc1b4c39559cb192efa81 Gerrit-Change-Number: 38706 Gerrit-PatchSet: 9 Gerrit-Owner: Joel Kitching kitching@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Frans Hendriks fhendriks@eltan.com Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-CC: Wim Vervoorn wvervoorn@eltan.com Gerrit-MessageType: merged

9elements QA (Code Review)

5:32 a.m.

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

Change subject: security/vboot: relocate vb2ex_abort and vb2ex_printf ......................................................................

Patch Set 9:

Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3 Emulation targets: EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/487 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/486 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/485

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

-- To view, visit https://review.coreboot.org/c/coreboot/+/38706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ica0103c5684b3d50ba7dc1b4c39559cb192efa81 Gerrit-Change-Number: 38706 Gerrit-PatchSet: 9 Gerrit-Owner: Joel Kitching kitching@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Frans Hendriks fhendriks@eltan.com Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: 9elements QA hardwaretestrobot@gmail.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-CC: Wim Vervoorn wvervoorn@eltan.com Gerrit-Comment-Date: Fri, 07 Feb 2020 04:32:41 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1751

days inactive

1754

days old

coreboot-gerrit@coreboot.org

29 comments

8 participants

tags (0)

participants (8)

9elements QA (Code Review)
Aaron Durbin (Code Review)
build bot (Jenkins) (Code Review)
Frans Hendriks (Code Review)
Joel Kitching (Code Review)
Julius Werner (Code Review)
Martin Roth (Code Review)
Patrick Georgi (Code Review)