Wim Vervoorn has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36544 )
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
security/vboot: Add rw_region_only support to vboot
In some case where the flash space is limited or when a large payload such as LinuxBoot is used it is required to make sure some components are only added to the RW_REGION.
This patch adds this possibility in the same way as the RO_ONLY_SUPPORT.
BUG=N/A TEST=build
Change-Id: Ie0df9b5dfc6df4f24efc5582a1aec9ecfb48c44d Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/security/vboot/Kconfig M src/security/vboot/Makefile.inc 2 files changed, 14 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/36544/1
diff --git a/src/security/vboot/Kconfig b/src/security/vboot/Kconfig index d6d74ca..7059fde 100644 --- a/src/security/vboot/Kconfig +++ b/src/security/vboot/Kconfig @@ -224,6 +224,13 @@ Add a space delimited list of filenames that should only be in the RO section.
+config RW_REGION_ONLY + string "Files that should ONLY be copied to RW" + default "" + help + Add a space delimited list of filenames that should only be in the + RW section. + menu "GBB configuration"
config GBB_HWID diff --git a/src/security/vboot/Makefile.inc b/src/security/vboot/Makefile.inc index 31c0f5d..c8f5899 100644 --- a/src/security/vboot/Makefile.inc +++ b/src/security/vboot/Makefile.inc @@ -170,10 +170,12 @@ # Check for RW_A partition ifeq ($(CONFIG_VBOOT_SLOTS_RW_A),y) VBOOT_PARTITIONS += FW_MAIN_A +RW_PARTITIONS := FW_MAIN_A endif # Check for RW_B partition ifeq ($(CONFIG_VBOOT_SLOTS_RW_AB),y) VBOOT_PARTITIONS += FW_MAIN_B +RW_PARTITIONS += FW_MAIN_B endif
# Define a list of files that need to be in RO only. @@ -193,7 +195,11 @@ cmos_layout.bin \ cmos.default \ $(call strip_quotes,$(CONFIG_RO_REGION_ONLY)) \ - ,$(1)),COREBOOT,$(VBOOT_PARTITIONS)))) + ,$(1)),COREBOOT,\ + $(if $(filter \ + $(call strip_quotes,$(CONFIG_RW_REGION_ONLY)) \ + ,$(1)), $(RW_PARTITIONS), $(VBOOT_PARTITIONS) ) \ + )))
CONFIG_GBB_HWID := $(call strip_quotes,$(CONFIG_GBB_HWID)) CONFIG_GBB_BMPFV_FILE := $(call strip_quotes,$(CONFIG_GBB_BMPFV_FILE))
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36544 )
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
Patch Set 1: Code-Review+2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36544 )
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36544/1/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/36544/1/src/security/vboot/Kconfig@... PS1, Line 232: section section(s)
https://review.coreboot.org/c/coreboot/+/36544/1/src/security/vboot/Makefile... File src/security/vboot/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36544/1/src/security/vboot/Makefile... PS1, Line 181: in RO only I think this comment could use some work to more accurately describe the behavior?
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36544 )
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36544/1/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/36544/1/src/security/vboot/Kconfig@... PS1, Line 232: section
section(s)
Done
Hello Aaron Durbin, Frans Hendriks, Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36544
to look at the new patch set (#2).
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
security/vboot: Add rw_region_only support to vboot
In some case where the flash space is limited or when a large payload such as LinuxBoot is used it is required to make sure some components are only added to the RW_REGION.
This patch adds this possibility in the same way as the RO_ONLY_SUPPORT.
BUG=N/A TEST=build
Change-Id: Ie0df9b5dfc6df4f24efc5582a1aec9ecfb48c44d Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/security/vboot/Kconfig M src/security/vboot/Makefile.inc 2 files changed, 17 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/36544/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36544 )
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36544/2/src/security/vboot/Makefile... File src/security/vboot/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36544/2/src/security/vboot/Makefile... PS2, Line 183: # specified in the CONFIG_RW_REGION_ONLY are only placed in the RW regions. trailing whitespace
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36544 )
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
Patch Set 2:
(1 comment)
Added a better description of what happens in the regions_for_file.
https://review.coreboot.org/c/coreboot/+/36544/1/src/security/vboot/Makefile... File src/security/vboot/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36544/1/src/security/vboot/Makefile... PS1, Line 181: in RO only
I think this comment could use some work to more accurately describe the behavior?
Done
Hello Aaron Durbin, Frans Hendriks, Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36544
to look at the new patch set (#3).
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
security/vboot: Add rw_region_only support to vboot
In some case where the flash space is limited or when a large payload such as LinuxBoot is used it is required to make sure some components are only added to the RW_REGION.
This patch adds this possibility in the same way as the RO_ONLY_SUPPORT.
BUG=N/A TEST=build
Change-Id: Ie0df9b5dfc6df4f24efc5582a1aec9ecfb48c44d Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/security/vboot/Kconfig M src/security/vboot/Makefile.inc 2 files changed, 17 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/36544/3
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36544 )
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
Patch Set 3: Code-Review+1
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36544 )
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36544/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36544/3//COMMIT_MSG@9 PS3, Line 9: In some case where the flash space is limited or when a large payload : such as LinuxBoot is used it is required to make sure some components : are only added to the RW_REGION. : : This patch adds this possibility in the same way as the RO_ONLY_SUPPORT. How does this work? The RO region ends up without a payload?
https://review.coreboot.org/c/coreboot/+/36544/3/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/36544/3/src/security/vboot/Kconfig@... PS3, Line 227: RW_REGION_ONLY This should be properly guarded. It needs to depend on on having at least one region.
Hello Aaron Durbin, Frans Hendriks, Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36544
to look at the new patch set (#4).
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
security/vboot: Add rw_region_only support to vboot
In some case where the flash space is limited or when a large payload such as LinuxBoot is used it is required to make sure some components are only added to the RW_REGION.
This patch adds this possibility in the same way as the RO_ONLY_SUPPORT.
BUG=N/A TEST=build
Change-Id: Ie0df9b5dfc6df4f24efc5582a1aec9ecfb48c44d Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/security/vboot/Kconfig M src/security/vboot/Makefile.inc 2 files changed, 18 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/36544/4
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36544 )
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36544/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36544/3//COMMIT_MSG@9 PS3, Line 9: In some case where the flash space is limited or when a large payload : such as LinuxBoot is used it is required to make sure some components : are only added to the RW_REGION. : : This patch adds this possibility in the same way as the RO_ONLY_SUPPORT.
How does this work? The RO region ends up without a payload?
You are right. Typically the RO and RW payloads will be different. The idea is that the RO payload is added manually. During development it makes sense to add a smaller payload and remove the payload from this option so it will be in both RO and RW regions.
At this point in time coreboot doesn't provide the possibility to specify 2 payloads, add one to the RO and one to the RW regions and I don't have the time to implement that. I think this is a pragmatic replacement for that.
https://review.coreboot.org/c/coreboot/+/36544/3/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/36544/3/src/security/vboot/Kconfig@... PS3, Line 227: RW_REGION_ONLY
This should be properly guarded. It needs to depend on on having at least one region.
You are right. Added the dependancy.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36544 )
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36544/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36544/3//COMMIT_MSG@9 PS3, Line 9: In some case where the flash space is limited or when a large payload : such as LinuxBoot is used it is required to make sure some components : are only added to the RW_REGION. : : This patch adds this possibility in the same way as the RO_ONLY_SUPPORT.
You are right. Typically the RO and RW payloads will be different. The idea is that the RO payload is added manually. During development it makes sense to add a smaller payload and remove the payload from this option so it will be in both RO and RW regions.
At this point in time coreboot doesn't provide the possibility to specify 2 payloads, add one to the RO and one to the RW regions and I don't have the time to implement that. I think this is a pragmatic replacement for that.
If you end up adding things manually to fmap regions, why bothering at all with this? You can build regularly with your smaller payload and replace the larger RW one? That just requires recreating the VBLOCK.
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36544 )
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
Patch Set 4:
Patch Set 4:
(1 comment)
The idea behind this is that the update image needs to be created more often than the "factory image" containing the correct RO partition. So this should be as easy as possible so it would even be acceptable to have an update-image without the payload (as the RO part won't be used anyway). During development of the initial image we can simply start out by adding the small payload to both the RO and RW regions. So the number of manual actions required will be limited.
For my understanding why do you think it is easier to add the larger payload later? Now I can build the large payload with the coreboot tree and generate the image with the correct payload in one go. How would that work in your case?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36544 )
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
Patch Set 4:
Patch Set 4:
Patch Set 4:
(1 comment)
The idea behind this is that the update image needs to be created more often than the "factory image" containing the correct RO partition. So this should be as easy as possible so it would even be acceptable to have an update-image without the payload (as the RO part won't be used anyway). During development of the initial image we can simply start out by adding the small payload to both the RO and RW regions. So the number of manual actions required will be limited.
I think with chromeos new 'factory' images are always created regardless if the RO partition is already locked.
For my understanding why do you think it is easier to add the larger payload later? Now I can build the large payload with the coreboot tree and generate the image with the correct payload in one go. How would that work in your case?
I just think that handling payloads outside of the coreboot build system makes more sense when shipping images, especially if you want different things in RO vs RW.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36544 )
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
Patch Set 4: Code-Review+2
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36544 )
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
Patch Set 4:
(1 comment)
Patch Set 4:
Patch Set 4:
Patch Set 4:
(1 comment)
The idea behind this is that the update image needs to be created more often than the "factory image" containing the correct RO partition. So this should be as easy as possible so it would even be acceptable to have an update-image without the payload (as the RO part won't be used anyway). During development of the initial image we can simply start out by adding the small payload to both the RO and RW regions. So the number of manual actions required will be limited.
I think with chromeos new 'factory' images are always created regardless if the RO partition is already locked.
For my understanding why do you think it is easier to add the larger payload later? Now I can build the large payload with the coreboot tree and generate the image with the correct payload in one go. How would that work in your case?
I just think that handling payloads outside of the coreboot build system makes more sense when shipping images, especially if you want different things in RO vs RW.
Please note that this config is not intended for ChromeOS systems, the intention is to allow some deviation from that behavior for embedded systems. In our case, the system will be delivered with the default image loaded and the RO region of the flash device locked so this can't be altered. So after that, the content of the RO region is irrelevant as this will never be flashed.
What we want to achieve for the customer is an easy way to generate update images. After setup the customer will be able to run make menuconfig and make (or just generate a new payload is this is a bzImage or elf) and generate a new coreboot image.
For some very specific customers we also have the requirement that the system should not boot at all if the verified boot check fails. In this case it doesn't make sense to waste space on a payload inside the RO region.
I know these are non standard situations but they still happen.
https://review.coreboot.org/c/coreboot/+/36544/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36544/3//COMMIT_MSG@9 PS3, Line 9: In some case where the flash space is limited or when a large payload : such as LinuxBoot is used it is required to make sure some components : are only added to the RW_REGION. : : This patch adds this possibility in the same way as the RO_ONLY_SUPPORT.
You are right. Typically the RO and RW payloads will be different. […]
Done
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36544 )
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
Patch Set 4:
Patch Set 4:
(1 comment)
Patch Set 4:
Patch Set 4:
Patch Set 4:
(1 comment)
The idea behind this is that the update image needs to be created more often than the "factory image" containing the correct RO partition. So this should be as easy as possible so it would even be acceptable to have an update-image without the payload (as the RO part won't be used anyway). During development of the initial image we can simply start out by adding the small payload to both the RO and RW regions. So the number of manual actions required will be limited.
I think with chromeos new 'factory' images are always created regardless if the RO partition is already locked.
For my understanding why do you think it is easier to add the larger payload later? Now I can build the large payload with the coreboot tree and generate the image with the correct payload in one go. How would that work in your case?
I just think that handling payloads outside of the coreboot build system makes more sense when shipping images, especially if you want different things in RO vs RW.
Please note that this config is not intended for ChromeOS systems, the intention is to allow some deviation from that behavior for embedded systems. In our case, the system will be delivered with the default image loaded and the RO region of the flash device locked so this can't be altered. So after that, the content of the RO region is irrelevant as this will never be flashed.
Then you are probably better off having a way to generate just the RW regions instead of some weird hacks to limit what goes into the RO region?
What we want to achieve for the customer is an easy way to generate update images. After setup the customer will be able to run make menuconfig and make (or just generate a new payload is this is a bzImage or elf) and generate a new coreboot image.
This changes how a whole image is build, while you only care about the RW part. So why not just generate the RW part?
For some very specific customers we also have the requirement that the system should not boot at all if the verified boot check fails. In this case it doesn't make sense to waste space on a payload inside the RO region.
I know these are non standard situations but they still happen.
The best way seems to just build without payload and add it manually to the RW region or build a full image without payloads and update only the RW region later on to have a payload?
With image composing being done in makefile, maintainability quickly becomes a problem when trying to support different use cases. see CB:32687.
Hello Aaron Durbin, Frans Hendriks, Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36544
to look at the new patch set (#7).
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
security/vboot: Add rw_region_only support to vboot
In some case where the flash space is limited or when a large payload such as LinuxBoot is used it is required to make sure some components are only added to the RW_REGION.
This patch adds this possibility in the same way as the RO_ONLY_SUPPORT.
BUG=N/A TEST=build
Change-Id: Ie0df9b5dfc6df4f24efc5582a1aec9ecfb48c44d Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/security/vboot/Kconfig M src/security/vboot/Makefile.inc 2 files changed, 17 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/36544/7
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36544 )
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
Patch Set 7: Code-Review+2
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36544 )
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
Patch Set 7: Code-Review-1
(1 comment)
-1 Until my points are addressed about why not building just a RW region if that is what you sort of seem to want.
https://review.coreboot.org/c/coreboot/+/36544/7/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/36544/7/src/security/vboot/Kconfig@... PS7, Line 224: "Files that should ONLY be copied to RW" I don't like this. You can insert bad stuff here like bootblock. It should not be visible like this IMO.
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36544 )
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
Patch Set 7:
(1 comment)
I explained a bit more how this feature works. I hope this addresses your concern about the possibility to do bad things (like removing the bootblock, romstage or verstage) using this option.
This simply isn't possible. For what remains you can do equally bad things using the RO region only support.
https://review.coreboot.org/c/coreboot/+/36544/7/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/36544/7/src/security/vboot/Kconfig@... PS7, Line 224: "Files that should ONLY be copied to RW"
I don't like this. You can insert bad stuff here like bootblock. […]
I don't have any problems to guard this feature with an enabling config to be added in the board Kconfig. I agree with you that you should know what you are doing when you use it.
I don't think this is as bad as you think it is. If you add the bootblock to the list nothing will happen. This option only works for files that would normally end up in all regions so the critical ones won't be affected anyhow.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36544 )
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
Patch Set 7:
(1 comment)
Patch Set 7:
(1 comment)
I explained a bit more how this feature works. I hope this addresses your concern about the possibility to do bad things (like removing the bootblock, romstage or verstage) using this option.
This simply isn't possible. For what remains you can do equally bad things using the RO region only support.
No RO only leaves you with a broken RW, which isn't as bad in general.
Can you address my idea of generating only the RW region? That along with flashrom being able to flash fmap regions should cover your use case better. It's also a quite useful feature in general.
https://review.coreboot.org/c/coreboot/+/36544/7/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/36544/7/src/security/vboot/Kconfig@... PS7, Line 224: "Files that should ONLY be copied to RW"
I don't have any problems to guard this feature with an enabling config to be added in the board Kconfig. I agree with you that you should know what you are doing when you use it.
I don't think this is as bad as you think it is. If you add the bootblock to the list nothing will happen. This option only works for files that would normally end up in all regions so the critical ones won't be affected anyhow.
Do you really need this to be visible in Kconfig? ok, bootblock is not the best example indeed but bootblock potentially depends on other things that need to be there: cmos_layout.bin, microcode, FSP if you are unlucky to work on such platforms.
One should generally avoid having visible Kconfig options that can break things.
Hello Aaron Durbin, Arthur Heymans, Frans Hendriks, Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36544
to look at the new patch set (#8).
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
security/vboot: Add rw_region_only support to vboot
In some case where the flash space is limited or when a large payload such as LinuxBoot is used it is required to make sure some components are only added to the RW_REGION.
This patch adds this possibility in the same way as the RO_ONLY_SUPPORT.
BUG=N/A TEST=build
Change-Id: Ie0df9b5dfc6df4f24efc5582a1aec9ecfb48c44d Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/security/vboot/Kconfig M src/security/vboot/Makefile.inc 2 files changed, 17 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/36544/8
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36544 )
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36544/8/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/36544/8/src/security/vboot/Kconfig@... PS8, Line 224: string trailing whitespace
Hello Aaron Durbin, Arthur Heymans, Frans Hendriks, Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36544
to look at the new patch set (#9).
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
security/vboot: Add rw_region_only support to vboot
In some case where the flash space is limited or when a large payload such as LinuxBoot is used it is required to make sure some components are only added to the RW_REGION.
This patch adds this possibility in the same way as the RO_ONLY_SUPPORT.
BUG=N/A TEST=build
Change-Id: Ie0df9b5dfc6df4f24efc5582a1aec9ecfb48c44d Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/security/vboot/Kconfig M src/security/vboot/Makefile.inc 2 files changed, 17 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/36544/9
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36544 )
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36544/7/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/36544/7/src/security/vboot/Kconfig@... PS7, Line 224: "Files that should ONLY be copied to RW"
I don't have any problems to guard this feature with an enabling config to be added in the board K […]
I misunderstood your concern. An invisible one is fine as well. This is something that is determined and put into a fixed config. I changed it in the commit.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36544 )
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
Patch Set 9: Code-Review+2
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36544 )
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
Patch Set 9:
Patch Set 7:
(1 comment)
Patch Set 7:
(1 comment)
I explained a bit more how this feature works. I hope this addresses your concern about the possibility to do bad things (like removing the bootblock, romstage or verstage) using this option.
This simply isn't possible. For what remains you can do equally bad things using the RO region only support.
No RO only leaves you with a broken RW, which isn't as bad in general.
Can you address my idea of generating only the RW region? That along with flashrom being able to flash fmap regions should cover your use case better. It's also a quite useful feature in general.
I overlooked responding to your idea to generate on the RW region. The issue is that I haven't found anything in the documentation to do this in an easy way. The coreboot build systen doesn't really let me do this. I know it's possible by using the tools and creating components seperately but not in an easy to use "make" type of way. If I am missing something please let me know.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36544 )
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
Patch Set 9:
(1 comment)
Patch Set 9:
Patch Set 7:
(1 comment)
Patch Set 7:
(1 comment)
I explained a bit more how this feature works. I hope this addresses your concern about the possibility to do bad things (like removing the bootblock, romstage or verstage) using this option.
This simply isn't possible. For what remains you can do equally bad things using the RO region only support.
No RO only leaves you with a broken RW, which isn't as bad in general.
Can you address my idea of generating only the RW region? That along with flashrom being able to flash fmap regions should cover your use case better. It's also a quite useful feature in general.
I overlooked responding to your idea to generate on the RW region. The issue is that I haven't found anything in the documentation to do this in an easy way. The coreboot build systen doesn't really let me do this. I know it's possible by using the tools and creating components seperately but not in an easy to use "make" type of way. If I am missing something please let me know.
It's not perfect but if COREBOOT is not in VBOOT_PARTITIONS, you get a pretty empty stub. Might be possible to improve that further to not populate it at all?
https://review.coreboot.org/c/coreboot/+/36544/9//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36544/9//COMMIT_MSG@10 PS9, Line 10: it is required to make sure some components : are only added to the RW_REGION. 'required to make sure' begs more questions. Please try to sketch a use case where this might be useful here.
Hello Aaron Durbin, Arthur Heymans, Frans Hendriks, Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36544
to look at the new patch set (#10).
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
security/vboot: Add rw_region_only support to vboot
In some case where the flash space is limited or when a large payload such as LinuxBoot is used, the RO region may not be large enough to contain all components that would normally be added.
This patch adds the possibility to add specific components to the RW regions only in the same way as the RO_ONLY_SUPPORT does for the RO region.
Please note: this applies only to the items that would normally be added to all regions. If the payload is directed to the RW region only, a recovery payload needs to be added to the RO region manually.
BUG=N/A TEST=build
Change-Id: Ie0df9b5dfc6df4f24efc5582a1aec9ecfb48c44d Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/security/vboot/Kconfig M src/security/vboot/Makefile.inc 2 files changed, 17 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/36544/10
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36544 )
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
Patch Set 10: Code-Review+1
(1 comment)
If RW and RO need different objects inside them I think it would make sense to generate them separately or even as a separate files, but that's part of a larger redesign. I guess this is good enough as a stop gap measure for now to support your use case.
https://review.coreboot.org/c/coreboot/+/36544/9//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36544/9//COMMIT_MSG@10 PS9, Line 10: it is required to make sure some components : are only added to the RW_REGION.
'required to make sure' begs more questions. […]
Done
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36544 )
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
Patch Set 10:
(1 comment)
Patch Set 10: Code-Review+1
(1 comment)
If RW and RO need different objects inside them I think it would make sense to generate them separately or even as a separate files, but that's part of a larger redesign. I guess this is good enough as a stop gap measure for now to support your use case.
Agreed I am definitely interested when a better solution is considered, perhaps adding a "region" option to the cbfs file properties instead of the current regions_for_file mechanism could be used. By doing that you could use the current regions_for_file assignment by default and allow it to be overridden by specifying the regions.
https://review.coreboot.org/c/coreboot/+/36544/9//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36544/9//COMMIT_MSG@10 PS9, Line 10: it is required to make sure some components : are only added to the RW_REGION.
'required to make sure' begs more questions. […]
Done
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36544 )
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
Patch Set 10:
Patch Set 10:
(1 comment)
Patch Set 10: Code-Review+1
(1 comment)
If RW and RO need different objects inside them I think it would make sense to generate them separately or even as a separate files, but that's part of a larger redesign. I guess this is good enough as a stop gap measure for now to support your use case.
Agreed I am definitely interested when a better solution is considered, perhaps adding a "region" option to the cbfs file properties instead of the current regions_for_file mechanism could be used. By doing that you could use the current regions_for_file assignment by default and allow it to be overridden by specifying the regions.
I personally think we need a tool for managing the complexities of building up an image w/ multiple CBFSes in it. Some history: this was talked about a few years back. The consensus, though not 100%, then was that people wanted to keep it in Makefiles. I think it's a harder to provide build up complex policies in a Makefile system. It may be time to rethink that conclusion.
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36544 )
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
Patch Set 10:
I personally think we need a tool for managing the complexities of building up an image w/ multiple CBFSes in it. Some history: this was talked about a few years back. The consensus, though not 100%, then was that people wanted to keep it in Makefiles. I think it's a harder to provide build up complex policies in a Makefile system. It may be time to rethink that conclusion.
A tool could work great but there should be support to create the flash image based on an input file. In my opinion there should be a mechanism that generates some general situation (like what is done with the fmd files). If more complex control is required the file specifying the content of the regions can be in the mainboard directory e.g. The developer has the option to create this file using the tool you mention or by hand.
This solution would allow control over the file location similar to fdf file in the edk2 build mechanism.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36544 )
Change subject: security/vboot: Add rw_region_only support to vboot ......................................................................
security/vboot: Add rw_region_only support to vboot
In some case where the flash space is limited or when a large payload such as LinuxBoot is used, the RO region may not be large enough to contain all components that would normally be added.
This patch adds the possibility to add specific components to the RW regions only in the same way as the RO_ONLY_SUPPORT does for the RO region.
Please note: this applies only to the items that would normally be added to all regions. If the payload is directed to the RW region only, a recovery payload needs to be added to the RO region manually.
BUG=N/A TEST=build
Change-Id: Ie0df9b5dfc6df4f24efc5582a1aec9ecfb48c44d Signed-off-by: Wim Vervoorn wvervoorn@eltan.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/36544 Reviewed-by: Arthur Heymans arthur@aheymans.xyz Reviewed-by: Frans Hendriks fhendriks@eltan.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/security/vboot/Kconfig M src/security/vboot/Makefile.inc 2 files changed, 17 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Arthur Heymans: Looks good to me, but someone else must approve Frans Hendriks: Looks good to me, approved
diff --git a/src/security/vboot/Kconfig b/src/security/vboot/Kconfig index 87bb80a..70180c7 100644 --- a/src/security/vboot/Kconfig +++ b/src/security/vboot/Kconfig @@ -220,6 +220,13 @@ Add a space delimited list of filenames that should only be in the RO section.
+config RW_REGION_ONLY + string + default "" + depends on VBOOT_SLOTS_RW_A + help + Add a space delimited list of filenames that should only be in the + RW sections.
config VBOOT_ENABLE_CBFS_FALLBACK bool diff --git a/src/security/vboot/Makefile.inc b/src/security/vboot/Makefile.inc index 31c0f5d..3e5956c 100644 --- a/src/security/vboot/Makefile.inc +++ b/src/security/vboot/Makefile.inc @@ -170,13 +170,17 @@ # Check for RW_A partition ifeq ($(CONFIG_VBOOT_SLOTS_RW_A),y) VBOOT_PARTITIONS += FW_MAIN_A +RW_PARTITIONS := FW_MAIN_A endif # Check for RW_B partition ifeq ($(CONFIG_VBOOT_SLOTS_RW_AB),y) VBOOT_PARTITIONS += FW_MAIN_B +RW_PARTITIONS += FW_MAIN_B endif
-# Define a list of files that need to be in RO only. +# Return the regions a specific file should be placed in. The files listed below and the ones +# that are specified in CONFIG_RO_REGION_ONLY are only specified in the RO region. The files +# specified in the CONFIG_RW_REGION_ONLY are only placed in the RW regions. # All other files will be installed into RO and RW regions # Use $(sort) to cut down on extra spaces that would be translated to commas regions-for-file = $(subst $(spc),$(comma),$(sort \ @@ -193,7 +197,11 @@ cmos_layout.bin \ cmos.default \ $(call strip_quotes,$(CONFIG_RO_REGION_ONLY)) \ - ,$(1)),COREBOOT,$(VBOOT_PARTITIONS)))) + ,$(1)),COREBOOT,\ + $(if $(filter \ + $(call strip_quotes,$(CONFIG_RW_REGION_ONLY)) \ + ,$(1)), $(RW_PARTITIONS), $(VBOOT_PARTITIONS) ) \ + )))
CONFIG_GBB_HWID := $(call strip_quotes,$(CONFIG_GBB_HWID)) CONFIG_GBB_BMPFV_FILE := $(call strip_quotes,$(CONFIG_GBB_BMPFV_FILE))
-
Aaron Durbin (Code Review) -
Arthur Heymans (Code Review)
-
build bot (Jenkins) (Code Review)
-
Frans Hendriks (Code Review)
-
Patrick Georgi (Code Review)
-
Wim Vervoorn (Code Review)