ACK: [PATCH] fwts: Fix madt testing with broken MADT entries
Alex Hung
alex.hung at canonical.com
Mon Jul 11 02:51:44 UTC 2016
On 2016-07-07 11:30 PM, Prarit Bhargava wrote:
> Please note I am not subscribed to fwdt-devel.
>
> P.
>
> ----8<----
>
> When running the madt test on a system with incorrectly labelled MADTs the
> following error happens:
>
> [root at intel-wildcatpass-05 fwts]# fwts madt
> Running 1 tests, results appended to results.log
> Test: MADT Multiple APIC Description Table (spec compliant).
> MADT checksum test. 1 passed
> MADT revision test. 2 passed
> MADT architecture minimum revision test. 1 passed
> MADT flags field reserved bits test. 1 passed
> MADT subtable tests. : 80.0% -
> Caught SIGNAL 11 (Segmentation fault), aborting.
> Backtrace:
> 0x00007f7c402ee984 /usr/local/lib/fwts/libfwts.so.1(+0xf984)
> 0x00007f7c3facf100 /lib64/libpthread.so.0(+0xf100)
> 0x00007f7c3ec3fab4 /lib64/libc.so.6(_IO_vfprintf+0x1564)
> 0x00007f7c3ec6b589 /lib64/libc.so.6(vsnprintf+0x79)
> 0x00007f7c402f3a4b /usr/local/lib/fwts/libfwts.so.1(fwts_framework_log+0xdb)
> 0x000000000041a156 fwts()
> 0x00007f7c402f352f /usr/local/lib/fwts/libfwts.so.1(+0x1452f)
> 0x00007f7c402f4ba3 /usr/local/lib/fwts/libfwts.so.1(fwts_framework_args+0x743)
> 0x0000000000404cda fwts()
> 0x00007f7c3ec18b15 /lib64/libc.so.6(__libc_start_main+0xf5)
> 0x0000000000405ad5 fwts()
>
> This occurs because the value of hdr->type (in the above case 0x7f) can
> exceed the array lengths of madt_sub_names[] and
> acpi_madt_subtable_lengths->lengths[] which have a maximum index of
> NUM_SUBTABLE_TYPES. This, of course, results in a segfault.
>
> This patch implements MADT error checking for the reserved (0x10 - 0x7f)
> and OEM (0x80 - 0xff) types. Any use of the reserved fields is considered
> a failure, and checking of the OEM fields is restricted to moving to the
> next entry as we cannot possibly know all the OEM uses of MADT entries.
>
> This results in the test passing with new failures (as expected):
>
> <snip>
> PASSED: Test 5, MADT I/O APIC flags are reserved, and set to zero.
> PASSED: Test 5, MADT I/O APIC address in non-zero.
> PASSED: Test 5, MADT subtable type 127 (Reserved. OSPM skips structures of the
> reserved type.) is defined.
> PASSED: Test 5, Subtable 196 of type 127 (Reserved. OSPM skips structures of the
> reserved type.) is the correct length: 12
> FAILED [MEDIUM] SPECMADTSubReservedID: Test 5, MADT subtable 196 is using the
> reserved value 0x7f for a type. Subtable type values 0x10..0x7f are reserved;
> 0x80..0xff can be used by OEMs.
> PASSED: Test 5, MADT subtable type 127 (Reserved. OSPM skips structures of the
> reserved type.) is defined.
> PASSED: Test 5, Subtable 197 of type 127 (Reserved. OSPM skips structures of the
> reserved type.) is the correct length: 12
> FAILED [MEDIUM] SPECMADTSubReservedID: Test 5, MADT subtable 197 is using the
> reserved value 0x7f for a type. Subtable type values 0x10..0x7f are reserved;
> 0x80..0xff can be used by OEMs.
> PASSED: Test 5, MADT subtable type 2 (Interrupt Source Override) is defined.
> PASSED: Test 5, Subtable 198 of type 2 (Interrupt Source Override) is the
> correct length: 10
> PASSED: Test 5, MADT Interrupt Source Override Bus is 0 for ISA bus.
> PASSED: Test 5, MADT Interrupt Source Override flags, bits 4..31 are reserved
> <snip>
>
> and the test completing successfully:
>
> ================================================================================
> 1182 passed, 2 failed, 0 warning, 0 aborted, 0 skipped, 0 info only.
> ================================================================================
>
> Signed-off-by: Prarit Bhargava <prarit at redhat.com>
> ---
> src/acpi/madt/madt.c | 68 +++++++++++++++++++++++++++++++++------------
> src/lib/include/fwts_acpi.h | 3 +-
> 2 files changed, 52 insertions(+), 19 deletions(-)
>
> diff --git a/src/acpi/madt/madt.c b/src/acpi/madt/madt.c
> index b65b89ed4fa7..8ee231ee16be 100644
> --- a/src/acpi/madt/madt.c
> +++ b/src/acpi/madt/madt.c
> @@ -399,6 +399,8 @@ static const char *madt_sub_names[] = {
> /* 0x0d */ "GICv2m MSI Frame",
> /* 0x0e */ "GICR Redistributor",
> /* 0x0f */ "GIC Interrupt Translation Service (ITS)",
> + /* 0x10 - 0x7f */ "Reserved. OSPM skips structures of the reserved type.",
> + /* 0x80 - 0xff */ "Reserved for OEM use",
> NULL
> };
>
> @@ -1207,8 +1209,9 @@ static int madt_subtables(fwts_framework *fw)
>
> while (length > (ssize_t)sizeof(fwts_acpi_madt_sub_table_header)) {
> ssize_t skip = 0;
> - int len;
> + int len = 0;
> bool passed = true;
> + int type;
>
> hdr = (fwts_acpi_madt_sub_table_header *)data;
> ii++;
> @@ -1216,18 +1219,32 @@ static int madt_subtables(fwts_framework *fw)
> data += sizeof(fwts_acpi_madt_sub_table_header);
> length -= sizeof(fwts_acpi_madt_sub_table_header);
>
> - /* is this subtable type defined? */
> - len = ms->lengths[hdr->type];
> + /* set initial type value, will be overriden for OEM and
> + * reserved entries */
> + type = hdr->type;
> +
> + /* check for OEM and reserved entries */
> + if (hdr->type >= NUM_SUBTABLE_TYPES) {
> + if (hdr->type < 0x80)
> + type = FWTS_ACPI_MADT_RESERVED;
> + else
> + type = FWTS_ACPI_MADT_OEM;
> + len = hdr->length;
> + } else {
> + /* this subtable is defined */
> + len = ms->lengths[hdr->type];
> + }
> +
> if (!len) {
> fwts_failed(fw, LOG_LEVEL_MEDIUM,
> "SPECMADTSubType",
> "Undefined MADT subtable type for this "
> "version of the MADT: %d (%s)",
> - hdr->type, madt_sub_names[hdr->type]);
> + hdr->type, madt_sub_names[type]);
> } else {
> fwts_passed(fw,
> "MADT subtable type %d (%s) is defined.",
> - hdr->type, madt_sub_names[hdr->type]);
> + hdr->type, madt_sub_names[type]);
> }
>
> /* verify that the length is what we expect */
> @@ -1250,7 +1267,7 @@ static int madt_subtables(fwts_framework *fw)
> "Subtable %d of type %d (%s) is the "
> " correct length: %d",
> ii, hdr->type,
> - madt_sub_names[hdr->type],
> + madt_sub_names[type],
> hdr->length);
> } else {
> fwts_failed(fw, LOG_LEVEL_MEDIUM,
> @@ -1258,12 +1275,12 @@ static int madt_subtables(fwts_framework *fw)
> "Subtable %d of type %d (%s) is %d bytes "
> " long but should be %d bytes",
> ii, hdr->type,
> - madt_sub_names[hdr->type],
> + madt_sub_names[type],
> hdr->length, len);
> }
>
> /* perform checks specific to subtable types */
> - switch (hdr->type) {
> + switch (type) {
> case FWTS_ACPI_MADT_LOCAL_APIC:
> skip = madt_local_apic(fw, hdr, data);
> break;
> @@ -1328,17 +1345,32 @@ static int madt_subtables(fwts_framework *fw)
> skip = madt_gic_its(fw, hdr, data);
> break;
>
> + case FWTS_ACPI_MADT_RESERVED:
> + fwts_failed(fw, LOG_LEVEL_MEDIUM,
> + "SPECMADTSubReservedID",
> + "MADT subtable %d is using the "
> + "reserved value 0x%x for a type. "
> + "Subtable type values 0x10..0x7f "
> + "are reserved; 0x80..0xff can be "
> + "used by OEMs.",
> + ii, hdr->type);
> + skip = (hdr->length -
> + sizeof(fwts_acpi_madt_sub_table_header));
> + break;
> + case FWTS_ACPI_MADT_OEM:
> + /* OEM entries must be assumed to be valid */
> + skip = (hdr->length -
> + sizeof(fwts_acpi_madt_sub_table_header));
> + break;
> default:
> - if (hdr->type >= 0x10 && hdr->type <= 0x7f)
> - fwts_failed(fw, LOG_LEVEL_MEDIUM,
> - "SPECMADTSubReservedID",
> - "MADT subtable %d is using the "
> - "reserved value 0x%x for a type. "
> - "Subtable type values 0x10..0x7f "
> - "are reserved; 0x80..0xff can be "
> - "used by OEMs.",
> - ii, hdr->type);
> - skip = hdr->length;
> + fwts_failed(fw, LOG_LEVEL_MEDIUM,
> + "SPECMADTSubReservedID",
> + "MADT subtable %d is using value 0x%x "
> + "for a type. This value is out of the "
> + "expected range of 0x00 .. 0xff.",
> + ii, hdr->type);
> + skip = (hdr->length -
> + sizeof(fwts_acpi_madt_sub_table_header));
> break;
> }
>
> diff --git a/src/lib/include/fwts_acpi.h b/src/lib/include/fwts_acpi.h
> index a3a5701c93b9..02fdceb2db11 100644
> --- a/src/lib/include/fwts_acpi.h
> +++ b/src/lib/include/fwts_acpi.h
> @@ -488,7 +488,8 @@ typedef enum {
> FWTS_ACPI_MADT_GIC_V2M_MSI_FRAME,
> FWTS_ACPI_MADT_GIC_R_REDISTRIBUTOR,
> FWTS_ACPI_MADT_GIC_ITS,
> - FWTS_ACPI_MADT_RESERVED
> + FWTS_ACPI_MADT_RESERVED, /* does not have defined structure */
> + FWTS_ACPI_MADT_OEM /* does not have defined structure */
> } fwts_acpi_madt_type;
>
> /* Type 0, FWTS_ACPI_MADT_LOCAL_APIC */
>
Acked-by: Alex Hung <alex.hung at canonical.com>
More information about the fwts-devel
mailing list