John Zhao has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33051
Change subject: src/arch/x86: Prevent attack on null pointer dereference ......................................................................
src/arch/x86: Prevent attack on null pointer dereference
Clang Static Analyzer version 8.0.0 detects null pointer argument in call to memory copy function. Add sanity check for pointer header to prevent null pointer dereference.
TEST=Built and boot up to kernel.
Change-Id: I7027b7cae3009a5481048bfa0536a6cbd9bef683 Signed-off-by: John Zhao john.zhao@intel.com --- M src/arch/x86/acpi.c 1 file changed, 52 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/33051/1
diff --git a/src/arch/x86/acpi.c b/src/arch/x86/acpi.c index d1dcd03..97190d2 100644 --- a/src/arch/x86/acpi.c +++ b/src/arch/x86/acpi.c @@ -218,6 +218,9 @@
memset((void *)madt, 0, sizeof(acpi_madt_t));
+ if (!header) + return; + /* Fill out header fields. */ memcpy(header->signature, "APIC", 4); memcpy(header->oem_id, OEM_ID, 6); @@ -248,6 +251,9 @@
memset((void *)mcfg, 0, sizeof(acpi_mcfg_t));
+ if (!header) + return; + /* Fill out header fields. */ memcpy(header->signature, "MCFG", 4); memcpy(header->oem_id, OEM_ID, 6); @@ -302,6 +308,9 @@ if (!lasa) return;
+ if (!header) + return; + /* Fill out header fields. */ memcpy(header->signature, "TCPA", 4); memcpy(header->oem_id, OEM_ID, 6); @@ -361,6 +370,9 @@ if (!lasa) tpm2_log_len = 0;
+ if (!header) + return; + /* Fill out header fields. */ memcpy(header->signature, "TPM2", 4); memcpy(header->oem_id, OEM_ID, 6); @@ -481,6 +493,9 @@
memset((void *)srat, 0, sizeof(acpi_srat_t));
+ if (!header) + return; + /* Fill out header fields. */ memcpy(header->signature, "SRAT", 4); memcpy(header->oem_id, OEM_ID, 6); @@ -508,6 +523,9 @@
memset((void *)dmar, 0, sizeof(acpi_dmar_t));
+ if (!header) + return; + /* Fill out header fields. */ memcpy(header->signature, "DMAR", 4); memcpy(header->oem_id, OEM_ID, 6); @@ -669,6 +687,9 @@
memset((void *)slit, 0, sizeof(acpi_slit_t));
+ if (!header) + return; + /* Fill out header fields. */ memcpy(header->signature, "SLIT", 4); memcpy(header->oem_id, OEM_ID, 6); @@ -694,6 +715,9 @@
memset((void *)hpet, 0, sizeof(acpi_hpet_t));
+ if (!header) + return; + /* Fill out header fields. */ memcpy(header->signature, "HPET", 4); memcpy(header->oem_id, OEM_ID, 6); @@ -728,6 +752,9 @@
memset((void *)vfct, 0, sizeof(struct acpi_vfct));
+ if (!header) + return; + /* Fill out header fields. */ memcpy(header->signature, "VFCT", 4); memcpy(header->oem_id, OEM_ID, 6); @@ -754,6 +781,9 @@
memset((void *)ivrs, 0, sizeof(acpi_ivrs_t));
+ if (!header) + return; + /* Fill out header fields. */ memcpy(header->signature, "IVRS", 4); memcpy(header->oem_id, OEM_ID, 6); @@ -807,6 +837,10 @@ current = (uintptr_t)dbg2; memset(dbg2, 0, sizeof(acpi_dbg2_header_t)); header = &(dbg2->header); + + if (!header) + return; + header->revision = get_acpi_table_revision(DBG2); memcpy(header->signature, "DBG2", 4); memcpy(header->oem_id, OEM_ID, 6); @@ -926,6 +960,9 @@ { acpi_header_t *header = &(rsdt->header);
+ if (!header) + return; + /* Fill out header fields. */ memcpy(header->signature, "RSDT", 4); memcpy(header->oem_id, oem_id, 6); @@ -946,6 +983,9 @@ { acpi_header_t *header = &(xsdt->header);
+ if (!header) + return; + /* Fill out header fields. */ memcpy(header->signature, "XSDT", 4); memcpy(header->oem_id, oem_id, 6); @@ -1046,7 +1086,8 @@
memcpy(pos, data, data_len); len += data_len; - header->length += len; + if(header) + header->length += len;
return len; } @@ -1058,6 +1099,9 @@ acpi_header_t *header = &(hest->header);
memset(hest, 0, sizeof(acpi_hest_t)); + + if (!header) + return;
memcpy(header->signature, "HEST", 4); memcpy(header->oem_id, OEM_ID, 6); @@ -1080,6 +1124,9 @@
memset(bert, 0, sizeof(acpi_bert_t));
+ if (!header) + return; + memcpy(header->signature, "BERT", 4); memcpy(header->oem_id, OEM_ID, 6); memcpy(header->oem_table_id, ACPI_TABLE_CREATOR, 8); @@ -1101,6 +1148,10 @@ acpi_header_t *header = &(fadt->header);
memset((void *) fadt, 0, sizeof(acpi_fadt_t)); + + if (!header) + return; + memcpy(header->signature, "FACP", 4); header->length = sizeof(acpi_fadt_t); header->revision = get_acpi_table_revision(FADT);
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33051 )
Change subject: src/arch/x86: Prevent attack on null pointer dereference ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/#/c/33051/1/src/arch/x86/acpi.c File src/arch/x86/acpi.c:
https://review.coreboot.org/#/c/33051/1/src/arch/x86/acpi.c@1089 PS1, Line 1089: if(header) space required before the open parenthesis '('
https://review.coreboot.org/#/c/33051/1/src/arch/x86/acpi.c@1102 PS1, Line 1102: trailing whitespace
Hello Balaji Manigandan, build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33051
to look at the new patch set (#2).
Change subject: src/arch/x86: Prevent attack on null pointer dereference ......................................................................
src/arch/x86: Prevent attack on null pointer dereference
Clang Static Analyzer version 8.0.0 detects null pointer argument in call to memory copy function. Add sanity check for pointer header to prevent null pointer dereference.
TEST=Built and boot up to kernel.
Change-Id: I7027b7cae3009a5481048bfa0536a6cbd9bef683 Signed-off-by: John Zhao john.zhao@intel.com --- M src/arch/x86/acpi.c 1 file changed, 52 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/33051/2
Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33051 )
Change subject: src/arch/x86: Prevent attack on null pointer dereference ......................................................................
Patch Set 2: Code-Review+1
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33051 )
Change subject: src/arch/x86: Prevent attack on null pointer dereference ......................................................................
Patch Set 2: Code-Review+2
Felix Held has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33051 )
Change subject: src/arch/x86: Prevent attack on null pointer dereference ......................................................................
src/arch/x86: Prevent attack on null pointer dereference
Clang Static Analyzer version 8.0.0 detects null pointer argument in call to memory copy function. Add sanity check for pointer header to prevent null pointer dereference.
TEST=Built and boot up to kernel.
Change-Id: I7027b7cae3009a5481048bfa0536a6cbd9bef683 Signed-off-by: John Zhao john.zhao@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/33051 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Lance Zhao lance.zhao@gmail.com Reviewed-by: Felix Held felix-coreboot@felixheld.de --- M src/arch/x86/acpi.c 1 file changed, 52 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Felix Held: Looks good to me, approved Lance Zhao: Looks good to me, but someone else must approve
diff --git a/src/arch/x86/acpi.c b/src/arch/x86/acpi.c index d1dcd03..bf9813c 100644 --- a/src/arch/x86/acpi.c +++ b/src/arch/x86/acpi.c @@ -218,6 +218,9 @@
memset((void *)madt, 0, sizeof(acpi_madt_t));
+ if (!header) + return; + /* Fill out header fields. */ memcpy(header->signature, "APIC", 4); memcpy(header->oem_id, OEM_ID, 6); @@ -248,6 +251,9 @@
memset((void *)mcfg, 0, sizeof(acpi_mcfg_t));
+ if (!header) + return; + /* Fill out header fields. */ memcpy(header->signature, "MCFG", 4); memcpy(header->oem_id, OEM_ID, 6); @@ -302,6 +308,9 @@ if (!lasa) return;
+ if (!header) + return; + /* Fill out header fields. */ memcpy(header->signature, "TCPA", 4); memcpy(header->oem_id, OEM_ID, 6); @@ -361,6 +370,9 @@ if (!lasa) tpm2_log_len = 0;
+ if (!header) + return; + /* Fill out header fields. */ memcpy(header->signature, "TPM2", 4); memcpy(header->oem_id, OEM_ID, 6); @@ -481,6 +493,9 @@
memset((void *)srat, 0, sizeof(acpi_srat_t));
+ if (!header) + return; + /* Fill out header fields. */ memcpy(header->signature, "SRAT", 4); memcpy(header->oem_id, OEM_ID, 6); @@ -508,6 +523,9 @@
memset((void *)dmar, 0, sizeof(acpi_dmar_t));
+ if (!header) + return; + /* Fill out header fields. */ memcpy(header->signature, "DMAR", 4); memcpy(header->oem_id, OEM_ID, 6); @@ -669,6 +687,9 @@
memset((void *)slit, 0, sizeof(acpi_slit_t));
+ if (!header) + return; + /* Fill out header fields. */ memcpy(header->signature, "SLIT", 4); memcpy(header->oem_id, OEM_ID, 6); @@ -694,6 +715,9 @@
memset((void *)hpet, 0, sizeof(acpi_hpet_t));
+ if (!header) + return; + /* Fill out header fields. */ memcpy(header->signature, "HPET", 4); memcpy(header->oem_id, OEM_ID, 6); @@ -728,6 +752,9 @@
memset((void *)vfct, 0, sizeof(struct acpi_vfct));
+ if (!header) + return; + /* Fill out header fields. */ memcpy(header->signature, "VFCT", 4); memcpy(header->oem_id, OEM_ID, 6); @@ -754,6 +781,9 @@
memset((void *)ivrs, 0, sizeof(acpi_ivrs_t));
+ if (!header) + return; + /* Fill out header fields. */ memcpy(header->signature, "IVRS", 4); memcpy(header->oem_id, OEM_ID, 6); @@ -807,6 +837,10 @@ current = (uintptr_t)dbg2; memset(dbg2, 0, sizeof(acpi_dbg2_header_t)); header = &(dbg2->header); + + if (!header) + return; + header->revision = get_acpi_table_revision(DBG2); memcpy(header->signature, "DBG2", 4); memcpy(header->oem_id, OEM_ID, 6); @@ -926,6 +960,9 @@ { acpi_header_t *header = &(rsdt->header);
+ if (!header) + return; + /* Fill out header fields. */ memcpy(header->signature, "RSDT", 4); memcpy(header->oem_id, oem_id, 6); @@ -946,6 +983,9 @@ { acpi_header_t *header = &(xsdt->header);
+ if (!header) + return; + /* Fill out header fields. */ memcpy(header->signature, "XSDT", 4); memcpy(header->oem_id, oem_id, 6); @@ -1046,7 +1086,8 @@
memcpy(pos, data, data_len); len += data_len; - header->length += len; + if (header) + header->length += len;
return len; } @@ -1059,6 +1100,9 @@
memset(hest, 0, sizeof(acpi_hest_t));
+ if (!header) + return; + memcpy(header->signature, "HEST", 4); memcpy(header->oem_id, OEM_ID, 6); memcpy(header->oem_table_id, ACPI_TABLE_CREATOR, 8); @@ -1080,6 +1124,9 @@
memset(bert, 0, sizeof(acpi_bert_t));
+ if (!header) + return; + memcpy(header->signature, "BERT", 4); memcpy(header->oem_id, OEM_ID, 6); memcpy(header->oem_table_id, ACPI_TABLE_CREATOR, 8); @@ -1101,6 +1148,10 @@ acpi_header_t *header = &(fadt->header);
memset((void *) fadt, 0, sizeof(acpi_fadt_t)); + + if (!header) + return; + memcpy(header->signature, "FACP", 4); header->length = sizeof(acpi_fadt_t); header->revision = get_acpi_table_revision(FADT);
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33051 )
Change subject: src/arch/x86: Prevent attack on null pointer dereference ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/#/c/33051/3/src/arch/x86/acpi.c File src/arch/x86/acpi.c:
https://review.coreboot.org/#/c/33051/3/src/arch/x86/acpi.c@219 PS3, Line 219: memset((void *)madt, 0, sizeof(acpi_madt_t)); IMHO one should have tested for madt!=NULL before this line. Possibly would have taken care of the static analyzer complaint too as header == &(madt->header) == madt.
https://review.coreboot.org/#/c/33051/3/src/arch/x86/acpi.c@225 PS3, Line 225: memcpy(header->signature, "APIC", 4); I am guessing analyzer complained on this line only? The signature is the first field of hearder, so only this might be mistaken for NULL by analyzer?
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33051 )
Change subject: src/arch/x86: Prevent attack on null pointer dereference ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/33051/3/src/arch/x86/acpi.c File src/arch/x86/acpi.c:
https://review.coreboot.org/c/coreboot/+/33051/3/src/arch/x86/acpi.c@219 PS3, Line 219: memset((void *)madt, 0, sizeof(acpi_madt_t));
IMHO one should have tested for madt!=NULL before this line. […]
Alternatively header!=NULL check could be moved before memset to guard null pointer dereference.
https://review.coreboot.org/c/coreboot/+/33051/3/src/arch/x86/acpi.c@225 PS3, Line 225: memcpy(header->signature, "APIC", 4);
I am guessing analyzer complained on this line only? The signature is the first field of hearder, so […]
yes, it only complains on this line.
-
build bot (Jenkins) (Code Review) -
Felix Held (Code Review)
-
John Zhao (Code Review)
-
Kyösti Mälkki (Code Review)
-
Lance Zhao (Code Review)