| Message ID | 1371291493-28262-1-git-send-email-colin.king@canonical.com |
|---|---|
| State | Accepted |
| Headers | show |
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>
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>
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
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
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;