Change in ...coreboot[master]: lib/prog_loaders.c: Add prog_locate_hook() for measured and verified ...
Frans Hendriks has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/30811
Change subject: lib/prog_loaders.c: Add prog_locate_hook() for measured and verified boot. ......................................................................
lib/prog_loaders.c: Add prog_locate_hook() for measured and verified boot.
Before images are loaded from cbfs it needs to be measured and/or verified. prog_locate_hook() is added and can be used to start measured/verified boot.
BUG=N/A TEST=Created verified binary and verify logging on Facebook FBG-1701
Change-Id: I12207fc8f2e9ca45d048cf8c8d9c057f53e5c2c7 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/include/program_loading.h M src/lib/prog_loaders.c 2 files changed, 7 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/30811/1
diff --git a/src/include/program_loading.h b/src/include/program_loading.h index 468f0b3..a382daf 100644 --- a/src/include/program_loading.h +++ b/src/include/program_loading.h @@ -3,6 +3,7 @@ * * Copyright 2015 Google Inc. * Copyright (C) 2014 Imagination Technologies + * Copyright (C) 2018 Eltan B.V. * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -136,6 +137,7 @@
/* Locate the identified program to run. Return 0 on success. < 0 on error. */ int prog_locate(struct prog *prog); +int prog_locate_hook(struct prog *prog);
/* Run the program described by prog. */ void prog_run(struct prog *prog); diff --git a/src/lib/prog_loaders.c b/src/lib/prog_loaders.c index b763417..4fa9a03 100644 --- a/src/lib/prog_loaders.c +++ b/src/lib/prog_loaders.c @@ -2,6 +2,7 @@ * This file is part of the coreboot project. * * Copyright 2015 Google Inc. + * Copyright (C) 2018 Eltan B.V. * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -39,6 +40,9 @@ { struct cbfsf file;
+ if (prog_locate_hook(prog)) + return -1; + cbfs_prepare_program_locate();
if (cbfs_boot_locate(&file, prog_name(prog), NULL)) @@ -74,6 +78,7 @@ halt(); }
+int __weak prog_locate_hook(struct prog *prog) {return 0; } void __weak stage_cache_add(int stage_id, const struct prog *stage) {} void __weak stage_cache_load_stage(int stage_id,
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() for measured and verified boot. ......................................................................
Patch Set 1: Code-Review+2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() for measured and verified boot. ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/#/c/30811/1/src/include/program_loading.h File src/include/program_loading.h:
https://review.coreboot.org/#/c/30811/1/src/include/program_loading.h@140 PS1, Line 140: int prog_locate_hook(struct prog *prog); I think you should just call this vboot_crtm_measure_prog() or something, put it in vboot_crtm.h, and implement it as an empty inline function there if VBOOT_MEASURED_BOOT isn't set (similar to how things like timestamp_add_now() work). Calling this prog_locate_hook() implies that other features could make use of this hook as well, but they can't really, because then they couldn't be used at the same time as measured boot. These sort of hook-by-function-name hooks only really work if they're meant to be implemented by a range of code components where only one of them can ever be used at the same time (like mainboards or SoCs). They don't work well for optional, platform-independent features, like timestamps or this.
https://review.coreboot.org/#/c/30811/1/src/lib/prog_loaders.c File src/lib/prog_loaders.c:
https://review.coreboot.org/#/c/30811/1/src/lib/prog_loaders.c@43 PS1, Line 43: if (prog_locate_hook(prog)) I don't think this works? prog->rdev is still uninitialized here. You'll have to put it at the end of this function, after cbfs_file_data(), to do what you want.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() for measured and verified boot. ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/30811/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/30811/1//COMMIT_MSG@9 PS1, Line 9: Before It seems odd to do it before loading, is there any reason to? Couldn't we simply avoid the TOCTOU disparity by hooking in later?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() for measured and verified boot. ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/30811/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/30811/1//COMMIT_MSG@9 PS1, Line 9: Before
It seems odd to do it before loading, is there any reason to? […]
Since x86 images are executed in place I'm pretty sure there's no way to avoid TOCTOU there (unless you know some details about how the SPI controller caches memory-mapped accesses that can be used to guarantee that).
For other architectures, this would be possible if you measure the stage after it was loaded into memory, but I think Philipp was trying to avoid that to avoid the disparity of measuring pre-verification stages as they are stored in CBFS and post-verification stages as they appear in memory (see my comment in https://review.coreboot.org/c/coreboot/+/29547/36/src/security/vboot/vboot_c...).
Also, any attacker that has the ability of manipulating the data returned from the SPI ROM on the fly most likely also has the ability to take complete control of the boot process and put any hashes they want into the TPM, so whether protecting against TOCTOU really has a value here may be doubtful. (Then again, that argument could be brought against measuring firmware in the TPM as a whole too, I guess, which is why I never really understood the point of the whole concept. ;) )
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() for measured and verified boot. ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/30811/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/30811/1//COMMIT_MSG@7 PS1, Line 7: lib/prog_loaders.c: Add prog_locate_hook() for measured and verified boot. Please remove the dot/period at the end.
Hello Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30811
to look at the new patch set (#2).
Change subject: lib/prog_loaders.c: Add prog_locate_hook() for measured and verified boot ......................................................................
lib/prog_loaders.c: Add prog_locate_hook() for measured and verified boot
Before images are loaded from cbfs it needs to be measured and/or verified. prog_locate_hook() is added and can be used to start measured/verified boot.
BUG=N/A TEST=Created verified binary and verify logging on Facebook FBG-1701
Change-Id: I12207fc8f2e9ca45d048cf8c8d9c057f53e5c2c7 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/include/program_loading.h M src/lib/prog_loaders.c 2 files changed, 7 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/30811/2
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() for measured and verified boot ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/#/c/30811/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/30811/1//COMMIT_MSG@7 PS1, Line 7: lib/prog_loaders.c: Add prog_locate_hook() for measured and verified boot.
Please remove the dot/period at the end.
Done
https://review.coreboot.org/#/c/30811/1//COMMIT_MSG@9 PS1, Line 9: Before
Since x86 images are executed in place I'm pretty sure there's no way to avoid TOCTOU there (unless […]
Implementation verifies image before loading. Don't load images which does not pass verification.
https://review.coreboot.org/#/c/30811/1/src/lib/prog_loaders.c File src/lib/prog_loaders.c:
https://review.coreboot.org/#/c/30811/1/src/lib/prog_loaders.c@43 PS1, Line 43: if (prog_locate_hook(prog))
I don't think this works? prog->rdev is still uninitialized here. […]
This code is working find on our project.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() for measured and verified boot ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/30811/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/30811/1//COMMIT_MSG@9 PS1, Line 9: Before
Implementation verifies image before loading. Don't load images which does not pass verification.
The whole measurement architecture should be documented and it must be explained why it is believed there's no TOCTOU.
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() for measured and verified boot ......................................................................
Patch Set 2: -Code-Review
Hello Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30811
to look at the new patch set (#3).
Change subject: lib/prog_loaders.c: Add prog_locate_hook() for measured and verified boot ......................................................................
lib/prog_loaders.c: Add prog_locate_hook() for measured and verified boot
Before images are loaded from cbfs they need to be measured and/or verified. prog_locate_hook() is added and can be used to start measured/verified boot.
BUG=N/A TEST=Created verified binary and verify logging on Facebook FBG-1701
Change-Id: I12207fc8f2e9ca45d048cf8c8d9c057f53e5c2c7 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/include/program_loading.h M src/lib/prog_loaders.c 2 files changed, 7 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/30811/3
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() for measured and verified boot ......................................................................
Patch Set 3: Code-Review+2
Frans Hendriks has removed a vote on this change.
Change subject: lib/prog_loaders.c: Add prog_locate_hook() for measured and verified boot ......................................................................
Removed Code-Review+2 by Frans Hendriks fhendriks@eltan.com
Hello Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30811
to look at the new patch set (#4).
Change subject: lib/prog_loaders.c: Add prog_locate_hook() ......................................................................
lib/prog_loaders.c: Add prog_locate_hook()
There is not posibility to prevent loading images from cbfs at this stage For security features prog_locate_hook() is added. This hook can be used to prevent loading the image.
BUG=N/A TEST=Created verified binary and verify logging on Facebook FBG-1701
Change-Id: I12207fc8f2e9ca45d048cf8c8d9c057f53e5c2c7 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/include/program_loading.h M src/lib/prog_loaders.c 2 files changed, 7 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/30811/4
Hello Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30811
to look at the new patch set (#5).
Change subject: lib/prog_loaders.c: Add prog_locate_hook() ......................................................................
lib/prog_loaders.c: Add prog_locate_hook()
There is not posibility to prevent loading images from cbfs at this stage For security features prog_locate_hook() is added. This hook can be used to prevent loading the image.
BUG=N/A TEST=Created verified binary and verify logging on Facebook FBG-1701
Change-Id: I12207fc8f2e9ca45d048cf8c8d9c057f53e5c2c7 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/include/program_loading.h M src/lib/prog_loaders.c 2 files changed, 8 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/30811/5
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/30811/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/30811/1//COMMIT_MSG@7 PS1, Line 7: lib/prog_loaders.c: Add prog_locate_hook() for measured and verified boot.
Done
Done
https://review.coreboot.org/c/coreboot/+/30811/1//COMMIT_MSG@9 PS1, Line 9: Before
The whole measurement architecture should be documented and it must be explained why it is believed […]
Done
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/30811/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/30811/1//COMMIT_MSG@7 PS1, Line 7: lib/prog_loaders.c: Add prog_locate_hook() for measured and verified boot.
Done
Done
https://review.coreboot.org/c/coreboot/+/30811/1//COMMIT_MSG@9 PS1, Line 9: Before
Done
Done
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/30811/1/src/lib/prog_loaders.c File src/lib/prog_loaders.c:
https://review.coreboot.org/c/coreboot/+/30811/1/src/lib/prog_loaders.c@43 PS1, Line 43: if (prog_locate_hook(prog))
This code is working find on our project.
Done
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/30811/1/src/include/program_loading... File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/30811/1/src/include/program_loading... PS1, Line 140: int prog_locate_hook(struct prog *prog);
I think you should just call this vboot_crtm_measure_prog() or something, put it in vboot_crtm. […]
Done
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() ......................................................................
Patch Set 7:
Please rebase, probably the jenkins was broken
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() ......................................................................
Patch Set 8:
Patch Set 7:
Please rebase, probably the jenkins was broken
Has been rebased
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() ......................................................................
Patch Set 8:
(2 comments)
https://review.coreboot.org/c/coreboot/+/30811/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/30811/8//COMMIT_MSG@9 PS8, Line 9: There is not posibility It’s not possible …
https://review.coreboot.org/c/coreboot/+/30811/8//COMMIT_MSG@9 PS8, Line 9: not no
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/30811/8/src/include/program_loading... File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/30811/8/src/include/program_loading... PS8, Line 142: int prog_locate_hook(struct prog *prog); Please provide a comment explaining the function just like the other ones in this file.
Hello Wim Vervoorn, Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30811
to look at the new patch set (#9).
Change subject: lib/prog_loaders.c: Add prog_locate_hook() ......................................................................
lib/prog_loaders.c: Add prog_locate_hook()
There is no posibility to prevent loading images from cbfs at this stage For security features prog_locate_hook() is added. This hook can be used to prevent loading the image.
BUG=N/A TEST=Created verified binary and verify logging on Facebook FBG-1701
Change-Id: I12207fc8f2e9ca45d048cf8c8d9c057f53e5c2c7 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/include/program_loading.h M src/lib/prog_loaders.c 2 files changed, 8 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/30811/9
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/30811/9/src/include/program_loading... File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/30811/9/src/include/program_loading... PS9, Line 142: int prog_locate_hook(struct prog *prog); Same comments as previous patch.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/c/coreboot/+/30811/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/30811/8//COMMIT_MSG@9 PS8, Line 9: There is not posibility
It’s not possible …
Done
https://review.coreboot.org/c/coreboot/+/30811/8//COMMIT_MSG@9 PS8, Line 9: not
no
Done
Hello Wim Vervoorn, Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30811
to look at the new patch set (#10).
Change subject: lib/prog_loaders.c: Add prog_locate_hook() ......................................................................
lib/prog_loaders.c: Add prog_locate_hook()
There is no posibility to prevent loading images from cbfs at this stage For security features prog_locate_hook() is added. This hook can be used to prevent loading the image.
BUG=N/A TEST=Created verified binary and verify logging on Facebook FBG-1701
Change-Id: I12207fc8f2e9ca45d048cf8c8d9c057f53e5c2c7 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/include/program_loading.h M src/lib/prog_loaders.c 2 files changed, 9 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/30811/10
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() ......................................................................
Patch Set 10: Code-Review+1
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() ......................................................................
Patch Set 10:
(2 comments)
https://review.coreboot.org/c/coreboot/+/30811/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/30811/8//COMMIT_MSG@9 PS8, Line 9: There is not posibility
Done
Done
https://review.coreboot.org/c/coreboot/+/30811/9/src/include/program_loading... File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/30811/9/src/include/program_loading... PS9, Line 142: int prog_locate_hook(struct prog *prog);
Same comments as previous patch.
Done
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/30811/8/src/include/program_loading... File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/30811/8/src/include/program_loading... PS8, Line 142: int prog_locate_hook(struct prog *prog);
Please provide a comment explaining the function just like the other ones in this file.
Done
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/30811/10/src/include/program_loadin... File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/30811/10/src/include/program_loadin... PS10, Line 142: if identified program might run. That's not true in absolute terms. This is a generic hook, and it could be used for such purposes. Please update the comment to describe the generic functionality and what it could be used for. e.g. when the function is called and what is valid. Current implementation is very early in prog_locate() w/o having many fields filled in yet as cbfs location hasn't been done.
Hello Felix Held, Wim Vervoorn, Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30811
to look at the new patch set (#11).
Change subject: lib/prog_loaders.c: Add prog_locate_hook() ......................................................................
lib/prog_loaders.c: Add prog_locate_hook()
There is no posibility to prevent loading images from cbfs at this stage For security features prog_locate_hook() is added. This hook can be used to prevent loading the image.
BUG=N/A TEST=Created verified binary and verify logging on Facebook FBG-1701
Change-Id: I12207fc8f2e9ca45d048cf8c8d9c057f53e5c2c7 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/include/program_loading.h M src/lib/prog_loaders.c 2 files changed, 14 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/30811/11
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/30811/10/src/include/program_loadin... File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/30811/10/src/include/program_loadin... PS10, Line 142: if identified program might run.
That's not true in absolute terms. This is a generic hook, and it could be used for such purposes. […]
Done
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() ......................................................................
Patch Set 11: Code-Review+1
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() ......................................................................
Patch Set 11:
(2 comments)
https://review.coreboot.org/c/coreboot/+/30811/11/src/include/program_loadin... File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/30811/11/src/include/program_loadin... PS11, Line 144: validate Validate what?
https://review.coreboot.org/c/coreboot/+/30811/11/src/include/program_loadin... PS11, Line 146: * return -1 Please use CB_SUCCESS and friends.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/30811/11/src/include/program_loadin... File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/30811/11/src/include/program_loadin... PS11, Line 146: * return -1
Please use CB_SUCCESS and friends.
In prog_locate() value 0 or -1 is returned. No friends of CB_SUCCESS. :)
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/30811/11/src/include/program_loadin... File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/30811/11/src/include/program_loadin... PS11, Line 144: validate
Validate what?
This is up to the person implementing the hook. He can just check the name and take a decision based on that or actually locate the file and check an hash. For this hook this doesn't make a difference.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/30811/11/src/include/program_loadin... File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/30811/11/src/include/program_loadin... PS11, Line 146: * return -1
In prog_locate() value 0 or -1 is returned. […]
Done
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/30811/11/src/include/program_loadin... File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/30811/11/src/include/program_loadin... PS11, Line 144: validate
This is up to the person implementing the hook. […]
Please explicitly note in this comment that this call is prior to any cbfs traversal.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/30811/11/src/include/program_loadin... File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/30811/11/src/include/program_loadin... PS11, Line 144: validate
Please explicitly note in this comment that this call is prior to any cbfs traversal.
imo line 'Hook .. proceed' already answers your comment.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/30811/11/src/include/program_loadin... File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/30811/11/src/include/program_loadin... PS11, Line 144: validate
imo line 'Hook .. proceed' already answers your comment.
Proceed from where to what? It's not clear; the statement ambiguous. I don't think it's too much to ask to improve clarity in comments for functions. FWIW, the hook doesn't have to verify anything either. It can do anything it wants from a policy perspective. How about the following?
The prog_locate_hook() is called prior to CBFS traversal. The hook can be used to implement policy that allows or prohibits further progress through prog_locate(). The type and name field within struct prog are the only valid fields. A 0 return value allows further progress while a non-zero return value prohibits further progress.
Hello Wim Vervoorn, Felix Held, Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30811
to look at the new patch set (#12).
Change subject: lib/prog_loaders.c: Add prog_locate_hook() ......................................................................
lib/prog_loaders.c: Add prog_locate_hook()
There is no posibility to prevent loading images from cbfs at this stage For security features prog_locate_hook() is added. This hook can be used to prevent loading the image.
BUG=N/A TEST=Created verified binary and verify logging on Facebook FBG-1701
Change-Id: I12207fc8f2e9ca45d048cf8c8d9c057f53e5c2c7 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/include/program_loading.h M src/lib/prog_loaders.c 2 files changed, 13 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/30811/12
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/30811/11/src/include/program_loadin... File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/30811/11/src/include/program_loadin... PS11, Line 144: validate
Proceed from where to what? It's not clear; the statement ambiguous. […]
Agree with proposal.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/30811/11/src/include/program_loadin... File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/30811/11/src/include/program_loadin... PS11, Line 144: validate
Agree with proposal.
Done
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() ......................................................................
Patch Set 12: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/30811 )
Change subject: lib/prog_loaders.c: Add prog_locate_hook() ......................................................................
lib/prog_loaders.c: Add prog_locate_hook()
There is no posibility to prevent loading images from cbfs at this stage For security features prog_locate_hook() is added. This hook can be used to prevent loading the image.
BUG=N/A TEST=Created verified binary and verify logging on Facebook FBG-1701
Change-Id: I12207fc8f2e9ca45d048cf8c8d9c057f53e5c2c7 Signed-off-by: Frans Hendriks fhendriks@eltan.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/30811 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org --- M src/include/program_loading.h M src/lib/prog_loaders.c 2 files changed, 13 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved
diff --git a/src/include/program_loading.h b/src/include/program_loading.h index 6dec192..601847d 100644 --- a/src/include/program_loading.h +++ b/src/include/program_loading.h @@ -3,6 +3,7 @@ * * Copyright 2015 Google Inc. * Copyright (C) 2014 Imagination Technologies + * Copyright (C) 2018 Eltan B.V. * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -138,6 +139,12 @@
/* Locate the identified program to run. Return 0 on success. < 0 on error. */ int prog_locate(struct prog *prog); +/* The prog_locate_hook() is called prior to CBFS traversal. The hook can be + * used to implement policy that allows or prohibits further progress through + * prog_locate(). The type and name field within struct prog are the only valid + * fields. A 0 return value allows further progress while a non-zero return + * value prohibits further progress */ +int prog_locate_hook(struct prog *prog);
/* Run the program described by prog. */ void prog_run(struct prog *prog); diff --git a/src/lib/prog_loaders.c b/src/lib/prog_loaders.c index 2ef6bdf..5048c99 100644 --- a/src/lib/prog_loaders.c +++ b/src/lib/prog_loaders.c @@ -2,6 +2,7 @@ * This file is part of the coreboot project. * * Copyright 2015 Google Inc. + * Copyright (C) 2018-2019 Eltan B.V. * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -38,6 +39,9 @@ { struct cbfsf file;
+ if (prog_locate_hook(prog)) + return -1; + cbfs_prepare_program_locate();
if (cbfs_boot_locate(&file, prog_name(prog), NULL)) @@ -74,6 +78,8 @@ halt(); }
+int __weak prog_locate_hook(struct prog *prog) { return 0; } + static void ramstage_cache_invalid(void) { printk(BIOS_ERR, "ramstage cache invalid.\n");
-
Aaron Durbin (Code Review) -
Frans Hendriks (Code Review)
-
Julius Werner (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)
-
Philipp Deppenwiese (Code Review)
-
Wim Vervoorn (Code Review)