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

[Laszlo Ersek]

On 11/10/16 18:18, Laszlo Ersek wrote:
> 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:)

Another bug I failed to notice at first: we are checking offsets from
the beginning of the entire "smbios_table" array. That's not good when
we already have at least one SMBIOS table in that array; we should be
checking the last table that we just read from the file and appended to
"smbios_tables". Therefore the offset should be

    smbios_tables_len + tables_str_field_offset[i]

I assume this issue was not noticed because only one table was passed in
for testing.

Anyway, I'm not suggesting to fix these problems; I'm just pointing them
out for completeness.

My proposal is to extend dmidecode.

Honestly, I don't even understand why dmidecode doesn't have this
capability yet: dump precisely one table (that is, instance N of table
type K) as it is in memory, defined by the SMBIOS spec. The SMBIOS spec
says in 6.1.1 "Structure evolution and usage guidelines",

    Each structure shall be terminated by a double-null (0000h), either
    directly following the formatted area (if no strings are present)
    or directly following the last string. This includes system- and
    OEM-specific structures and allows upper-level software to easily
    traverse the structure table. (See structure-termination examples
    later in this clause.)

In other words, the terminator is part of the table.

Thanks
Laszlo

> 
>>>          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
>