ACK: [PATCH 2/2] acpi: dbg2: fix segfault and refactor dbg2_obj_find
Alex Hung
alex.hung at canonical.com
Mon Oct 31 23:45:26 UTC 2016
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>
More information about the fwts-devel
mailing list