Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32971
Change subject: arch/x86: Do not add properties to null DP packages ......................................................................
arch/x86: Do not add properties to null DP packages
It doesn't make sense to add a property to a non-existent device property package. However, some of these functions will proceed anyway and allocate a new device property package, add the property to that, and then immediately leak the new package. This changes all the acpi_dp_add_* functions to ignore a null package.
Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 135745{6,7}, 138029{2-6} Change-Id: I664dcdbaa6b1b8a3aeb9a0126d622e2ffb736efd --- M src/arch/x86/acpi_device.c 1 file changed, 18 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/32971/1
diff --git a/src/arch/x86/acpi_device.c b/src/arch/x86/acpi_device.c index 5d8777f..89708d5 100644 --- a/src/arch/x86/acpi_device.c +++ b/src/arch/x86/acpi_device.c @@ -740,6 +740,9 @@ const struct acpi_dp *prop; size_t i, properties_added = 0;
+ if (!dp) + return 0; + for (i = 0; i < property_count; i++) { prop = &property_list[i];
@@ -775,6 +778,9 @@ struct acpi_dp *acpi_dp_add_integer(struct acpi_dp *dp, const char *name, uint64_t value) { + if (!dp) + return NULL; + struct acpi_dp *new = acpi_dp_new(dp, ACPI_DP_TYPE_INTEGER, name);
if (new) @@ -786,6 +792,9 @@ struct acpi_dp *acpi_dp_add_string(struct acpi_dp *dp, const char *name, const char *string) { + if (!dp) + return NULL; + struct acpi_dp *new = acpi_dp_new(dp, ACPI_DP_TYPE_STRING, name);
if (new) @@ -797,6 +806,9 @@ struct acpi_dp *acpi_dp_add_reference(struct acpi_dp *dp, const char *name, const char *reference) { + if (!dp) + return NULL; + struct acpi_dp *new = acpi_dp_new(dp, ACPI_DP_TYPE_REFERENCE, name);
if (new) @@ -810,7 +822,7 @@ { struct acpi_dp *new;
- if (!child || child->type != ACPI_DP_TYPE_TABLE) + if (!dp || !child || child->type != ACPI_DP_TYPE_TABLE) return NULL;
new = acpi_dp_new(dp, ACPI_DP_TYPE_CHILD, name); @@ -826,7 +838,7 @@ { struct acpi_dp *new;
- if (!array || array->type != ACPI_DP_TYPE_TABLE) + if (!dp || !array || array->type != ACPI_DP_TYPE_TABLE) return NULL;
new = acpi_dp_new(dp, ACPI_DP_TYPE_ARRAY, array->name); @@ -842,7 +854,7 @@ struct acpi_dp *dp_array; int i;
- if (len <= 0) + if (!dp || len <= 0) return NULL;
dp_array = acpi_dp_new_table(name); @@ -862,6 +874,9 @@ const char *ref, int index, int pin, int active_low) { + if (!dp) + return NULL; + struct acpi_dp *gpio = acpi_dp_new_table(name);
if (!gpio)
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32971 )
Change subject: arch/x86: Do not add properties to null DP packages ......................................................................
Patch Set 1: Code-Review+1
Hello HAOUAS Elyes, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32971
to look at the new patch set (#2).
Change subject: arch/x86: Do not add properties to null DP packages ......................................................................
arch/x86: Do not add properties to null DP packages
It doesn't make sense to add a property to a non-existent device property package. However, some of these functions will proceed anyway and allocate a new device property package, add the property to that, and then immediately leak the new package. This changes all the acpi_dp_add_* functions to ignore a null package.
Change-Id: I664dcdbaa6b1b8a3aeb9a0126d622e2ffb736efd Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 135745{6,7}, 138029{2-6} --- M src/arch/x86/acpi_device.c 1 file changed, 18 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/32971/2
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32971 )
Change subject: arch/x86: Do not add properties to null DP packages ......................................................................
Patch Set 3: Code-Review+1
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32971 )
Change subject: arch/x86: Do not add properties to null DP packages ......................................................................
Patch Set 3: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/32971/3/src/arch/x86/acpi_device.c File src/arch/x86/acpi_device.c:
https://review.coreboot.org/#/c/32971/3/src/arch/x86/acpi_device.c@743 PS3, Line 743: ) property_list should be checked as well?
Hello HAOUAS Elyes, David Hendricks, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32971
to look at the new patch set (#4).
Change subject: arch/x86: Do not add properties to null DP packages ......................................................................
arch/x86: Do not add properties to null DP packages
It doesn't make sense to add a property to a non-existent Device Property package. However, some of these functions will proceed anyway and allocate a new Device Property package, add the property to that, and then immediately leak the new package. This changes all the acpi_dp_add_* functions to ignore a null package.
Change-Id: I664dcdbaa6b1b8a3aeb9a0126d622e2ffb736efd Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 135745{6,7}, 138029{2-6} --- M src/arch/x86/acpi_device.c 1 file changed, 18 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/32971/4
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32971 )
Change subject: arch/x86: Do not add properties to null DP packages ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/32971/3/src/arch/x86/acpi_device.c File src/arch/x86/acpi_device.c:
https://review.coreboot.org/#/c/32971/3/src/arch/x86/acpi_device.c@743 PS3, Line 743: )
property_list should be checked as well?
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32971 )
Change subject: arch/x86: Do not add properties to null DP packages ......................................................................
Patch Set 4: Code-Review+2
Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32971 )
Change subject: arch/x86: Do not add properties to null DP packages ......................................................................
arch/x86: Do not add properties to null DP packages
It doesn't make sense to add a property to a non-existent Device Property package. However, some of these functions will proceed anyway and allocate a new Device Property package, add the property to that, and then immediately leak the new package. This changes all the acpi_dp_add_* functions to ignore a null package.
Change-Id: I664dcdbaa6b1b8a3aeb9a0126d622e2ffb736efd Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 135745{6,7}, 138029{2-6} Reviewed-on: https://review.coreboot.org/c/coreboot/+/32971 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/arch/x86/acpi_device.c 1 file changed, 18 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/arch/x86/acpi_device.c b/src/arch/x86/acpi_device.c index 5d8777f..57fbc89 100644 --- a/src/arch/x86/acpi_device.c +++ b/src/arch/x86/acpi_device.c @@ -740,6 +740,9 @@ const struct acpi_dp *prop; size_t i, properties_added = 0;
+ if (!dp || !property_list) + return 0; + for (i = 0; i < property_count; i++) { prop = &property_list[i];
@@ -775,6 +778,9 @@ struct acpi_dp *acpi_dp_add_integer(struct acpi_dp *dp, const char *name, uint64_t value) { + if (!dp) + return NULL; + struct acpi_dp *new = acpi_dp_new(dp, ACPI_DP_TYPE_INTEGER, name);
if (new) @@ -786,6 +792,9 @@ struct acpi_dp *acpi_dp_add_string(struct acpi_dp *dp, const char *name, const char *string) { + if (!dp) + return NULL; + struct acpi_dp *new = acpi_dp_new(dp, ACPI_DP_TYPE_STRING, name);
if (new) @@ -797,6 +806,9 @@ struct acpi_dp *acpi_dp_add_reference(struct acpi_dp *dp, const char *name, const char *reference) { + if (!dp) + return NULL; + struct acpi_dp *new = acpi_dp_new(dp, ACPI_DP_TYPE_REFERENCE, name);
if (new) @@ -810,7 +822,7 @@ { struct acpi_dp *new;
- if (!child || child->type != ACPI_DP_TYPE_TABLE) + if (!dp || !child || child->type != ACPI_DP_TYPE_TABLE) return NULL;
new = acpi_dp_new(dp, ACPI_DP_TYPE_CHILD, name); @@ -826,7 +838,7 @@ { struct acpi_dp *new;
- if (!array || array->type != ACPI_DP_TYPE_TABLE) + if (!dp || !array || array->type != ACPI_DP_TYPE_TABLE) return NULL;
new = acpi_dp_new(dp, ACPI_DP_TYPE_ARRAY, array->name); @@ -842,7 +854,7 @@ struct acpi_dp *dp_array; int i;
- if (len <= 0) + if (!dp || len <= 0) return NULL;
dp_array = acpi_dp_new_table(name); @@ -862,6 +874,9 @@ const char *ref, int index, int pin, int active_low) { + if (!dp) + return NULL; + struct acpi_dp *gpio = acpi_dp_new_table(name);
if (!gpio)
-
Furquan Shaikh (Code Review) -
HAOUAS Elyes (Code Review)
-
Jacob Garber (Code Review)
-
Martin Roth (Code Review)