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 <l...@suse.com>
>>
>> Seems to make sense superficially
>>
>> Acked-by: Michael S. Tsirkin <m...@redhat.com>
>>
>> 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
>
