Re: [Qemu-devel] [PATCH] smbios: Add 1 terminator if there is any string field defined in given table.

[Laszlo Ersek]

On 11/10/16 16:09, Michael S. Tsirkin wrote:
> On Tue, Sep 06, 2016 at 04:28:33PM +0800, Lin Ma wrote:
>> If user specifies binary file on command line to load smbios entries, then
>> will get error messages while decoding them in guest.
>>
>> Reproducer:
>> 1. dump a smbios table to a binary file from host or guest.(says table 1)
>> 2. load the binary file through command line: 'qemu -smbios file=...'.
>> 3. perform 'dmidecode' or 'dmidecode -t 1' in guest.
>>
>> It reports 'Invalid entry length...' because qemu doesn't add terminator(s) 
>> for
>> the table correctly.
>> For smbios tables which have string field provided, qemu should add 1 
>> terminator.
>> For smbios tables which dont have string field provided, qemu should add 2.
>>
>> This patch fixed the issue.
>>
>> Signed-off-by: Lin Ma <address@hidden>
> 
> Seems to make sense superficially
> 
> Acked-by: Michael S. Tsirkin <address@hidden>
> 
> Fam, would you like to take this?

If we're not in a mortal hurry to enable QEMU to consume dmidecode
output directly, can we please think about enabling dmidecode to dump
binarily-correct tables?

The commit message doesn't mention, but in the dmidecode manual, I see
"--dump-bin" and "--from-dump". Hm... The manual says,

>            --dump-bin FILE
>               Do  not  decode the entries, instead dump the DMI data
>               to a file in binary form. The generated file is  suit-
>               able to pass to --from-dump later.
>
>            --from-dump FILE
>               Read the DMI data from a binary file previously gener-
>               ated using --dump-bin.
> [...]
>
> BINARY DUMP FILE FORMAT
>        The binary dump files generated by --dump-bin and read  using
>        --from-dump are formatted as follows:
>
>        * The  SMBIOS  or  DMI entry point is located at offset 0x00.
>          It is crafted to hard-code  the  table  address  at  offset
>          0x20.
>
>        * The DMI table is located at offset 0x20.

Is this how the tables were dumped originally, in step 1?

Actually, the manual also says,

>        Options  --string, --type and --dump-bin determine the output
>        format and are mutually exclusive.

Since step 1 mentions "say[] table 1", I think --dump-bin was not used.
In that case however, dmidecode can only produce textual output, for
example, hexadecimal output, with "--dump".

This means that some helper utility must have been used to turn the
hexadecimal output into binary. Since the dmidecode output has to be
post-processed anyway, I wonder if we should keep this data munging out
of QEMU.

Anyway, I have some comments for the patch as well:


>> ---
>>  hw/smbios/smbios.c         | 90 
>> ++++++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/smbios/smbios.h | 44 +++++++++++++++++++++++
>>  2 files changed, 134 insertions(+)
>>
>> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
>> index 74c7102..6293bc5 100644
>> --- a/hw/smbios/smbios.c
>> +++ b/hw/smbios/smbios.c
>> @@ -885,6 +885,9 @@ void smbios_entry_add(QemuOpts *opts)
>>  {
>>      const char *val;
>>  
>> +    int i, terminator_count = 2, table_str_field_count = 0;
>> +    int *tables_str_field_offset = NULL;
>> +
>>      assert(!smbios_immutable);
>>  
>>      val = qemu_opt_get(opts, "file");
>> @@ -926,7 +929,94 @@ void smbios_entry_add(QemuOpts *opts)
>>              smbios_type4_count++;
>>          }
>>  
>> +        switch (header->type) {
>> +        case 0:
>> +            tables_str_field_offset = g_malloc0(sizeof(int) * \
>> +                                                TYPE_0_STR_FIELD_COUNT);
>> +            tables_str_field_offset = (int []){\
>> +                                    TYPE_0_STR_FIELD_OFFSET_VENDOR, \
>> +                                    TYPE_0_STR_FIELD_OFFSET_BIOS_VERSION, \
>> +                                    
>> TYPE_0_STR_FIELD_OFFSET_BIOS_RELEASE_DATE};

This assignment doesn't do what you think it does.
"tables_str_field_offset" is a pointer, and the result of the

  (int []){...}

compound literal is an unnamed array object with automatic storage
duration. The lifetime of the unnamed array object is limited to the
scope of the enclosing block, which means the "switch" statement here.

The assignment does not copy the contents of the array into the
dynamically allocated area; instead, the unnamed array object is
converted to a pointer to its first element, and the
"tables_str_field_offset" pointer is set to that value. The original
dynamic allocation is leaked.

>> +            table_str_field_count = sizeof(tables_str_field_offset) / \
>> +                                    sizeof(tables_str_field_offset[0]);

This is wrong again; the dividend is the size of the pointer, not that
of the pointed-to-array. The size of the array is not available through
the pointer.

I assume testing has been done with 64-bit builds, so that the pointer
size is 8 bytes, and the pointed-to type is 4 bytes ("int"). This would
yield a count of 2, and I guess no input was tested where only the third
(or a later) string pointer was nonzero.

>> +            break;
>> +        case 1:
>> +            tables_str_field_offset = g_malloc0(sizeof(int) * \
>> +                                                TYPE_1_STR_FIELD_COUNT);
>> +            tables_str_field_offset = (int []){
>> +                                    TYPE_1_STR_FIELD_OFFSET_MANUFACTURER, \
>> +                                    TYPE_1_STR_FIELD_OFFSET_PRODUCT, \
>> +                                    TYPE_1_STR_FIELD_OFFSET_VERSION, \
>> +                                    TYPE_1_STR_FIELD_OFFSET_SERIAL, \
>> +                                    TYPE_1_STR_FIELD_OFFSET_SKU, \
>> +                                    TYPE_1_STR_FIELD_OFFSET_FAMILY};
>> +            table_str_field_count = sizeof(tables_str_field_offset) / \
>> +                                    sizeof(tables_str_field_offset[0]);
>> +            break;
>> +        case 2:
>> +            tables_str_field_offset = g_malloc0(sizeof(int) * \
>> +                                                TYPE_2_STR_FIELD_COUNT);
>> +            tables_str_field_offset = (int []){\
>> +                                    TYPE_2_STR_FIELD_OFFSET_MANUFACTURER, \
>> +                                    TYPE_2_STR_FIELD_OFFSET_PRODUCT, \
>> +                                    TYPE_2_STR_FIELD_OFFSET_VERSION, \
>> +                                    TYPE_2_STR_FIELD_OFFSET_SERIAL, \
>> +                                    TYPE_2_STR_FIELD_OFFSET_ASSET, \
>> +                                    TYPE_2_STR_FIELD_OFFSET_LOCATION};
>> +            table_str_field_count = sizeof(tables_str_field_offset) / \
>> +                                    sizeof(tables_str_field_offset[0]);
>> +            break;
>> +        case 3:
>> +            tables_str_field_offset = g_malloc0(sizeof(int) * \
>> +                                                TYPE_3_STR_FIELD_COUNT);
>> +            tables_str_field_offset = (int []){\
>> +                                    TYPE_3_STR_FIELD_OFFSET_MANUFACTURER, \
>> +                                    TYPE_3_STR_FIELD_OFFSET_VERSION, \
>> +                                    TYPE_3_STR_FIELD_OFFSET_SERIAL, \
>> +                                    TYPE_3_STR_FIELD_OFFSET_ASSET, \
>> +                                    TYPE_3_STR_FIELD_OFFSET_SKU};
>> +            table_str_field_count = sizeof(tables_str_field_offset) / \
>> +                                    sizeof(tables_str_field_offset[0]);
>> +            break;
>> +        case 4:
>> +            tables_str_field_offset = g_malloc0(sizeof(int) * \
>> +                                                TYPE_4_STR_FIELD_COUNT);
>> +            tables_str_field_offset = (int []){\
>> +                                    TYPE_4_STR_FIELD_OFFSET_SOCKET, \
>> +                                    
>> TYPE_4_STR_FIELD_OFFSET_PROCESSOR_MANUFACTURER, \
>> +                                    
>> TYPE_4_STR_FIELD_OFFSET_PROCESSOR_VERSION, \
>> +                                    TYPE_4_STR_FIELD_OFFSET_SERIAL, \
>> +                                    TYPE_4_STR_FIELD_OFFSET_ASSET, \
>> +                                    TYPE_4_STR_FIELD_OFFSET_PART};
>> +            table_str_field_count = sizeof(tables_str_field_offset) / \
>> +                                    sizeof(tables_str_field_offset[0]);
>> +            break;
>> +        case 17:
>> +            tables_str_field_offset = g_malloc0(sizeof(int) * \
>> +                                                TYPE_17_STR_FIELD_COUNT);
>> +            tables_str_field_offset = (int []){\
>> +                                    
>> TYPE_17_STR_FIELD_OFFSET_DEVICE_LOCATOR, \
>> +                                    TYPE_17_STR_FIELD_OFFSET_BANK_LOCATOR, \
>> +                                    TYPE_17_STR_FIELD_OFFSET_MANUFACTURER, \
>> +                                    TYPE_17_STR_FIELD_OFFSET_SERIAL, \
>> +                                    TYPE_17_STR_FIELD_OFFSET_ASSET, \
>> +                                    TYPE_17_STR_FIELD_OFFSET_PART};
>> +            table_str_field_count = sizeof(tables_str_field_offset) / \
>> +                                    sizeof(tables_str_field_offset[0]);
>> +            break;
>> +        default:
>> +            break;
>> +        }

So, at this point, with the switch statement's scope terminated,
"tables_str_field_offset" points into released storage.

>> +
>> +        for (i = 0; i < table_str_field_count; i++) {
>> +            if (*(uint8_t *)(smbios_tables + tables_str_field_offset[i]) > 
>> 0) {
>> +                terminator_count = 1;
>> +                break;
>> +            }
>> +        }
>> +

The condition can be rewritten more simply as follows (because
smbios_tables already has type (uint8_t*):

  smbios_tables[tables_str_field_offset[i]] > 0

Independently of the bug that "tables_str_field_offset" points into
released storage, the above expression is unsafe in its own right.
Namely, we are not checking whether

  tables_str_field_offset[i] < smbios_tables_len

(And even if we wanted to enforce that, the "smbios_tables_len" variable
is incremented only below:)

>>          smbios_tables_len += size;
>> +        smbios_tables_len += terminator_count;
>>          if (size > smbios_table_max) {
>>              smbios_table_max = size;
>>          }

Wrong again: we haven't allocated additional storage for the
terminator(s). We've allocated extra space that's enough only for the
loaded file itself:

        size = get_image_size(val);
        if (size == -1 || size < sizeof(struct smbios_structure_header)) {
            error_report("Cannot read SMBIOS file %s", val);
            exit(1);
        }

        /*
         * NOTE: standard double '\0' terminator expected, per smbios spec.
         * (except in legacy mode, where the second '\0' is implicit and
         *  will be inserted by the BIOS).
         */
        smbios_tables = g_realloc(smbios_tables, smbios_tables_len + size);
        header = (struct smbios_structure_header *)(smbios_tables +
                                                    smbios_tables_len);

(In fact, the comment spells out the requirement for the external files
provided by the user.

>> diff --git a/include/hw/smbios/smbios.h b/include/hw/smbios/smbios.h
>> index 1cd53cc..6d59c3d 100644
>> --- a/include/hw/smbios/smbios.h
>> +++ b/include/hw/smbios/smbios.h
>> @@ -267,4 +267,48 @@ void smbios_get_tables(const struct 
>> smbios_phys_mem_area *mem_array,
>>                         const unsigned int mem_array_size,
>>                         uint8_t **tables, size_t *tables_len,
>>                         uint8_t **anchor, size_t *anchor_len);
>> +
>> +#define TYPE_0_STR_FIELD_OFFSET_VENDOR 0x4
>> +#define TYPE_0_STR_FIELD_OFFSET_BIOS_VERSION 0x5
>> +#define TYPE_0_STR_FIELD_OFFSET_BIOS_RELEASE_DATE 0x8
>> +#define TYPE_0_STR_FIELD_COUNT 3
>> +
>> +#define TYPE_1_STR_FIELD_OFFSET_MANUFACTURER 0x4
>> +#define TYPE_1_STR_FIELD_OFFSET_PRODUCT 0x5
>> +#define TYPE_1_STR_FIELD_OFFSET_VERSION 0x6
>> +#define TYPE_1_STR_FIELD_OFFSET_SERIAL 0x7
>> +#define TYPE_1_STR_FIELD_OFFSET_SKU 0x19
>> +#define TYPE_1_STR_FIELD_OFFSET_FAMILY 0x1a
>> +#define TYPE_1_STR_FIELD_COUNT 6
>> +
>> +#define TYPE_2_STR_FIELD_OFFSET_MANUFACTURER 0x4
>> +#define TYPE_2_STR_FIELD_OFFSET_PRODUCT 0x5
>> +#define TYPE_2_STR_FIELD_OFFSET_VERSION 0x6
>> +#define TYPE_2_STR_FIELD_OFFSET_SERIAL 0x7
>> +#define TYPE_2_STR_FIELD_OFFSET_ASSET 0x8
>> +#define TYPE_2_STR_FIELD_OFFSET_LOCATION 0xa
>> +#define TYPE_2_STR_FIELD_COUNT 6
>> +
>> +#define TYPE_3_STR_FIELD_OFFSET_MANUFACTURER 0x4
>> +#define TYPE_3_STR_FIELD_OFFSET_VERSION 0x6
>> +#define TYPE_3_STR_FIELD_OFFSET_SERIAL 0x7
>> +#define TYPE_3_STR_FIELD_OFFSET_ASSET 0x8
>> +#define TYPE_3_STR_FIELD_OFFSET_SKU 0x14
>> +#define TYPE_3_STR_FIELD_COUNT 5
>> +
>> +#define TYPE_4_STR_FIELD_OFFSET_SOCKET 0x4
>> +#define TYPE_4_STR_FIELD_OFFSET_PROCESSOR_MANUFACTURER 0x7
>> +#define TYPE_4_STR_FIELD_OFFSET_PROCESSOR_VERSION 0x10
>> +#define TYPE_4_STR_FIELD_OFFSET_SERIAL 0x20
>> +#define TYPE_4_STR_FIELD_OFFSET_ASSET 0x21
>> +#define TYPE_4_STR_FIELD_OFFSET_PART 0x22
>> +#define TYPE_4_STR_FIELD_COUNT 6
>> +
>> +#define TYPE_17_STR_FIELD_OFFSET_DEVICE_LOCATOR 0x10
>> +#define TYPE_17_STR_FIELD_OFFSET_BANK_LOCATOR 0x11
>> +#define TYPE_17_STR_FIELD_OFFSET_MANUFACTURER 0x17
>> +#define TYPE_17_STR_FIELD_OFFSET_SERIAL 0x18
>> +#define TYPE_17_STR_FIELD_OFFSET_ASSET 0x19
>> +#define TYPE_17_STR_FIELD_OFFSET_PART 0x1a
>> +#define TYPE_17_STR_FIELD_COUNT 6
>>  #endif /* QEMU_SMBIOS_H */
>> -- 
>> 2.9.2

This hunk demonstrates why, in my opinion, this approach doesn't scale.
This way QEMU should know about every offset in every table type. If a
new version of the SMBIOS spec were released, QEMU would have to learn
the internals of the new tables.

I think this is the wrong approach. "dmidecode" is the dedicated tool
for working with SMBIOS tables. Whenever a new spec version is released,
dmidecode is unconditionally updated. We should try to teach dmidecode
to output directly loadable SMBIOS tables. QEMU is an important enough
project to exert this kind of influence on dmidecode.

(Thanks for the CC, Michael!)

Thanks
Laszlo