ACK: [PATCH 2/2] acpi: dbg2: fix segfault and refactor dbg2_obj_find
Colin Ian King
colin.king at canonical.com
Mon Oct 31 23:47:14 UTC 2016
On 31/10/16 17:45, Alex Hung wrote:
> On 2016-10-31 07:49 AM, Erico Nunes wrote:
>> The dbg2 test may crash on some systems due to dbg2_obj_find not being
>> able to handle malformed or nonstandard NamespaceString attributes.
>> It has been seen to crash due to NamespaceString being described with a
>> leading escape backslash, such as '\\_SB_.AHBC.URT1'. In these cases,
>> regardless of whether the test should pass or not, it should not crash
>> fwts.
>> In order to avoid more problems with exact length calculation, the
>> 'expanded' string size is just moved to a reasonably large size which
>> should be able to fit any NamespaceString.
>>
>> It has also been observed that the stao table test implements a very
>> similar routine and therefore may be vulnerable to the same sort of
>> problem, so that code has been refactored into fwts_acpi_obj_find which
>> can be used by both stao and dbg2 tests.
>>
>> The printed strings in case of error logs are now printed with the
>> unexpanded string, so the expected outputs used in 'make check' have
>> also been updated to reflect that.
>>
>> Signed-off-by: Erico Nunes <ernunes at redhat.com>
>> ---
>> fwts-test/dbg2-0001/dbg2-0002.log | 2 +-
>> fwts-test/stao-0001/stao-0002.log | 6 +--
>> src/acpi/dbg2/dbg2.c | 87
>> +++++---------------------------------
>> src/acpi/stao/stao.c | 68 +++--------------------------
>> src/lib/include/fwts_acpi_tables.h | 1 +
>> src/lib/src/fwts_acpi_tables.c | 60 ++++++++++++++++++++++++++
>> 6 files changed, 80 insertions(+), 144 deletions(-)
>>
>> diff --git a/fwts-test/dbg2-0001/dbg2-0002.log
>> b/fwts-test/dbg2-0001/dbg2-0002.log
>> index a55efd3..68be687 100644
>> --- a/fwts-test/dbg2-0001/dbg2-0002.log
>> +++ b/fwts-test/dbg2-0001/dbg2-0002.log
>> @@ -25,7 +25,7 @@ dbg2 FAILED [HIGH]
>> DBG2PortSubTypeReserved: Test 1, DBG2 Info
>> dbg2 Structure Port Subtype is 0x0008 which is a reserved
>> type.
>> dbg2 Namespace String: '\_SB.PCI0.EHC1.HUB0.PRT2'
>> dbg2 FAILED [HIGH] DBG2DeviceNotFound: Test 1, DBG2 Device
>> -dbg2 '\_SB_.PCI0.EHC1.HUB0.PRT2' not found in ACPI object
>> name
>> +dbg2 '\_SB.PCI0.EHC1.HUB0.PRT2' not found in ACPI object name
>> dbg2 space.
>> dbg2 FAILED [HIGH] DBG2TooShort: Test 1, DBG2 table too
>> short,
>> dbg2 expecting 65323 bytes, instead got 107 bytes for a DBG2
>> diff --git a/fwts-test/stao-0001/stao-0002.log
>> b/fwts-test/stao-0001/stao-0002.log
>> index 6756845..78b2eb9 100644
>> --- a/fwts-test/stao-0001/stao-0002.log
>> +++ b/fwts-test/stao-0001/stao-0002.log
>> @@ -7,11 +7,11 @@ stao ACPI String: '\_SB.C0E0'
>> stao ACPI String: '\_SB.C022'
>> stao ACPI String: '\_SB.C02E'
>> stao FAILED [HIGH] STAOAcpiStringNotFound: Test 1, STAO ACPI
>> -stao String '\_SB_.C0E0' not found in ACPI object name space.
>> +stao String '\_SB.C0E0' not found in ACPI object name space.
>> stao FAILED [HIGH] STAOAcpiStringNotFound: Test 1, STAO ACPI
>> -stao String '\_SB_.C022' not found in ACPI object name space.
>> +stao String '\_SB.C022' not found in ACPI object name space.
>> stao FAILED [HIGH] STAOAcpiStringNotFound: Test 1, STAO ACPI
>> -stao String '\_SB_.C02E' not found in ACPI object name space.
>> +stao String '\_SB.C02E' not found in ACPI object name space.
>> stao
>> stao
>> ==========================================================
>> stao 0 passed, 3 failed, 0 warning, 0 aborted, 0 skipped, 0
>> diff --git a/src/acpi/dbg2/dbg2.c b/src/acpi/dbg2/dbg2.c
>> index a542f1b..38ca65e 100644
>> --- a/src/acpi/dbg2/dbg2.c
>> +++ b/src/acpi/dbg2/dbg2.c
>> @@ -89,80 +89,6 @@ static void dbg2_check_namespace_string(
>> *passed = false;
>> }
>>
>> -/*
>> - * dbg2_obj_find()
>> - * find a given object from the given path name
>> - */
>> -static void dbg2_obj_find(
>> - fwts_framework *fw,
>> - char *obj_name,
>> - bool *passed)
>> -{
>> - fwts_list_link *item;
>> - fwts_list *objects;
>> - int i = -1;
>> - char *ptr1;
>> -
>> - if (fwts_acpi_init(fw) != FWTS_OK) {
>> - fwts_log_error(fw, "Cannot initialise ACPI.");
>> - return;
>> - }
>> - if ((objects = fwts_acpi_object_get_names()) == NULL) {
>> - fwts_log_info(fw, "Cannot find any ACPI objects");
>> - fwts_acpi_deinit(fw);
>> - return;
>> - }
>> -
>> - /* Find number of '.' in object path */
>> - for (i = 0, ptr1 = obj_name; *ptr1; ptr1++) {
>> - if (*ptr1 == '.')
>> - i++;
>> - }
>> -
>> - /*
>> - * Allocate buffer big enough to take expanded path
>> - * and pad out any fields that are not 4 chars in size
>> - * between each . separator
>> - *
>> - * Converts \_SB.A.BB.CCC.DDDD.EE to
>> - * \_SB_.A___.BB__.CCC_.DDDD.EE__
>> - */
>> - {
>> - char expanded[1 + (5 * (i + 1))];
>> - char *ptr2 = expanded;
>> -
>> - for (i = -1, ptr1 = obj_name; ; ptr1++) {
>> - if (*ptr1 == '.' || *ptr1 == '\0') {
>> - while (i < 4) {
>> - *ptr2++ = '_';
>> - i++;
>> - }
>> - i = 0;
>> - } else {
>> - i++;
>> - }
>> - *ptr2++ = *ptr1;
>> - if (!*ptr1)
>> - break;
>> - }
>> -
>> - /* Search for object */
>> - fwts_list_foreach(item, objects) {
>> - char *name = fwts_list_data(char*, item);
>> - if (!strcmp(expanded, name))
>> - goto done;
>> - }
>> - /* Not found */
>> - *passed = false;
>> - fwts_failed(fw, LOG_LEVEL_HIGH,
>> - "DBG2DeviceNotFound",
>> - "DBG2 Device '%s' not found in ACPI object name space.",
>> - expanded);
>> - }
>> -done:
>> - fwts_acpi_deinit(fw);
>> -}
>> -
>>
>> /*
>> * DBG2 Table
>> @@ -333,10 +259,17 @@ static int dbg2_test1(fwts_framework *fw)
>> char *str = (char *)table->data + offset +
>> info->namespace_offset;
>> dbg2_check_namespace_string(fw, str,
>> info->namespace_length, &passed);
>> fwts_log_info_verbatim(fw, " Namespace String:
>> '%s'", str);
>> - if (strcmp(str, "."))
>> - dbg2_obj_find(fw, str, &ok);
>> + if (strcmp(str, ".") != 0) {
>> + bool found = fwts_acpi_obj_find(fw, str);
>> + if (!found) {
>> + passed = false;
>> + fwts_failed(fw, LOG_LEVEL_HIGH,
>> + "DBG2DeviceNotFound",
>> + "DBG2 Device '%s' not found in ACPI
>> object name space.",
>> + str);
>> + }
>> + }
>> }
>> - passed &= ok;
>>
>> dbg2_check_offset(fw, table->length, offset +
>> info->oem_data_offset,
>> "DBG2 Info Structure OEM Data Offset", &passed);
>> diff --git a/src/acpi/stao/stao.c b/src/acpi/stao/stao.c
>> index 1461ba6..c529213 100644
>> --- a/src/acpi/stao/stao.c
>> +++ b/src/acpi/stao/stao.c
>> @@ -85,11 +85,7 @@ static int stao_test1(fwts_framework *fw)
>> const fwts_acpi_table_stao *stao = (const fwts_acpi_table_stao
>> *)table->data;
>> bool passed = true;
>> char *ptr, *end;
>> - fwts_list_link *item;
>> - fwts_list *objects;
>> - int i = -1, strings = 0;
>> - char *ptr1, *ptr2;
>> - char *expanded;
>> + int strings = 0;
>>
>> if (stao->header.length > (uint32_t)table->length) {
>> passed = false;
>> @@ -124,82 +120,28 @@ static int stao_test1(fwts_framework *fw)
>> if (!strings)
>> goto done;
>>
>> - if (fwts_acpi_init(fw) != FWTS_OK) {
>> - fwts_log_error(fw, "Cannot initialise ACPI, skipping ACPI
>> string check");
>> - goto done;
>> - }
>> - if ((objects = fwts_acpi_object_get_names()) == NULL) {
>> - fwts_log_info(fw, "Cannot find any ACPI objects");
>> - fwts_acpi_deinit(fw);
>> - goto deinit;
>> - }
>> ptr = (char *)stao->namelist;
>> end = (char *)table->data + table->length;
>>
>> while (ptr < end) {
>> - bool not_found = true;
>> + bool found;
>> size_t len;
>>
>> if (!stao_acpi_string(fw, ptr, end, &passed, &len))
>> break;
>>
>> - /* Find number of '.' in object path */
>> - for (i = 0, ptr1 = ptr; *ptr1; ptr1++) {
>> - if (*ptr1 == '.')
>> - i++;
>> - }
>> -
>> - /*
>> - * Allocate buffer big enough to take expanded path
>> - * and pad out any fields that are not 4 chars in size
>> - * between each . separator
>> - *
>> - * Converts \_SB.A.BB.CCC.DDDD.EE to
>> - * \_SB_.A___.BB__.CCC_.DDDD.EE__
>> - */
>> - expanded = malloc(1 + (5 * (i + 1)));
>> - if (!expanded) {
>> - fwts_log_error(fw, "Cannot allocate temporary ACPI string
>> buffer");
>> - goto deinit;
>> - }
>> -
>> - for (i = -1, ptr1 = ptr, ptr2 = expanded; ; ptr1++) {
>> - if (*ptr1 == '.' || *ptr1 == '\0') {
>> - while (i < 4) {
>> - *ptr2++ = '_';
>> - i++;
>> - }
>> - i = 0;
>> - } else {
>> - i++;
>> - }
>> - *ptr2++ = *ptr1;
>> - if (!*ptr1)
>> - break;
>> - }
>> -
>> - /* Search for object */
>> - fwts_list_foreach(item, objects) {
>> - char *name = fwts_list_data(char *, item);
>> - if (!strcmp(expanded, name)) {
>> - not_found = false;
>> - break;
>> - }
>> - }
>> + found = fwts_acpi_obj_find(fw, ptr);
>>
>> - if (not_found) {
>> + if (!found) {
>> passed = false;
>> fwts_failed(fw, LOG_LEVEL_HIGH,
>> "STAOAcpiStringNotFound",
>> "STAO ACPI String '%s' not found in ACPI object name
>> space.",
>> - expanded);
>> + ptr);
>> }
>> - free(expanded);
>> ptr += len + 1;
>> }
>>
>> -deinit:
>> - fwts_acpi_deinit(fw);
>> done:
>> if (passed)
>> fwts_passed(fw, "No issues found in STAO table.");
>> diff --git a/src/lib/include/fwts_acpi_tables.h
>> b/src/lib/include/fwts_acpi_tables.h
>> index 06ae373..a16a4d3 100644
>> --- a/src/lib/include/fwts_acpi_tables.h
>> +++ b/src/lib/include/fwts_acpi_tables.h
>> @@ -49,6 +49,7 @@ int fwts_acpi_free_tables(void);
>> int fwts_acpi_find_table(fwts_framework *fw, const char *name, const
>> int which, fwts_acpi_table_info **info);
>> int fwts_acpi_find_table_by_addr(fwts_framework *fw, const uint64_t
>> addr, fwts_acpi_table_info **info);
>> int fwts_acpi_get_table(fwts_framework *fw, const int index,
>> fwts_acpi_table_info **info);
>> +bool fwts_acpi_obj_find(fwts_framework *fw, const char *obj_name);
>>
>> fwts_bool fwts_acpi_is_reduced_hardware(const fwts_acpi_table_fadt
>> *fadt);
>>
>> diff --git a/src/lib/src/fwts_acpi_tables.c
>> b/src/lib/src/fwts_acpi_tables.c
>> index 5e0fe1d..30b4060 100644
>> --- a/src/lib/src/fwts_acpi_tables.c
>> +++ b/src/lib/src/fwts_acpi_tables.c
>> @@ -35,6 +35,8 @@
>>
>> #include "fwts.h"
>>
>> +#include "fwts_acpi_object_eval.h"
>> +
>> #if defined(FWTS_HAS_ACPI)
>>
>> #define BIOS_START (0x000e0000) /* Start of BIOS memory */
>> @@ -1282,4 +1284,62 @@ int fwts_acpi_get_table(fwts_framework *fw,
>> const int index, fwts_acpi_table_inf
>> return FWTS_OK;
>> }
>>
>> +/*
>> + * fwts_acpi_obj_find()
>> + * Returns whether obj_name can be found in the ACPI object name space.
>> + */
>> +bool fwts_acpi_obj_find(fwts_framework *fw, const char *obj_name)
>> +{
>> + char expanded[BUFSIZ];
>> + char *c = expanded;
>> + const char *obj_ptr;
>> + int i;
>> + fwts_list_link *item;
>> + fwts_list *objects;
>> + bool found = false;
>> +
>> + if (fwts_acpi_init(fw) != FWTS_OK) {
>> + fwts_log_error(fw, "Cannot initialise ACPI.");
>> + return false;
>> + }
>> + if ((objects = fwts_acpi_object_get_names()) == NULL) {
>> + fwts_log_info(fw, "Cannot find any ACPI objects");
>> + fwts_acpi_deinit(fw);
>> + return false;
>> + }
>> +
>> + memset(expanded, 0, BUFSIZ);
>> +
>> + /*
>> + * Converts \_SB.A.BB.CCC.DDDD.EE to
>> + * \_SB_.A___.BB__.CCC_.DDDD.EE__
>> + */
>> + for (i = -1, obj_ptr = obj_name; ; obj_ptr++) {
>> + if (*obj_ptr == '.' || *obj_ptr == '\0') {
>> + while (i < 4) {
>> + *c++ = '_';
>> + i++;
>> + }
>> + i = 0;
>> + } else {
>> + i++;
>> + }
>> + *c++ = *obj_ptr;
>> + if (!*obj_ptr)
>> + break;
>> + }
>> +
>> + /* Search for object */
>> + fwts_list_foreach(item, objects) {
>> + char *name = fwts_list_data(char*, item);
>> + if (strcmp(expanded, name) == 0) {
>> + found = true;
>> + break;
>> + }
>> + }
>> +
>> + fwts_acpi_deinit(fw);
>> + return found;
>> +}
>> +
>> #endif
>>
>
>
> Acked-by: Alex Hung <alex.hung at canoniocal.com>
>
I'll ack this once it's got through the static analysis; unfortunately
there is a backlog on CoverityScan at the moment.
Colin
More information about the fwts-devel
mailing list