Patchwork bios: smbios: add more sanity checks to SMBIOS information

login
register
mail settings
Submitter Colin King
Date June 15, 2013, 10:18 a.m.
Message ID <1371291493-28262-1-git-send-email-colin.king@canonical.com>
Download mbox | patch
Permalink /patch/251619/
State Accepted
Headers show

Comments

Colin King - June 15, 2013, 10:18 a.m.
From: Colin Ian King <colin.king@canonical.com>

Add some more sanity checks to SMBIOS information to see if the
DMI table it points to is OK and if the SMBIOS DMI table length
looks valid.

Also add pass information on some of the existing SMBIOS sanity checks.

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 src/bios/smbios/smbios.c | 114 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 102 insertions(+), 12 deletions(-)
Alex Hung - June 17, 2013, 3:45 a.m.
On 06/15/2013 06:18 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Add some more sanity checks to SMBIOS information to see if the
> DMI table it points to is OK and if the SMBIOS DMI table length
> looks valid.
>
> Also add pass information on some of the existing SMBIOS sanity checks.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/bios/smbios/smbios.c | 114 ++++++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 102 insertions(+), 12 deletions(-)
>
> diff --git a/src/bios/smbios/smbios.c b/src/bios/smbios/smbios.c
> index 57c8999..022447a 100644
> --- a/src/bios/smbios/smbios.c
> +++ b/src/bios/smbios/smbios.c
> @@ -16,12 +16,15 @@
>    * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
>    *
>    */
> +#include <stdint.h>
> +#include <inttypes.h>
> +
>   #include "fwts.h"
>
>   #ifdef FWTS_ARCH_INTEL
>
>   static void smbios_dump_entry(fwts_framework *fw, fwts_smbios_entry *entry, fwts_smbios_type type)
> -{	
> +{
>   	if (type == FWTS_SMBIOS) {
>   		fwts_log_info_verbatum(fw, "SMBIOS Entry Point Structure:");
>   		fwts_log_info_verbatum(fw, "  Anchor String          : %4.4s", entry->signature);
> @@ -31,7 +34,7 @@ static void smbios_dump_entry(fwts_framework *fw, fwts_smbios_entry *entry, fwts
>   		fwts_log_info_verbatum(fw, "  Minor Version          : 0x%2.2x", entry->minor_version);
>   		fwts_log_info_verbatum(fw, "  Maximum Struct Size    : 0x%2.2x", entry->max_struct_size);
>   		fwts_log_info_verbatum(fw, "  Entry Point Revision   : 0x%2.2x", entry->revision);
> -		fwts_log_info_verbatum(fw, "  Formatted Area         : 0x%2.2x 0x%2.2x 0x%2.2x 0x%2.2x 0x%2.2x",
> +		fwts_log_info_verbatum(fw, "  Formatted Area         : 0x%2.2x 0x%2.2x 0x%2.2x 0x%2.2x 0x%2.2x",
>   			entry->formatted_area[0], entry->formatted_area[1],
>   			entry->formatted_area[2], entry->formatted_area[3],
>   			entry->formatted_area[4]);
> @@ -50,6 +53,77 @@ static void smbios_dump_entry(fwts_framework *fw, fwts_smbios_entry *entry, fwts
>   		fwts_log_info_verbatum(fw, "    BCD Revision 00 indicates compliance with specification stated in Major/Minor Version.");
>   }
>
> +static int smbios_dmi_sane(fwts_framework *fw, fwts_smbios_entry *entry)
> +{
> +	uint8_t	*table, *ptr;
> +	uint8_t dmi_entry_length;
> +	uint8_t dmi_entry_type = 0;
> +	uint16_t i = 0;
> +	uint16_t table_length = entry->struct_table_length;
> +	int ret = FWTS_OK;
> +
> +	ptr = table = fwts_mmap((off_t)entry->struct_table_address,
> +			  (size_t)table_length);
> +	if (table == FWTS_MAP_FAILED) {
> +		fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +			"SMBIOSTableAddressNotMapped",
> +			"Cannot mmap SMBIOS tables from "
> +			"%8.8" PRIx32 "..%8.8" PRIx32 ".",
> +			entry->struct_table_address,
> +			entry->struct_table_address + table_length);
> +			return FWTS_ERROR;
> +	}
> +
> +	for (i = 0; i < entry->number_smbios_structures; i++) {
> +		if (ptr > table + table_length) {
> +			fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +				"SMBIOSTableLengthTooSmall",
> +				"The size indicated by the SMBIOS table length is "
> +				"smaller than the DMI data.");
> +			ret = FWTS_ERROR;
> +			break;
> +		}
> +
> +		dmi_entry_type = ptr[0];
> +		dmi_entry_length = ptr[1];
> +
> +		if (dmi_entry_length < 4) {
> +			fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +				"SMBIOSIllegalTableEntry",
> +				"The size of a DMI entry %" PRIu16 " is illegal, "
> +				"DMI data is either wrong or the SMBIOS Table "
> +				"Pointer is pointing to the wrong memory region.", i);
> +			ret = FWTS_ERROR;
> +			break;
> +		}
> +		ptr += dmi_entry_length;
> +
> +		/* Scan for end of DMI entry, must be 2 zero bytes */
> +		while (((ptr - table + 1) < table_length) &&
> +		       ((ptr[0] != 0) || (ptr[1] != 0)))
> +				ptr++;
> +		/* Skip over the two zero bytes */
> +		ptr += 2;
> +	}
> +
> +	/* We found DMI end of table, are number of entries correct? */
> +	if ((dmi_entry_type == 127) && (i != entry->number_smbios_structures)) {
> +		fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +			"SMBIOSNumberOfStructures",
> +			"The end of DMI table marker structure was found "
> +			"but only %d DMI structures were found. The SMBIOS "
> +			"entry table reported that there were %" PRIu16
> +			" DMI structures in the DMI table.",
> +			i, entry->number_smbios_structures);
> +		/* And table length can't be correct either */
> +		ret = FWTS_ERROR;
> +	}
> +
> +	(void)fwts_munmap(table, (size_t)entry->struct_table_length);
> +
> +	return ret;
> +}
> +
>   static int smbios_test1(fwts_framework *fw)
>   {
>   	void *addr = 0;
> @@ -57,7 +131,7 @@ static int smbios_test1(fwts_framework *fw)
>   	fwts_smbios_type  type;
>   	uint16_t	  version;
>   	uint8_t checksum;
> -	static char warning[] =
> +	static char warning[] =
>   		"This field is not checked by the kernel, and so will not affect the system, "
>   		"however it should be fixed to conform to the latest version of the "
>   		"System Management BIOS (SMBIOS) Reference Specification. ";
> @@ -75,39 +149,48 @@ static int smbios_test1(fwts_framework *fw)
>
>   	fwts_passed(fw, "Found SMBIOS Table Entry Point at %p", addr);
>   	smbios_dump_entry(fw, &entry, type);
> -		
> +	fwts_log_nl(fw);
> +
>   	if (type == FWTS_SMBIOS) {
>   		checksum = fwts_checksum((uint8_t*)&entry, sizeof(fwts_smbios_entry));
> -		if (checksum != 0) {
> +		if (checksum != 0)
>   			fwts_failed(fw, LOG_LEVEL_HIGH,
>   				"SMBIOSBadChecksum",
>   				"SMBIOS Table Entry Point Checksum is 0x%2.2x, should be 0x%2.2x",
>   					entry.checksum, (uint8_t)(entry.checksum - checksum));
> -		}
> +		else
> +			fwts_passed(fw, "SMBIOS Table Entry Point Checksum is valid.");
> +
>   		if (entry.length != 0x1f) {
>   			fwts_failed(fw, LOG_LEVEL_LOW,
>   				"SMBIOSBadEntryLength",
>   				"SMBIOS Table Entry Point Length is 0x%2.2x, should be 0x1f", entry.length);
>   			fwts_advice(fw, "%s Note that version 2.1 of the specification incorrectly stated that the "
>   				"Entry Point Length should be 0x1e when it should be 0x1f.", warning);
> -		}
> +		} else
> +			fwts_passed(fw, "SMBIOS Table Entry Point Length is valid.");
>   	}
> +
>   	if (memcmp(entry.anchor_string, "_DMI_", 5) != 0) {
>   		fwts_failed(fw, LOG_LEVEL_HIGH,
> -			"SMBIOSBadIntermediateAnchor",
> -			"SMBIOS Table Entry Intermediate Anchor String was '%5.5s' and should be '_DMI_'.",
> +			"SMBIOSBadIntermediateAnchor",
> +			"SMBIOS Table Entry Intermediate Anchor String was '%5.5s' and should be '_DMI_'.",
>   			entry.anchor_string);
>   		fwts_advice(fw, "%s", warning);
> -	}
> +	} else
> +		fwts_passed(fw, "SMBIOS Table Entry Intermediate Anchor String _DMI_ is valid.");
> +
>   	/* Intermediate checksum for legacy DMI */
>   	checksum = fwts_checksum(((uint8_t*)&entry)+16, 15);
> -	if (checksum != 0) {
> +	if (checksum != 0)
>   		fwts_failed(fw, LOG_LEVEL_HIGH,
>   			"SMBIOSBadChecksum",
>   			"SMBIOS Table Entry Point Intermediate Checksum is 0x%2.2x, should be 0x%2.2x",
>   				entry.intermediate_checksum,
>   				(uint8_t)(entry.intermediate_checksum - checksum));
> -	}
> +	else
> +		fwts_passed(fw, "SMBIOS Table Entry Point Intermediate Checksum is valid.");
> +
>   	if ((entry.struct_table_length > 0) && (entry.struct_table_address == 0)) {
>   		fwts_failed(fw, LOG_LEVEL_HIGH,
>   			"SMBIOSBadTableAddress",
> @@ -115,6 +198,13 @@ static int smbios_test1(fwts_framework *fw)
>   		fwts_advice(fw,
>   			"The address of the SMBIOS Structure Tables must be defined if the "
>   			"length of these tables is defined.");
> +	} else {
> +		/*
> +		 * Now does the DMI table look sane? If not,
> +		 * the SMBIOS Structure Table could be bad
> +		 */
> +		if (smbios_dmi_sane(fw, &entry) == FWTS_OK)
> +			fwts_passed(fw, "SMBIOS Table Entry Structure Table Address and Length looks valid.");
>   	}
>
>   	return FWTS_OK;
>

Acked-by: Alex Hung <alex.hung@canonical.com>
Ivan Hu - June 20, 2013, 7:36 a.m.
On 06/15/2013 06:18 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Add some more sanity checks to SMBIOS information to see if the
> DMI table it points to is OK and if the SMBIOS DMI table length
> looks valid.
>
> Also add pass information on some of the existing SMBIOS sanity checks.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/bios/smbios/smbios.c | 114 ++++++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 102 insertions(+), 12 deletions(-)
>
> diff --git a/src/bios/smbios/smbios.c b/src/bios/smbios/smbios.c
> index 57c8999..022447a 100644
> --- a/src/bios/smbios/smbios.c
> +++ b/src/bios/smbios/smbios.c
> @@ -16,12 +16,15 @@
>    * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
>    *
>    */
> +#include <stdint.h>
> +#include <inttypes.h>
> +
>   #include "fwts.h"
>
>   #ifdef FWTS_ARCH_INTEL
>
>   static void smbios_dump_entry(fwts_framework *fw, fwts_smbios_entry *entry, fwts_smbios_type type)
> -{	
> +{
>   	if (type == FWTS_SMBIOS) {
>   		fwts_log_info_verbatum(fw, "SMBIOS Entry Point Structure:");
>   		fwts_log_info_verbatum(fw, "  Anchor String          : %4.4s", entry->signature);
> @@ -31,7 +34,7 @@ static void smbios_dump_entry(fwts_framework *fw, fwts_smbios_entry *entry, fwts
>   		fwts_log_info_verbatum(fw, "  Minor Version          : 0x%2.2x", entry->minor_version);
>   		fwts_log_info_verbatum(fw, "  Maximum Struct Size    : 0x%2.2x", entry->max_struct_size);
>   		fwts_log_info_verbatum(fw, "  Entry Point Revision   : 0x%2.2x", entry->revision);
> -		fwts_log_info_verbatum(fw, "  Formatted Area         : 0x%2.2x 0x%2.2x 0x%2.2x 0x%2.2x 0x%2.2x",
> +		fwts_log_info_verbatum(fw, "  Formatted Area         : 0x%2.2x 0x%2.2x 0x%2.2x 0x%2.2x 0x%2.2x",
>   			entry->formatted_area[0], entry->formatted_area[1],
>   			entry->formatted_area[2], entry->formatted_area[3],
>   			entry->formatted_area[4]);
> @@ -50,6 +53,77 @@ static void smbios_dump_entry(fwts_framework *fw, fwts_smbios_entry *entry, fwts
>   		fwts_log_info_verbatum(fw, "    BCD Revision 00 indicates compliance with specification stated in Major/Minor Version.");
>   }
>
> +static int smbios_dmi_sane(fwts_framework *fw, fwts_smbios_entry *entry)
> +{
> +	uint8_t	*table, *ptr;
> +	uint8_t dmi_entry_length;
> +	uint8_t dmi_entry_type = 0;
> +	uint16_t i = 0;
> +	uint16_t table_length = entry->struct_table_length;
> +	int ret = FWTS_OK;
> +
> +	ptr = table = fwts_mmap((off_t)entry->struct_table_address,
> +			  (size_t)table_length);
> +	if (table == FWTS_MAP_FAILED) {
> +		fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +			"SMBIOSTableAddressNotMapped",
> +			"Cannot mmap SMBIOS tables from "
> +			"%8.8" PRIx32 "..%8.8" PRIx32 ".",
> +			entry->struct_table_address,
> +			entry->struct_table_address + table_length);
> +			return FWTS_ERROR;
> +	}
> +
> +	for (i = 0; i < entry->number_smbios_structures; i++) {
> +		if (ptr > table + table_length) {
> +			fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +				"SMBIOSTableLengthTooSmall",
> +				"The size indicated by the SMBIOS table length is "
> +				"smaller than the DMI data.");
> +			ret = FWTS_ERROR;
> +			break;
> +		}
> +
> +		dmi_entry_type = ptr[0];
> +		dmi_entry_length = ptr[1];
> +
> +		if (dmi_entry_length < 4) {
> +			fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +				"SMBIOSIllegalTableEntry",
> +				"The size of a DMI entry %" PRIu16 " is illegal, "
> +				"DMI data is either wrong or the SMBIOS Table "
> +				"Pointer is pointing to the wrong memory region.", i);
> +			ret = FWTS_ERROR;
> +			break;
> +		}
> +		ptr += dmi_entry_length;
> +
> +		/* Scan for end of DMI entry, must be 2 zero bytes */
> +		while (((ptr - table + 1) < table_length) &&
> +		       ((ptr[0] != 0) || (ptr[1] != 0)))
> +				ptr++;
> +		/* Skip over the two zero bytes */
> +		ptr += 2;
> +	}
> +
> +	/* We found DMI end of table, are number of entries correct? */
> +	if ((dmi_entry_type == 127) && (i != entry->number_smbios_structures)) {
> +		fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +			"SMBIOSNumberOfStructures",
> +			"The end of DMI table marker structure was found "
> +			"but only %d DMI structures were found. The SMBIOS "
> +			"entry table reported that there were %" PRIu16
> +			" DMI structures in the DMI table.",
> +			i, entry->number_smbios_structures);
> +		/* And table length can't be correct either */
> +		ret = FWTS_ERROR;
> +	}
> +
> +	(void)fwts_munmap(table, (size_t)entry->struct_table_length);
> +
> +	return ret;
> +}
> +
>   static int smbios_test1(fwts_framework *fw)
>   {
>   	void *addr = 0;
> @@ -57,7 +131,7 @@ static int smbios_test1(fwts_framework *fw)
>   	fwts_smbios_type  type;
>   	uint16_t	  version;
>   	uint8_t checksum;
> -	static char warning[] =
> +	static char warning[] =
>   		"This field is not checked by the kernel, and so will not affect the system, "
>   		"however it should be fixed to conform to the latest version of the "
>   		"System Management BIOS (SMBIOS) Reference Specification. ";
> @@ -75,39 +149,48 @@ static int smbios_test1(fwts_framework *fw)
>
>   	fwts_passed(fw, "Found SMBIOS Table Entry Point at %p", addr);
>   	smbios_dump_entry(fw, &entry, type);
> -		
> +	fwts_log_nl(fw);
> +
>   	if (type == FWTS_SMBIOS) {
>   		checksum = fwts_checksum((uint8_t*)&entry, sizeof(fwts_smbios_entry));
> -		if (checksum != 0) {
> +		if (checksum != 0)
>   			fwts_failed(fw, LOG_LEVEL_HIGH,
>   				"SMBIOSBadChecksum",
>   				"SMBIOS Table Entry Point Checksum is 0x%2.2x, should be 0x%2.2x",
>   					entry.checksum, (uint8_t)(entry.checksum - checksum));
> -		}
> +		else
> +			fwts_passed(fw, "SMBIOS Table Entry Point Checksum is valid.");
> +
>   		if (entry.length != 0x1f) {
>   			fwts_failed(fw, LOG_LEVEL_LOW,
>   				"SMBIOSBadEntryLength",
>   				"SMBIOS Table Entry Point Length is 0x%2.2x, should be 0x1f", entry.length);
>   			fwts_advice(fw, "%s Note that version 2.1 of the specification incorrectly stated that the "
>   				"Entry Point Length should be 0x1e when it should be 0x1f.", warning);
> -		}
> +		} else
> +			fwts_passed(fw, "SMBIOS Table Entry Point Length is valid.");
>   	}
> +
>   	if (memcmp(entry.anchor_string, "_DMI_", 5) != 0) {
>   		fwts_failed(fw, LOG_LEVEL_HIGH,
> -			"SMBIOSBadIntermediateAnchor",
> -			"SMBIOS Table Entry Intermediate Anchor String was '%5.5s' and should be '_DMI_'.",
> +			"SMBIOSBadIntermediateAnchor",
> +			"SMBIOS Table Entry Intermediate Anchor String was '%5.5s' and should be '_DMI_'.",
>   			entry.anchor_string);
>   		fwts_advice(fw, "%s", warning);
> -	}
> +	} else
> +		fwts_passed(fw, "SMBIOS Table Entry Intermediate Anchor String _DMI_ is valid.");
> +
>   	/* Intermediate checksum for legacy DMI */
>   	checksum = fwts_checksum(((uint8_t*)&entry)+16, 15);
> -	if (checksum != 0) {
> +	if (checksum != 0)
>   		fwts_failed(fw, LOG_LEVEL_HIGH,
>   			"SMBIOSBadChecksum",
>   			"SMBIOS Table Entry Point Intermediate Checksum is 0x%2.2x, should be 0x%2.2x",
>   				entry.intermediate_checksum,
>   				(uint8_t)(entry.intermediate_checksum - checksum));
> -	}
> +	else
> +		fwts_passed(fw, "SMBIOS Table Entry Point Intermediate Checksum is valid.");
> +
>   	if ((entry.struct_table_length > 0) && (entry.struct_table_address == 0)) {
>   		fwts_failed(fw, LOG_LEVEL_HIGH,
>   			"SMBIOSBadTableAddress",
> @@ -115,6 +198,13 @@ static int smbios_test1(fwts_framework *fw)
>   		fwts_advice(fw,
>   			"The address of the SMBIOS Structure Tables must be defined if the "
>   			"length of these tables is defined.");
> +	} else {
> +		/*
> +		 * Now does the DMI table look sane? If not,
> +		 * the SMBIOS Structure Table could be bad
> +		 */
> +		if (smbios_dmi_sane(fw, &entry) == FWTS_OK)
> +			fwts_passed(fw, "SMBIOS Table Entry Structure Table Address and Length looks valid.");
>   	}
>
>   	return FWTS_OK;
>


Acked-by: Ivan Hu <ivan.hu@canonical.com>
Keng-Yu Lin - June 20, 2013, 8:15 a.m.
Hi Colin,
  This patch has some rejected hunks seemingly due to some tidy-up of
the leading tabs.

  if there is any missing patch from you? (or anyone I overlooked?)

On Sat, Jun 15, 2013 at 6:18 PM, Colin King <colin.king@canonical.com> wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Add some more sanity checks to SMBIOS information to see if the
> DMI table it points to is OK and if the SMBIOS DMI table length
> looks valid.
>
> Also add pass information on some of the existing SMBIOS sanity checks.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  src/bios/smbios/smbios.c | 114 ++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 102 insertions(+), 12 deletions(-)
>
> diff --git a/src/bios/smbios/smbios.c b/src/bios/smbios/smbios.c
> index 57c8999..022447a 100644
> --- a/src/bios/smbios/smbios.c
> +++ b/src/bios/smbios/smbios.c
> @@ -16,12 +16,15 @@
>   * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
>   *
>   */
> +#include <stdint.h>
> +#include <inttypes.h>
> +
>  #include "fwts.h"
>
>  #ifdef FWTS_ARCH_INTEL
>
>  static void smbios_dump_entry(fwts_framework *fw, fwts_smbios_entry *entry, fwts_smbios_type type)
> -{
> +{
>         if (type == FWTS_SMBIOS) {
>                 fwts_log_info_verbatum(fw, "SMBIOS Entry Point Structure:");
>                 fwts_log_info_verbatum(fw, "  Anchor String          : %4.4s", entry->signature);
> @@ -31,7 +34,7 @@ static void smbios_dump_entry(fwts_framework *fw, fwts_smbios_entry *entry, fwts
>                 fwts_log_info_verbatum(fw, "  Minor Version          : 0x%2.2x", entry->minor_version);
>                 fwts_log_info_verbatum(fw, "  Maximum Struct Size    : 0x%2.2x", entry->max_struct_size);
>                 fwts_log_info_verbatum(fw, "  Entry Point Revision   : 0x%2.2x", entry->revision);
> -               fwts_log_info_verbatum(fw, "  Formatted Area         : 0x%2.2x 0x%2.2x 0x%2.2x 0x%2.2x 0x%2.2x",
> +               fwts_log_info_verbatum(fw, "  Formatted Area         : 0x%2.2x 0x%2.2x 0x%2.2x 0x%2.2x 0x%2.2x",
>                         entry->formatted_area[0], entry->formatted_area[1],
>                         entry->formatted_area[2], entry->formatted_area[3],
>                         entry->formatted_area[4]);
> @@ -50,6 +53,77 @@ static void smbios_dump_entry(fwts_framework *fw, fwts_smbios_entry *entry, fwts
>                 fwts_log_info_verbatum(fw, "    BCD Revision 00 indicates compliance with specification stated in Major/Minor Version.");
>  }
>
> +static int smbios_dmi_sane(fwts_framework *fw, fwts_smbios_entry *entry)
> +{
> +       uint8_t *table, *ptr;
> +       uint8_t dmi_entry_length;
> +       uint8_t dmi_entry_type = 0;
> +       uint16_t i = 0;
> +       uint16_t table_length = entry->struct_table_length;
> +       int ret = FWTS_OK;
> +
> +       ptr = table = fwts_mmap((off_t)entry->struct_table_address,
> +                         (size_t)table_length);
> +       if (table == FWTS_MAP_FAILED) {
> +               fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +                       "SMBIOSTableAddressNotMapped",
> +                       "Cannot mmap SMBIOS tables from "
> +                       "%8.8" PRIx32 "..%8.8" PRIx32 ".",
> +                       entry->struct_table_address,
> +                       entry->struct_table_address + table_length);
> +                       return FWTS_ERROR;
> +       }
> +
> +       for (i = 0; i < entry->number_smbios_structures; i++) {
> +               if (ptr > table + table_length) {
> +                       fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +                               "SMBIOSTableLengthTooSmall",
> +                               "The size indicated by the SMBIOS table length is "
> +                               "smaller than the DMI data.");
> +                       ret = FWTS_ERROR;
> +                       break;
> +               }
> +
> +               dmi_entry_type = ptr[0];
> +               dmi_entry_length = ptr[1];
> +
> +               if (dmi_entry_length < 4) {
> +                       fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +                               "SMBIOSIllegalTableEntry",
> +                               "The size of a DMI entry %" PRIu16 " is illegal, "
> +                               "DMI data is either wrong or the SMBIOS Table "
> +                               "Pointer is pointing to the wrong memory region.", i);
> +                       ret = FWTS_ERROR;
> +                       break;
> +               }
> +               ptr += dmi_entry_length;
> +
> +               /* Scan for end of DMI entry, must be 2 zero bytes */
> +               while (((ptr - table + 1) < table_length) &&
> +                      ((ptr[0] != 0) || (ptr[1] != 0)))
> +                               ptr++;
> +               /* Skip over the two zero bytes */
> +               ptr += 2;
> +       }
> +
> +       /* We found DMI end of table, are number of entries correct? */
> +       if ((dmi_entry_type == 127) && (i != entry->number_smbios_structures)) {
> +               fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +                       "SMBIOSNumberOfStructures",
> +                       "The end of DMI table marker structure was found "
> +                       "but only %d DMI structures were found. The SMBIOS "
> +                       "entry table reported that there were %" PRIu16
> +                       " DMI structures in the DMI table.",
> +                       i, entry->number_smbios_structures);
> +               /* And table length can't be correct either */
> +               ret = FWTS_ERROR;
> +       }
> +
> +       (void)fwts_munmap(table, (size_t)entry->struct_table_length);
> +
> +       return ret;
> +}
> +
>  static int smbios_test1(fwts_framework *fw)
>  {
>         void *addr = 0;
> @@ -57,7 +131,7 @@ static int smbios_test1(fwts_framework *fw)
>         fwts_smbios_type  type;
>         uint16_t          version;
>         uint8_t checksum;
> -       static char warning[] =
> +       static char warning[] =
>                 "This field is not checked by the kernel, and so will not affect the system, "
>                 "however it should be fixed to conform to the latest version of the "
>                 "System Management BIOS (SMBIOS) Reference Specification. ";
> @@ -75,39 +149,48 @@ static int smbios_test1(fwts_framework *fw)
>
>         fwts_passed(fw, "Found SMBIOS Table Entry Point at %p", addr);
>         smbios_dump_entry(fw, &entry, type);
> -
> +       fwts_log_nl(fw);
> +
>         if (type == FWTS_SMBIOS) {
>                 checksum = fwts_checksum((uint8_t*)&entry, sizeof(fwts_smbios_entry));
> -               if (checksum != 0) {
> +               if (checksum != 0)
>                         fwts_failed(fw, LOG_LEVEL_HIGH,
>                                 "SMBIOSBadChecksum",
>                                 "SMBIOS Table Entry Point Checksum is 0x%2.2x, should be 0x%2.2x",
>                                         entry.checksum, (uint8_t)(entry.checksum - checksum));
> -               }
> +               else
> +                       fwts_passed(fw, "SMBIOS Table Entry Point Checksum is valid.");
> +
>                 if (entry.length != 0x1f) {
>                         fwts_failed(fw, LOG_LEVEL_LOW,
>                                 "SMBIOSBadEntryLength",
>                                 "SMBIOS Table Entry Point Length is 0x%2.2x, should be 0x1f", entry.length);
>                         fwts_advice(fw, "%s Note that version 2.1 of the specification incorrectly stated that the "
>                                 "Entry Point Length should be 0x1e when it should be 0x1f.", warning);
> -               }
> +               } else
> +                       fwts_passed(fw, "SMBIOS Table Entry Point Length is valid.");
>         }
> +
>         if (memcmp(entry.anchor_string, "_DMI_", 5) != 0) {
>                 fwts_failed(fw, LOG_LEVEL_HIGH,
> -                       "SMBIOSBadIntermediateAnchor",
> -                       "SMBIOS Table Entry Intermediate Anchor String was '%5.5s' and should be '_DMI_'.",
> +                       "SMBIOSBadIntermediateAnchor",
> +                       "SMBIOS Table Entry Intermediate Anchor String was '%5.5s' and should be '_DMI_'.",
>                         entry.anchor_string);
>                 fwts_advice(fw, "%s", warning);
> -       }
> +       } else
> +               fwts_passed(fw, "SMBIOS Table Entry Intermediate Anchor String _DMI_ is valid.");
> +
>         /* Intermediate checksum for legacy DMI */
>         checksum = fwts_checksum(((uint8_t*)&entry)+16, 15);
> -       if (checksum != 0) {
> +       if (checksum != 0)
>                 fwts_failed(fw, LOG_LEVEL_HIGH,
>                         "SMBIOSBadChecksum",
>                         "SMBIOS Table Entry Point Intermediate Checksum is 0x%2.2x, should be 0x%2.2x",
>                                 entry.intermediate_checksum,
>                                 (uint8_t)(entry.intermediate_checksum - checksum));
> -       }
> +       else
> +               fwts_passed(fw, "SMBIOS Table Entry Point Intermediate Checksum is valid.");
> +
>         if ((entry.struct_table_length > 0) && (entry.struct_table_address == 0)) {
>                 fwts_failed(fw, LOG_LEVEL_HIGH,
>                         "SMBIOSBadTableAddress",
> @@ -115,6 +198,13 @@ static int smbios_test1(fwts_framework *fw)
>                 fwts_advice(fw,
>                         "The address of the SMBIOS Structure Tables must be defined if the "
>                         "length of these tables is defined.");
> +       } else {
> +               /*
> +                * Now does the DMI table look sane? If not,
> +                * the SMBIOS Structure Table could be bad
> +                */
> +               if (smbios_dmi_sane(fw, &entry) == FWTS_OK)
> +                       fwts_passed(fw, "SMBIOS Table Entry Structure Table Address and Length looks valid.");
>         }
>
>         return FWTS_OK;
> --
> 1.8.3.1
>
>
> --
> fwts-devel mailing list
> fwts-devel@lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/fwts-devel
Colin King - June 20, 2013, 8:28 a.m.
On 20/06/13 09:15, Keng-YĆ¼ Lin wrote:
> Hi Colin,
>   This patch has some rejected hunks seemingly due to some tidy-up of
> the leading tabs.
> 
>   if there is any missing patch from you? (or anyone I overlooked?)
> 
Oops, I'll send that in a moment. I overlooked that. Apologies

Patch

diff --git a/src/bios/smbios/smbios.c b/src/bios/smbios/smbios.c
index 57c8999..022447a 100644
--- a/src/bios/smbios/smbios.c
+++ b/src/bios/smbios/smbios.c
@@ -16,12 +16,15 @@ 
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
  *
  */
+#include <stdint.h>
+#include <inttypes.h>
+
 #include "fwts.h"
 
 #ifdef FWTS_ARCH_INTEL
 
 static void smbios_dump_entry(fwts_framework *fw, fwts_smbios_entry *entry, fwts_smbios_type type)
-{	
+{
 	if (type == FWTS_SMBIOS) {
 		fwts_log_info_verbatum(fw, "SMBIOS Entry Point Structure:");
 		fwts_log_info_verbatum(fw, "  Anchor String          : %4.4s", entry->signature);
@@ -31,7 +34,7 @@  static void smbios_dump_entry(fwts_framework *fw, fwts_smbios_entry *entry, fwts
 		fwts_log_info_verbatum(fw, "  Minor Version          : 0x%2.2x", entry->minor_version);
 		fwts_log_info_verbatum(fw, "  Maximum Struct Size    : 0x%2.2x", entry->max_struct_size);
 		fwts_log_info_verbatum(fw, "  Entry Point Revision   : 0x%2.2x", entry->revision);
-		fwts_log_info_verbatum(fw, "  Formatted Area         : 0x%2.2x 0x%2.2x 0x%2.2x 0x%2.2x 0x%2.2x", 
+		fwts_log_info_verbatum(fw, "  Formatted Area         : 0x%2.2x 0x%2.2x 0x%2.2x 0x%2.2x 0x%2.2x",
 			entry->formatted_area[0], entry->formatted_area[1],
 			entry->formatted_area[2], entry->formatted_area[3],
 			entry->formatted_area[4]);
@@ -50,6 +53,77 @@  static void smbios_dump_entry(fwts_framework *fw, fwts_smbios_entry *entry, fwts
 		fwts_log_info_verbatum(fw, "    BCD Revision 00 indicates compliance with specification stated in Major/Minor Version.");
 }
 
+static int smbios_dmi_sane(fwts_framework *fw, fwts_smbios_entry *entry)
+{
+	uint8_t	*table, *ptr;
+	uint8_t dmi_entry_length;
+	uint8_t dmi_entry_type = 0;
+	uint16_t i = 0;
+	uint16_t table_length = entry->struct_table_length;
+	int ret = FWTS_OK;
+
+	ptr = table = fwts_mmap((off_t)entry->struct_table_address,
+			  (size_t)table_length);
+	if (table == FWTS_MAP_FAILED) {
+		fwts_failed(fw, LOG_LEVEL_MEDIUM,
+			"SMBIOSTableAddressNotMapped",
+			"Cannot mmap SMBIOS tables from "
+			"%8.8" PRIx32 "..%8.8" PRIx32 ".",
+			entry->struct_table_address,
+			entry->struct_table_address + table_length);
+			return FWTS_ERROR;
+	}
+
+	for (i = 0; i < entry->number_smbios_structures; i++) {
+		if (ptr > table + table_length) {
+			fwts_failed(fw, LOG_LEVEL_MEDIUM,
+				"SMBIOSTableLengthTooSmall",
+				"The size indicated by the SMBIOS table length is "
+				"smaller than the DMI data.");
+			ret = FWTS_ERROR;
+			break;
+		}
+
+		dmi_entry_type = ptr[0];
+		dmi_entry_length = ptr[1];
+
+		if (dmi_entry_length < 4) {
+			fwts_failed(fw, LOG_LEVEL_MEDIUM,
+				"SMBIOSIllegalTableEntry",
+				"The size of a DMI entry %" PRIu16 " is illegal, "
+				"DMI data is either wrong or the SMBIOS Table "
+				"Pointer is pointing to the wrong memory region.", i);
+			ret = FWTS_ERROR;
+			break;
+		}
+		ptr += dmi_entry_length;
+
+		/* Scan for end of DMI entry, must be 2 zero bytes */
+		while (((ptr - table + 1) < table_length) &&
+		       ((ptr[0] != 0) || (ptr[1] != 0)))
+				ptr++;
+		/* Skip over the two zero bytes */
+		ptr += 2;
+	}
+
+	/* We found DMI end of table, are number of entries correct? */
+	if ((dmi_entry_type == 127) && (i != entry->number_smbios_structures)) {
+		fwts_failed(fw, LOG_LEVEL_MEDIUM,
+			"SMBIOSNumberOfStructures",
+			"The end of DMI table marker structure was found "
+			"but only %d DMI structures were found. The SMBIOS "
+			"entry table reported that there were %" PRIu16
+			" DMI structures in the DMI table.",
+			i, entry->number_smbios_structures);
+		/* And table length can't be correct either */
+		ret = FWTS_ERROR;
+	}
+
+	(void)fwts_munmap(table, (size_t)entry->struct_table_length);
+
+	return ret;
+}
+
 static int smbios_test1(fwts_framework *fw)
 {
 	void *addr = 0;
@@ -57,7 +131,7 @@  static int smbios_test1(fwts_framework *fw)
 	fwts_smbios_type  type;
 	uint16_t	  version;
 	uint8_t checksum;
-	static char warning[] = 
+	static char warning[] =
 		"This field is not checked by the kernel, and so will not affect the system, "
 		"however it should be fixed to conform to the latest version of the "
 		"System Management BIOS (SMBIOS) Reference Specification. ";
@@ -75,39 +149,48 @@  static int smbios_test1(fwts_framework *fw)
 
 	fwts_passed(fw, "Found SMBIOS Table Entry Point at %p", addr);
 	smbios_dump_entry(fw, &entry, type);
-		
+	fwts_log_nl(fw);
+
 	if (type == FWTS_SMBIOS) {
 		checksum = fwts_checksum((uint8_t*)&entry, sizeof(fwts_smbios_entry));
-		if (checksum != 0) {
+		if (checksum != 0)
 			fwts_failed(fw, LOG_LEVEL_HIGH,
 				"SMBIOSBadChecksum",
 				"SMBIOS Table Entry Point Checksum is 0x%2.2x, should be 0x%2.2x",
 					entry.checksum, (uint8_t)(entry.checksum - checksum));
-		}
+		else
+			fwts_passed(fw, "SMBIOS Table Entry Point Checksum is valid.");
+
 		if (entry.length != 0x1f) {
 			fwts_failed(fw, LOG_LEVEL_LOW,
 				"SMBIOSBadEntryLength",
 				"SMBIOS Table Entry Point Length is 0x%2.2x, should be 0x1f", entry.length);
 			fwts_advice(fw, "%s Note that version 2.1 of the specification incorrectly stated that the "
 				"Entry Point Length should be 0x1e when it should be 0x1f.", warning);
-		}
+		} else
+			fwts_passed(fw, "SMBIOS Table Entry Point Length is valid.");
 	}
+
 	if (memcmp(entry.anchor_string, "_DMI_", 5) != 0) {
 		fwts_failed(fw, LOG_LEVEL_HIGH,
-			"SMBIOSBadIntermediateAnchor", 
-			"SMBIOS Table Entry Intermediate Anchor String was '%5.5s' and should be '_DMI_'.", 
+			"SMBIOSBadIntermediateAnchor",
+			"SMBIOS Table Entry Intermediate Anchor String was '%5.5s' and should be '_DMI_'.",
 			entry.anchor_string);
 		fwts_advice(fw, "%s", warning);
-	}
+	} else
+		fwts_passed(fw, "SMBIOS Table Entry Intermediate Anchor String _DMI_ is valid.");
+
 	/* Intermediate checksum for legacy DMI */
 	checksum = fwts_checksum(((uint8_t*)&entry)+16, 15);
-	if (checksum != 0) {
+	if (checksum != 0)
 		fwts_failed(fw, LOG_LEVEL_HIGH,
 			"SMBIOSBadChecksum",
 			"SMBIOS Table Entry Point Intermediate Checksum is 0x%2.2x, should be 0x%2.2x",
 				entry.intermediate_checksum,
 				(uint8_t)(entry.intermediate_checksum - checksum));
-	}
+	else
+		fwts_passed(fw, "SMBIOS Table Entry Point Intermediate Checksum is valid.");
+
 	if ((entry.struct_table_length > 0) && (entry.struct_table_address == 0)) {
 		fwts_failed(fw, LOG_LEVEL_HIGH,
 			"SMBIOSBadTableAddress",
@@ -115,6 +198,13 @@  static int smbios_test1(fwts_framework *fw)
 		fwts_advice(fw,
 			"The address of the SMBIOS Structure Tables must be defined if the "
 			"length of these tables is defined.");
+	} else {
+		/*
+		 * Now does the DMI table look sane? If not,
+		 * the SMBIOS Structure Table could be bad
+		 */
+		if (smbios_dmi_sane(fw, &entry) == FWTS_OK)
+			fwts_passed(fw, "SMBIOS Table Entry Structure Table Address and Length looks valid.");
 	}
 
 	return FWTS_OK;