Change in coreboot[master]: soc/intel/common: Check sizes of CSE CBFS RW blob and CSE RW BP
Sridhar Siricilla has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48423 )
Change subject: soc/intel/common: Check sizes of CSE CBFS RW blob and CSE RW BP ......................................................................
soc/intel/common: Check sizes of CSE CBFS RW blob and CSE RW BP
The patch triggeres CrOS recovery mode if the sizes of CSE CBFS RW blob and CSE RW boot are different.
TEST=Verified on drawcia.
Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Change-Id: I8be589eae905b1a54a8cf981ccd3a00bd5e733f5 --- M src/soc/intel/common/block/cse/cse_lite.c 1 file changed, 5 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/48423/1
diff --git a/src/soc/intel/common/block/cse/cse_lite.c b/src/soc/intel/common/block/cse/cse_lite.c index eb4be6e..535b9fd 100644 --- a/src/soc/intel/common/block/cse/cse_lite.c +++ b/src/soc/intel/common/block/cse/cse_lite.c @@ -74,6 +74,9 @@
/* CSE CBFS RW metadata is not found */ CSE_LITE_SKU_RW_METADATA_NOT_FOUND = 10, + + /* CSE CBFS RW blob layout is not correct */ + CSE_LITE_SKU_LAYOUT_MISMATCH_ERROR = 11, };
/* @@ -674,6 +677,8 @@ const void *cse_cbfs_rw, const size_t cse_blob_sz, struct region_device *target_rdev) { + if (region_device_sz(target_rdev) != cse_blob_sz) + return CSE_LITE_SKU_LAYOUT_MISMATCH_ERROR;
if (!cse_erase_rw_region(target_rdev)) return CSE_LITE_SKU_FW_UPDATE_ERROR;
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48423 )
Change subject: soc/intel/common: Check sizes of CSE CBFS RW blob and CSE RW BP ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48423/1/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/48423/1/src/soc/intel/common/block/... PS1, Line 680: region_device_sz(target_rdev) != cse_blob_sz I don't think this check is correct. We need to ensure that the target_rdev size is greater than the size of blob being written. We cannot expect them to be the same always. Think of the scenarios where we start with larger RW size on flash where some part is left unused and blobs that are written to that region can be smaller.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48423 )
Change subject: soc/intel/common: Check sizes of CSE CBFS RW blob and CSE RW BP ......................................................................
Patch Set 3: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/48423/3/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/48423/3/src/soc/intel/common/block/... PS3, Line 680: if (region_device_sz(target_rdev) < cse_blob_sz) It would be helpful to have an error print in this case indicating the sizes:
printk(BIOS_ERR, "RW update does not fit. CSE RW flash region size: %zx, Update blob size: %zx\n", region_device_sz(target_rdev), cse_blob_sz);
https://review.coreboot.org/c/coreboot/+/48423/3/src/soc/intel/common/block/... PS3, Line 681: CSE_LITE_SKU_LAYOUT_MISMATCH_ERROR I would really like to catch this error at build time instead of waiting until runtime to know that a failure has occured. It will require some changes to FMAP to handle this. Can you please raise a partner issue to follow up on it?
Hello build bot (Jenkins), Furquan Shaikh, Jamie Ryu, Rizwan Qureshi, Krishna P Bhat D, Tim Wawrzynczak, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48423
to look at the new patch set (#4).
Change subject: soc/intel/common: Check sizes of CSE CBFS RW blob and CSE RW BP ......................................................................
soc/intel/common: Check sizes of CSE CBFS RW blob and CSE RW BP
The patch triggeres CrOS recovery mode if the sizes of CSE CBFS RW blob and CSE RW boot are different.
TEST=Verified on drawcia.
Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Change-Id: I8be589eae905b1a54a8cf981ccd3a00bd5e733f5 --- M src/soc/intel/common/block/cse/cse_lite.c 1 file changed, 9 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/48423/4
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48423 )
Change subject: soc/intel/common: Check sizes of CSE CBFS RW blob and CSE RW BP ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/48423/3/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/48423/3/src/soc/intel/common/block/... PS3, Line 680: if (region_device_sz(target_rdev) < cse_blob_sz)
It would be helpful to have an error print in this case indicating the sizes: […]
Ack
https://review.coreboot.org/c/coreboot/+/48423/3/src/soc/intel/common/block/... PS3, Line 681: CSE_LITE_SKU_LAYOUT_MISMATCH_ERROR
I would really like to catch this error at build time instead of waiting until runtime to know that […]
I have raised crosbug(B:175304872) to track the changes.
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48423 )
Change subject: soc/intel/common: Check sizes of CSE CBFS RW blob and CSE RW BP ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48423/3/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/48423/3/src/soc/intel/common/block/... PS3, Line 681: CSE_LITE_SKU_LAYOUT_MISMATCH_ERROR
I have raised crosbug(B:175304872) to track the changes.
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48423 )
Change subject: soc/intel/common: Check sizes of CSE CBFS RW blob and CSE RW BP ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/48423/4/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/48423/4/src/soc/intel/common/block/... PS4, Line 682: "Update blob size:%zx\n" No need to split printk string. Coreboot allows the printk string to go beyond the column limit. It is easier that way when grepping for error messages.
Hello build bot (Jenkins), Furquan Shaikh, Jamie Ryu, Rizwan Qureshi, Krishna P Bhat D, Tim Wawrzynczak, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48423
to look at the new patch set (#5).
Change subject: soc/intel/common: Check sizes of CSE CBFS RW blob and CSE RW BP ......................................................................
soc/intel/common: Check sizes of CSE CBFS RW blob and CSE RW BP
The patch triggeres CrOS recovery mode if the sizes of CSE CBFS RW blob and CSE RW boot are different.
TEST=Verified on drawcia.
Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Change-Id: I8be589eae905b1a54a8cf981ccd3a00bd5e733f5 --- M src/soc/intel/common/block/cse/cse_lite.c 1 file changed, 8 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/48423/5
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48423 )
Change subject: soc/intel/common: Check sizes of CSE CBFS RW blob and CSE RW BP ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48423/4/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/48423/4/src/soc/intel/common/block/... PS4, Line 682: "Update blob size:%zx\n"
No need to split printk string. Coreboot allows the printk string to go beyond the column limit. […]
Ack
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48423 )
Change subject: soc/intel/common: Check sizes of CSE CBFS RW blob and CSE RW BP ......................................................................
Patch Set 5: Code-Review+2
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48423 )
Change subject: soc/intel/common: Check sizes of CSE CBFS RW blob and CSE RW BP ......................................................................
Patch Set 6: Code-Review+2
Jamie Ryu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48423 )
Change subject: soc/intel/common: Check sizes of CSE CBFS RW blob and CSE RW BP ......................................................................
Patch Set 6: Code-Review+1
Tim Wawrzynczak has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48423 )
Change subject: soc/intel/common: Check sizes of CSE CBFS RW blob and CSE RW BP ......................................................................
soc/intel/common: Check sizes of CSE CBFS RW blob and CSE RW BP
The patch triggeres CrOS recovery mode if the sizes of CSE CBFS RW blob and CSE RW boot are different.
TEST=Verified on drawcia.
Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Change-Id: I8be589eae905b1a54a8cf981ccd3a00bd5e733f5 Reviewed-on: https://review.coreboot.org/c/coreboot/+/48423 Reviewed-by: Karthik Ramasubramanian kramasub@google.com Reviewed-by: Jamie Ryu jamie.m.ryu@intel.com Reviewed-by: Furquan Shaikh furquan@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/common/block/cse/cse_lite.c 1 file changed, 8 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Karthik Ramasubramanian: Looks good to me, approved Jamie Ryu: Looks good to me, but someone else must approve
diff --git a/src/soc/intel/common/block/cse/cse_lite.c b/src/soc/intel/common/block/cse/cse_lite.c index eb4be6e..8e89723 100644 --- a/src/soc/intel/common/block/cse/cse_lite.c +++ b/src/soc/intel/common/block/cse/cse_lite.c @@ -74,6 +74,9 @@
/* CSE CBFS RW metadata is not found */ CSE_LITE_SKU_RW_METADATA_NOT_FOUND = 10, + + /* CSE CBFS RW blob layout is not correct */ + CSE_LITE_SKU_LAYOUT_MISMATCH_ERROR = 11, };
/* @@ -674,6 +677,11 @@ const void *cse_cbfs_rw, const size_t cse_blob_sz, struct region_device *target_rdev) { + if (region_device_sz(target_rdev) < cse_blob_sz) { + printk(BIOS_ERR, "RW update does not fit. CSE RW flash region size: %zx, Update blob size:%zx\n", + region_device_sz(target_rdev), cse_blob_sz); + return CSE_LITE_SKU_LAYOUT_MISMATCH_ERROR; + }
if (!cse_erase_rw_region(target_rdev)) return CSE_LITE_SKU_FW_UPDATE_ERROR;
-
Furquan Shaikh (Code Review) -
Jamie Ryu (Code Review)
-
Karthik Ramasubramanian (Code Review)
-
Sridhar Siricilla (Code Review)
-
Tim Wawrzynczak (Code Review)