[prev in list] [next in list] [prev in thread] [next in thread]
List: fwts-devel
Subject: Re: [PATCH] dmicheck: checks type length vs. SMBIOS versions
From: Colin Ian King <colin.king () canonical ! com>
Date: 2019-04-25 8:23:55
Message-ID: 2012b058-ab08-fa1b-0573-a1e2796c29b9 () canonical ! com
[Download RAW message or body]
On 25/04/2019 06:07, Alex Hung wrote:
> Buglink: https://bugs.launchpad.net/bugs/1824397
>
> Some SMBIOS types grow with SMBIOS specs. This checks whether BIOS
> implements new SMBIOS spec accordingly.
>
> Signed-off-by: Alex Hung <alex.hung@canonical.com>
> ---
> src/dmi/dmicheck/dmicheck.c | 150 +++++++++++++++++++++++++++++++++++-
> 1 file changed, 147 insertions(+), 3 deletions(-)
>
> diff --git a/src/dmi/dmicheck/dmicheck.c b/src/dmi/dmicheck/dmicheck.c
> index a5b88187..c86050c4 100644
> --- a/src/dmi/dmicheck/dmicheck.c
> +++ b/src/dmi/dmicheck/dmicheck.c
> @@ -1113,9 +1113,149 @@ static void dmi_uuid_check(fwts_framework *fw,
> }
> }
I'd prefer all struct declarations and statics to be at the top of the
source to follow the vague fwts coding style.
>
> +struct dmi_type_length {
> + uint8_t type;
> + uint16_t max_version;
> + uint16_t min_version;
> + uint8_t length;
> +};
> +
> +static struct dmi_type_length type_info[] = {
This could be a const; static const struct dmi_type_length etc
> + {
> + .type = 15,
> + .max_version = 0x201,
> + .min_version = 0x200,
> + .length = 0x14,
> + },
> + {
> + .type = 16,
> + .max_version = 0x207,
> + .min_version = 0x201,
> + .length = 0xf,
> + },
> + {
> + .type = 16,
> + .max_version = 0xFFFF, /* no max */
> + .min_version = 0x207,
> + .length = 0x17,
> + },
> + {
> + .type = 17,
> + .max_version = 0x203,
> + .min_version = 0x201,
> + .length = 0x15,
> + },
> + {
> + .type = 17,
> + .max_version = 0x206,
> + .min_version = 0x203,
> + .length = 0x1b,
> + },
> + {
> + .type = 17,
> + .max_version = 0x207,
> + .min_version = 0x206,
> + .length = 0x1c,
> + },
> + {
> + .type = 17,
> + .max_version = 0x208,
> + .min_version = 0x207,
> + .length = 0x22,
> + },
> + {
> + .type = 17,
> + .max_version = 0x302,
> + .min_version = 0x208,
> + .length = 0x28,
> + },
> + {
> + .type = 19,
> + .max_version = 0x207,
> + .min_version = 0x201,
> + .length = 0xf,
> + },
> + {
> + .type = 19,
> + .max_version = 0xFFFF, /* no max */
> + .min_version = 0x207,
> + .length = 0x1f,
> + },
> + {
> + .type = 20,
> + .max_version = 0x207,
> + .min_version = 0x201,
> + .length = 0x13,
> + },
> + {
> + .type = 20,
> + .max_version = 0xFFFF, /* no max */
> + .min_version = 0x207,
> + .length = 0x23,
> + },
> + { /* terminator */
> + .type = 0,
> + .max_version = 0,
> + .min_version = 0,
> + .length = 0,
> + }
> +};
> +
> +static void dmicheck_type_length(
> + fwts_framework *fw,
> + const uint16_t smbios_version,
> + const fwts_dmi_header *hdr)
> +{
> + struct dmi_type_length *tbl = type_info;
const struct dmi_type_length *tbl = type_info;
> + uint8_t length = hdr->length;
> + uint8_t *data = hdr->data;
const uint8_t *data ...
> + bool passed = false;
> +
> + /* Special cases */
> + switch (hdr->type) {
> + case 15:
> + if (smbios_version >= 0x201) {
In the very unlikely case where length is less than 0x15, accesses to
data[0x15] are out of bounds.
> + length = 0x17 + data[0x15] * data[0x16];
> + if (length == hdr->length)
> + passed = true;
> + }
> + break;
> + case 17:
> + if (smbios_version >= 0x302 && length >= 0x34)
> + passed = true;
> + break;
> + default:
> + passed = true;
> + break;
> + }
> +
> + /* general cases */
> + while (tbl->length != 0) {
> + if (hdr->type != tbl->type) {
> + tbl++;
> + continue;
> + }
> + if (smbios_version >= tbl->min_version && smbios_version < tbl->max_version) {
> + if (length == tbl->length) {
> + passed = true;
> + } else
> + passed = false;
> + break;
> + }
> + tbl++;
> + }
> +
> + if (!passed)
> + fwts_failed(fw, LOG_LEVEL_HIGH, DMI_BAD_TABLE_LENGTH,
> + "Type %" PRId8 " expects length of 0x%" PRIx8
> + ", has incorrect length of 0x%" PRIx8,
> + hdr->type, tbl->length, length);
> +}
> +
> static void dmicheck_entry(fwts_framework *fw,
> uint32_t addr,
> - const fwts_dmi_header *hdr)
> + const fwts_dmi_header *hdr,
> + const uint16_t smbios_version)
> {
> uint8_t *ptr;
> uint8_t count;
> @@ -1129,6 +1269,8 @@ static void dmicheck_entry(fwts_framework *fw,
> uint32_t battery_count;
> int ret;
>
> + dmicheck_type_length(fw, smbios_version, hdr);
> +
> switch (hdr->type) {
> case 0: /* 7.1 */
> table = "BIOS Information (Type 0)";
> @@ -1892,6 +2034,7 @@ static void dmi_scan_tables(fwts_framework *fw,
> fwts_smbios_entry *entry,
> uint8_t *table)
> {
> + uint16_t smbios_version = (entry->major_version << 8) + entry->minor_version;
make smbios_version const perhaps?
Also, the line is rather wide, can it be broken over to lines?
> uint8_t *entry_data = table;
> uint16_t table_length;
> uint16_t struct_count;
> @@ -1937,7 +2080,7 @@ static void dmi_scan_tables(fwts_framework *fw,
> next_entry += 2;
>
> if ((next_entry - table) <= table_length)
> - dmicheck_entry(fw, addr, &hdr);
> + dmicheck_entry(fw, addr, &hdr, smbios_version);
>
> i++;
> entry_data = next_entry;
> @@ -2008,6 +2151,7 @@ static void dmi_scan_smbios30_table(fwts_framework *fw,
> fwts_smbios30_entry *entry,
> uint8_t *table)
> {
> + uint16_t smbios_version = (entry->major_version << 8) + entry->minor_version;
same here, const smbios_version and ensure line is less than 80 chars wide
> uint8_t *entry_data = table;
> uint16_t table_max_length;
> int i = 0;
> @@ -2054,7 +2198,7 @@ static void dmi_scan_smbios30_table(fwts_framework *fw,
> next_entry += 2;
>
> if ((next_entry - table) <= table_max_length) {
> - dmicheck_entry(fw, addr, &hdr);
> + dmicheck_entry(fw, addr, &hdr, smbios_version);
> if (fw->flags & FWTS_FLAG_TEST_SBBR)
> sbbr_test_entry_check(&hdr);
> }
>
Just my minor nit-picks, apart from that, it's good.
Colin
--
fwts-devel mailing list
fwts-devel@lists.ubuntu.com
Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/fwts-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic