John Zhao has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44186 )
Change subject: security/intel/txt: Avoid shifting by a negative value ......................................................................
security/intel/txt: Avoid shifting by a negative value
Coverity detects an integer handling issue with BAD_SHIFT. The inline function log2_ceil(u32 x) { return (x == 0) ? -1 : log2(x * 2 - 1); } could return -1, which causes shifting by a negative amount value and has undefined behavior. Add sanity check for the acm_header->size to avoid shifting negative value.
Found-by: Coverity CID 1431124 TEST=None
Signed-off-by: John Zhao john.zhao@intel.com Change-Id: Ic687349b14917e39d2a8186968037ca2521c7cdc --- M src/security/intel/txt/common.c 1 file changed, 3 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/44186/1
diff --git a/src/security/intel/txt/common.c b/src/security/intel/txt/common.c index d3e18376..d641585 100644 --- a/src/security/intel/txt/common.c +++ b/src/security/intel/txt/common.c @@ -138,6 +138,7 @@ { const struct acm_header_v0 *acm_header = (struct acm_header_v0 *)ptr; uint32_t max_size_acm_area = 0; + const size_t acm_len = 0;
if (acm_header->module_type != CHIPSET_ACM) return ACM_E_TYPE_NOT_MATCH; @@ -163,7 +164,8 @@ * SAFER MODE EXTENSIONS REFERENCE. * Intel 64 and IA-32 Architectures Software Developer Manuals Vol 2D */ - const size_t acm_len = 1UL << log2_ceil((acm_header->size & 0xffffff) << 2); + if (acm_header->size != 0) + acm_len = 1UL << log2_ceil((acm_header->size & 0xffffff) << 2); if (max_size_acm_area < acm_len) { printk(BIOS_ERR, "TEE-TXT: BIOS ACM doesn't fit into AC execution region\n"); return ACM_E_NOT_FIT_INTO_CPU_ACM_MEM;
Hello build bot (Jenkins), Tim Wawrzynczak, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44186
to look at the new patch set (#2).
Change subject: security/intel/txt: Avoid shifting by a negative value ......................................................................
security/intel/txt: Avoid shifting by a negative value
Coverity detects an integer handling issue with BAD_SHIFT. The inline function log2_ceil(u32 x) { return (x == 0) ? -1 : log2(x * 2 - 1); } could return -1, which causes shifting by a negative amount value and has undefined behavior. Add sanity check for the acm_header->size to avoid shifting negative value.
Found-by: Coverity CID 1431124 TEST=None
Signed-off-by: John Zhao john.zhao@intel.com Change-Id: Ic687349b14917e39d2a8186968037ca2521c7cdc --- M src/security/intel/txt/common.c 1 file changed, 3 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/44186/2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44186 )
Change subject: security/intel/txt: Avoid shifting by a negative value ......................................................................
Patch Set 2: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44186 )
Change subject: security/intel/txt: Avoid shifting by a negative value ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
While I agree this is a potential problem, I'd suggest a cleaner way to handle it.
https://review.coreboot.org/c/coreboot/+/44186/2/src/security/intel/txt/comm... File src/security/intel/txt/common.c:
https://review.coreboot.org/c/coreboot/+/44186/2/src/security/intel/txt/comm... PS2, Line 152: I'd just make sure that the size is non-zero here:
if (acm_header->size == 0) return ACM_E_SIZE_INCORRECT;
This will return the correct error code for such a case.
Hello build bot (Jenkins), Tim Wawrzynczak, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44186
to look at the new patch set (#3).
Change subject: security/intel/txt: Avoid shifting by a negative value ......................................................................
security/intel/txt: Avoid shifting by a negative value
Coverity detects an integer handling issue with BAD_SHIFT. The inline function log2_ceil(u32 x) { return (x == 0) ? -1 : log2(x * 2 - 1); } could return -1, which causes shifting by a negative amount value and has undefined behavior. Add sanity check for the acm_header->size to avoid shifting negative value.
Found-by: Coverity CID 1431124 TEST=None
Signed-off-by: John Zhao john.zhao@intel.com Change-Id: Ic687349b14917e39d2a8186968037ca2521c7cdc --- M src/security/intel/txt/common.c 1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/44186/3
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44186 )
Change subject: security/intel/txt: Avoid shifting by a negative value ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44186/2/src/security/intel/txt/comm... File src/security/intel/txt/common.c:
https://review.coreboot.org/c/coreboot/+/44186/2/src/security/intel/txt/comm... PS2, Line 152:
I'd just make sure that the size is non-zero here: […]
Good call, Angel. My repo wasn't up to date and I couldn't find the ACM_E_* defines...
Hello build bot (Jenkins), Tim Wawrzynczak, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44186
to look at the new patch set (#4).
Change subject: security/intel/txt: Avoid shifting by a negative value ......................................................................
security/intel/txt: Avoid shifting by a negative value
Coverity detects an integer handling issue with BAD_SHIFT. The inline function log2_ceil(u32 x) { return (x == 0) ? -1 : log2(x * 2 - 1); } could return -1, which causes shifting by a negative amount value and has undefined behavior. Add sanity check for the acm_header->size to avoid shifting negative value.
Found-by: Coverity CID 1431124 TEST=None
Signed-off-by: John Zhao john.zhao@intel.com Change-Id: Ic687349b14917e39d2a8186968037ca2521c7cdc --- M src/security/intel/txt/common.c 1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/44186/4
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44186 )
Change subject: security/intel/txt: Avoid shifting by a negative value ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44186/2/src/security/intel/txt/comm... File src/security/intel/txt/common.c:
https://review.coreboot.org/c/coreboot/+/44186/2/src/security/intel/txt/comm... PS2, Line 152:
Good call, Angel. My repo wasn't up to date and I couldn't find the ACM_E_* defines...
Ack
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44186 )
Change subject: security/intel/txt: Avoid shifting by a negative value ......................................................................
Patch Set 4: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/44186/2/src/security/intel/txt/comm... File src/security/intel/txt/common.c:
https://review.coreboot.org/c/coreboot/+/44186/2/src/security/intel/txt/comm... PS2, Line 152:
Good call, Angel. My repo wasn't up to date and I couldn't find the ACM_E_* defines...
Note: I've suggested this place because `acm_header->size` is used on the statement right below. The place where it was added in PS3 would still work, but the comment about maximum authenticated code size gets misplaced
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44186 )
Change subject: security/intel/txt: Avoid shifting by a negative value ......................................................................
Patch Set 4: Code-Review+2
Looks legit to me (famous last words...)
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44186 )
Change subject: security/intel/txt: Avoid shifting by a negative value ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44186/2/src/security/intel/txt/comm... File src/security/intel/txt/common.c:
https://review.coreboot.org/c/coreboot/+/44186/2/src/security/intel/txt/comm... PS2, Line 152:
Note: I've suggested this place because `acm_header->size` is used on the statement right below. […]
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44186 )
Change subject: security/intel/txt: Avoid shifting by a negative value ......................................................................
Patch Set 4: Code-Review+2
Thanks!
Philipp Deppenwiese has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44186 )
Change subject: security/intel/txt: Avoid shifting by a negative value ......................................................................
security/intel/txt: Avoid shifting by a negative value
Coverity detects an integer handling issue with BAD_SHIFT. The inline function log2_ceil(u32 x) { return (x == 0) ? -1 : log2(x * 2 - 1); } could return -1, which causes shifting by a negative amount value and has undefined behavior. Add sanity check for the acm_header->size to avoid shifting negative value.
Found-by: Coverity CID 1431124 TEST=None
Signed-off-by: John Zhao john.zhao@intel.com Change-Id: Ic687349b14917e39d2a8186968037ca2521c7cdc Reviewed-on: https://review.coreboot.org/c/coreboot/+/44186 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/security/intel/txt/common.c 1 file changed, 3 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved Tim Wawrzynczak: Looks good to me, approved
diff --git a/src/security/intel/txt/common.c b/src/security/intel/txt/common.c index d3e18376..f16bbea 100644 --- a/src/security/intel/txt/common.c +++ b/src/security/intel/txt/common.c @@ -149,6 +149,9 @@ if (acm_header->module_vendor != INTEL_ACM_VENDOR) return ACM_E_MODULE_VENDOR_NOT_INTEL;
+ if (acm_header->size == 0) + return ACM_E_SIZE_INCORRECT; + if (((acm_header->header_len + acm_header->scratch_size) * sizeof(uint32_t) + sizeof(struct acm_info_table)) > (acm_header->size & 0xffffff) * sizeof(uint32_t)) { return ACM_E_SIZE_INCORRECT;
-
Angel Pons (Code Review) -
John Zhao (Code Review)
-
Philipp Deppenwiese (Code Review)
-
Tim Wawrzynczak (Code Review)