Patchwork [1/2] acpi: acpitables: more ACPI table header sanity checking

login
register
mail settings
Submitter Colin King
Date March 28, 2014, 12:17 p.m.
Message ID <1396009052-3915-2-git-send-email-colin.king@canonical.com>
Download mbox | patch
Permalink /patch/334681/
State Accepted
Headers show

Comments

Colin King - March 28, 2014, 12:17 p.m.
From: Colin Ian King <colin.king@canonical.com>

Some recent firmware contained corrupt XSDT that pointed to garbage
and fwts was not catching this.  Extend the acpitables test to do
some simple sanity checks to see if the table header is a suitable
size and also if some of the human readable fields contain garbage.

The ACPI specification seems vague about the vendor specific fields
that we check here, but I think if they don't contain valid 7 bit
ASCII text then they may be corrupt or the RSDT/XSDT may be pointing
to garbage, so it is worth reporting any weird looking fields.

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 src/acpi/acpitables/acpitables.c | 91 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 91 insertions(+)
Alex Hung - April 7, 2014, 6:37 a.m.
On 03/28/2014 08:17 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Some recent firmware contained corrupt XSDT that pointed to garbage
> and fwts was not catching this.  Extend the acpitables test to do
> some simple sanity checks to see if the table header is a suitable
> size and also if some of the human readable fields contain garbage.
> 
> The ACPI specification seems vague about the vendor specific fields
> that we check here, but I think if they don't contain valid 7 bit
> ASCII text then they may be corrupt or the RSDT/XSDT may be pointing
> to garbage, so it is worth reporting any weird looking fields.
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  src/acpi/acpitables/acpitables.c | 91 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 91 insertions(+)
> 
> diff --git a/src/acpi/acpitables/acpitables.c b/src/acpi/acpitables/acpitables.c
> index 6878734..9e562f6 100644
> --- a/src/acpi/acpitables/acpitables.c
> +++ b/src/acpi/acpitables/acpitables.c
> @@ -505,8 +505,99 @@ static int acpi_table_check_test1(fwts_framework *fw)
>  	return FWTS_OK;
>  }
>  
> +bool acpi_table_check_field(const char *field, const size_t len)
> +{
> +	size_t i;
> +
> +	for (i = 0; i < len; i++)
> +		if (!isascii(field[i]))
> +			return false;
> +
> +	return true;
> +}
> +
> +bool acpi_table_check_field_test(
> +	fwts_framework *fw,
> +	const char *table_name,
> +	const char *field_name,
> +	const char *field,
> +	const size_t len)
> +{
> +	if (!acpi_table_check_field(field, len)) {
> +		fwts_failed(fw, LOG_LEVEL_LOW, "ACPITableHdrInfo",
> +			"ACPI Table %s has non-ASCII characters in "
> +			"header field %s\n", table_name, field_name);
> +		return false;
> +	}
> +	return true;
> +}
> +
> +static int acpi_table_check_test2(fwts_framework *fw)
> +{
> +	int i;
> +	bool checked = false;
> +
> +	for (i = 0; ; i++) {
> +		fwts_acpi_table_info *info;
> +		fwts_acpi_table_header *hdr;
> +		char name[50];
> +		bool passed;
> +
> +		if (fwts_acpi_get_table(fw, i, &info) != FWTS_OK)
> +			break;
> +		if (info == NULL)
> +			continue;
> +
> +		checked = true;
> +		/* RSDP and FACS are special cases, skip these */
> +		if (!strcmp(info->name, "RSDP") ||
> +		    !strcmp(info->name, "FACS"))
> +			continue;
> +
> +		hdr = (fwts_acpi_table_header *)info->data;
> +		if (acpi_table_check_field(hdr->signature, 4)) {
> +			snprintf(name, sizeof(name), "%4.4s", hdr->signature);
> +		} else {
> +			/* Table name not printable, so identify it by the address */
> +			snprintf(name, sizeof(name), "at address 0x%" PRIx64, info->addr);
> +		}
> +
> +		/*
> +		 * Tables shouldn't be short, however, they do have at
> +		 * least 4 bytes with their signature else they would not
> +		 * have been loaded by this stage.
> +		 */
> +		if (hdr->length < sizeof(fwts_acpi_table_header)) {
> +			fwts_failed(fw, LOG_LEVEL_HIGH, "ACPITableHdrShort",
> +				"ACPI Table %s is too short, only %d bytes long. Further "
> +				"header checks will be ommitted.", name, hdr->length);
> +			continue;
> +		}
> +		/* Warn about empty tables */
> +		if (hdr->length == sizeof(fwts_acpi_table_header)) {
> +			fwts_warning(fw,
> +				"ACPI Table %s is empty and just contains a table header. Further "
> +				"header checks will be ommitted.", name);
> +			continue;
> +		}
> +
> +		passed = acpi_table_check_field_test(fw, name, "Signature", hdr->signature, 4) &
> +			 acpi_table_check_field_test(fw, name, "OEM ID", hdr->oem_id, 6) &
> +			 acpi_table_check_field_test(fw, name, "OEM Table ID", hdr->oem_tbl_id, 8) &
> +			acpi_table_check_field_test(fw, name, "OEM Creator ID", hdr->creator_id, 4);
> +		if (passed)
> +			fwts_passed(fw, "Table %s has valid signature and ID strings.", name);
> +
> +	}
> +	if (!checked)
> +		fwts_aborted(fw, "Cannot find any ACPI tables.");
> +
> +	return FWTS_OK;
> +}
> +
>  static fwts_framework_minor_test acpi_table_check_tests[] = {
>  	{ acpi_table_check_test1, "Test ACPI tables." },
> +	{ acpi_table_check_test2, "Test ACPI headers." },
>  	{ NULL, NULL }
>  };
>  
> 


Acked-by: Alex Hung <alex.hung@canonical.com>
Ivan Hu - April 17, 2014, 3:49 a.m.
On 03/28/2014 08:17 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Some recent firmware contained corrupt XSDT that pointed to garbage
> and fwts was not catching this.  Extend the acpitables test to do
> some simple sanity checks to see if the table header is a suitable
> size and also if some of the human readable fields contain garbage.
>
> The ACPI specification seems vague about the vendor specific fields
> that we check here, but I think if they don't contain valid 7 bit
> ASCII text then they may be corrupt or the RSDT/XSDT may be pointing
> to garbage, so it is worth reporting any weird looking fields.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/acpi/acpitables/acpitables.c | 91 ++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 91 insertions(+)
>
> diff --git a/src/acpi/acpitables/acpitables.c b/src/acpi/acpitables/acpitables.c
> index 6878734..9e562f6 100644
> --- a/src/acpi/acpitables/acpitables.c
> +++ b/src/acpi/acpitables/acpitables.c
> @@ -505,8 +505,99 @@ static int acpi_table_check_test1(fwts_framework *fw)
>   	return FWTS_OK;
>   }
>
> +bool acpi_table_check_field(const char *field, const size_t len)
> +{
> +	size_t i;
> +
> +	for (i = 0; i < len; i++)
> +		if (!isascii(field[i]))
> +			return false;
> +
> +	return true;
> +}
> +
> +bool acpi_table_check_field_test(
> +	fwts_framework *fw,
> +	const char *table_name,
> +	const char *field_name,
> +	const char *field,
> +	const size_t len)
> +{
> +	if (!acpi_table_check_field(field, len)) {
> +		fwts_failed(fw, LOG_LEVEL_LOW, "ACPITableHdrInfo",
> +			"ACPI Table %s has non-ASCII characters in "
> +			"header field %s\n", table_name, field_name);
> +		return false;
> +	}
> +	return true;
> +}
> +
> +static int acpi_table_check_test2(fwts_framework *fw)
> +{
> +	int i;
> +	bool checked = false;
> +
> +	for (i = 0; ; i++) {
> +		fwts_acpi_table_info *info;
> +		fwts_acpi_table_header *hdr;
> +		char name[50];
> +		bool passed;
> +
> +		if (fwts_acpi_get_table(fw, i, &info) != FWTS_OK)
> +			break;
> +		if (info == NULL)
> +			continue;
> +
> +		checked = true;
> +		/* RSDP and FACS are special cases, skip these */
> +		if (!strcmp(info->name, "RSDP") ||
> +		    !strcmp(info->name, "FACS"))
> +			continue;
> +
> +		hdr = (fwts_acpi_table_header *)info->data;
> +		if (acpi_table_check_field(hdr->signature, 4)) {
> +			snprintf(name, sizeof(name), "%4.4s", hdr->signature);
> +		} else {
> +			/* Table name not printable, so identify it by the address */
> +			snprintf(name, sizeof(name), "at address 0x%" PRIx64, info->addr);
> +		}
> +
> +		/*
> +		 * Tables shouldn't be short, however, they do have at
> +		 * least 4 bytes with their signature else they would not
> +		 * have been loaded by this stage.
> +		 */
> +		if (hdr->length < sizeof(fwts_acpi_table_header)) {
> +			fwts_failed(fw, LOG_LEVEL_HIGH, "ACPITableHdrShort",
> +				"ACPI Table %s is too short, only %d bytes long. Further "
> +				"header checks will be ommitted.", name, hdr->length);
> +			continue;
> +		}
> +		/* Warn about empty tables */
> +		if (hdr->length == sizeof(fwts_acpi_table_header)) {
> +			fwts_warning(fw,
> +				"ACPI Table %s is empty and just contains a table header. Further "
> +				"header checks will be ommitted.", name);
> +			continue;
> +		}
> +
> +		passed = acpi_table_check_field_test(fw, name, "Signature", hdr->signature, 4) &
> +			 acpi_table_check_field_test(fw, name, "OEM ID", hdr->oem_id, 6) &
> +			 acpi_table_check_field_test(fw, name, "OEM Table ID", hdr->oem_tbl_id, 8) &
> +			acpi_table_check_field_test(fw, name, "OEM Creator ID", hdr->creator_id, 4);
> +		if (passed)
> +			fwts_passed(fw, "Table %s has valid signature and ID strings.", name);
> +
> +	}
> +	if (!checked)
> +		fwts_aborted(fw, "Cannot find any ACPI tables.");
> +
> +	return FWTS_OK;
> +}
> +
>   static fwts_framework_minor_test acpi_table_check_tests[] = {
>   	{ acpi_table_check_test1, "Test ACPI tables." },
> +	{ acpi_table_check_test2, "Test ACPI headers." },
>   	{ NULL, NULL }
>   };
>
>


Acked-by: Ivan Hu <ivan.hu@canonical.com>

Patch

diff --git a/src/acpi/acpitables/acpitables.c b/src/acpi/acpitables/acpitables.c
index 6878734..9e562f6 100644
--- a/src/acpi/acpitables/acpitables.c
+++ b/src/acpi/acpitables/acpitables.c
@@ -505,8 +505,99 @@  static int acpi_table_check_test1(fwts_framework *fw)
 	return FWTS_OK;
 }
 
+bool acpi_table_check_field(const char *field, const size_t len)
+{
+	size_t i;
+
+	for (i = 0; i < len; i++)
+		if (!isascii(field[i]))
+			return false;
+
+	return true;
+}
+
+bool acpi_table_check_field_test(
+	fwts_framework *fw,
+	const char *table_name,
+	const char *field_name,
+	const char *field,
+	const size_t len)
+{
+	if (!acpi_table_check_field(field, len)) {
+		fwts_failed(fw, LOG_LEVEL_LOW, "ACPITableHdrInfo",
+			"ACPI Table %s has non-ASCII characters in "
+			"header field %s\n", table_name, field_name);
+		return false;
+	}
+	return true;
+}
+
+static int acpi_table_check_test2(fwts_framework *fw)
+{
+	int i;
+	bool checked = false;
+
+	for (i = 0; ; i++) {
+		fwts_acpi_table_info *info;
+		fwts_acpi_table_header *hdr;
+		char name[50];
+		bool passed;
+
+		if (fwts_acpi_get_table(fw, i, &info) != FWTS_OK)
+			break;
+		if (info == NULL)
+			continue;
+
+		checked = true;
+		/* RSDP and FACS are special cases, skip these */
+		if (!strcmp(info->name, "RSDP") ||
+		    !strcmp(info->name, "FACS"))
+			continue;
+
+		hdr = (fwts_acpi_table_header *)info->data;
+		if (acpi_table_check_field(hdr->signature, 4)) {
+			snprintf(name, sizeof(name), "%4.4s", hdr->signature);
+		} else {
+			/* Table name not printable, so identify it by the address */
+			snprintf(name, sizeof(name), "at address 0x%" PRIx64, info->addr);
+		}
+
+		/*
+		 * Tables shouldn't be short, however, they do have at
+		 * least 4 bytes with their signature else they would not
+		 * have been loaded by this stage.
+		 */
+		if (hdr->length < sizeof(fwts_acpi_table_header)) {
+			fwts_failed(fw, LOG_LEVEL_HIGH, "ACPITableHdrShort",
+				"ACPI Table %s is too short, only %d bytes long. Further "
+				"header checks will be ommitted.", name, hdr->length);
+			continue;
+		}
+		/* Warn about empty tables */
+		if (hdr->length == sizeof(fwts_acpi_table_header)) {
+			fwts_warning(fw,
+				"ACPI Table %s is empty and just contains a table header. Further "
+				"header checks will be ommitted.", name);
+			continue;
+		}
+
+		passed = acpi_table_check_field_test(fw, name, "Signature", hdr->signature, 4) &
+			 acpi_table_check_field_test(fw, name, "OEM ID", hdr->oem_id, 6) &
+			 acpi_table_check_field_test(fw, name, "OEM Table ID", hdr->oem_tbl_id, 8) &
+			acpi_table_check_field_test(fw, name, "OEM Creator ID", hdr->creator_id, 4);
+		if (passed)
+			fwts_passed(fw, "Table %s has valid signature and ID strings.", name);
+
+	}
+	if (!checked)
+		fwts_aborted(fw, "Cannot find any ACPI tables.");
+
+	return FWTS_OK;
+}
+
 static fwts_framework_minor_test acpi_table_check_tests[] = {
 	{ acpi_table_check_test1, "Test ACPI tables." },
+	{ acpi_table_check_test2, "Test ACPI headers." },
 	{ NULL, NULL }
 };