* [Qemu-devel] [PATCH] smbios: Add 1 terminator if there is any string field defined in given table. @ 2016-09-06 8:28 Lin Ma 2016-11-07 16:25 ` [Qemu-devel] 答复： " Lin Ma 2016-11-10 15:09 ` [Qemu-devel] " Michael S. Tsirkin 0 siblings, 2 replies; 7+ messages in thread From: Lin Ma @ 2016-09-06 8:28 UTC (permalink / raw) To: mst, imammedo, qemu-devel 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 <lma@suse.com> --- 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}; + table_str_field_count = sizeof(tables_str_field_offset) / \ + sizeof(tables_str_field_offset[0]); + 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; + } + + for (i = 0; i < table_str_field_count; i++) { + if (*(uint8_t *)(smbios_tables + tables_str_field_offset[i]) > 0) { + terminator_count = 1; + break; + } + } + smbios_tables_len += size; + smbios_tables_len += terminator_count; if (size > smbios_table_max) { smbios_table_max = size; } 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [Qemu-devel] 答复： [PATCH] smbios: Add 1 terminator if there is any string field defined in given table. 2016-09-06 8:28 [Qemu-devel] [PATCH] smbios: Add 1 terminator if there is any string field defined in given table Lin Ma @ 2016-11-07 16:25 ` Lin Ma 2016-11-07 16:31 ` Daniel P. Berrange 2016-11-10 15:09 ` [Qemu-devel] " Michael S. Tsirkin 1 sibling, 1 reply; 7+ messages in thread From: Lin Ma @ 2016-11-07 16:25 UTC (permalink / raw) To: qemu-devel, imammedo, mst Ping. >>> Lin Ma <lma@suse.com> 2016/9/6 星期二 下午 4:28 >>> 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 <lma@suse.com> --- 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}; + table_str_field_count = sizeof(tables_str_field_offset) / \ + sizeof(tables_str_field_offset[0]); + 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; + } + + for (i = 0; i < table_str_field_count; i++) { + if (*(uint8_t *)(smbios_tables + tables_str_field_offset[i]) > 0) { + terminator_count = 1; + break; + } + } + smbios_tables_len += size; + smbios_tables_len += terminator_count; if (size > smbios_table_max) { smbios_table_max = size; } 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] 答复： [PATCH] smbios: Add 1 terminator if there is any string field defined in given table. 2016-11-07 16:25 ` [Qemu-devel] 答复： " Lin Ma @ 2016-11-07 16:31 ` Daniel P. Berrange 2016-11-07 16:45 ` Lin Ma 0 siblings, 1 reply; 7+ messages in thread From: Daniel P. Berrange @ 2016-11-07 16:31 UTC (permalink / raw) To: Lin Ma; +Cc: qemu-devel, imammedo, mst On Mon, Nov 07, 2016 at 09:25:37AM -0700, Lin Ma wrote: > Ping. > > >>> Lin Ma <lma@suse.com> 2016/9/6 星期二 下午 4:28 >>> > 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 <lma@suse.com> > --- > 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}; > + table_str_field_count = sizeof(tables_str_field_offset) / \ > + sizeof(tables_str_field_offset[0]); > + 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; > + } > + > + for (i = 0; i < table_str_field_count; i++) { > + if (*(uint8_t *)(smbios_tables + tables_str_field_offset[i]) > 0) { > + terminator_count = 1; > + break; > + } > + } > + > smbios_tables_len += size; > + smbios_tables_len += terminator_count; > if (size > smbios_table_max) { > smbios_table_max = size; > } Code identation is this patch looks totally mangled. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :| ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] 答复： [PATCH] smbios: Add 1 terminator if there is any string field defined in given table. 2016-11-07 16:31 ` Daniel P. Berrange @ 2016-11-07 16:45 ` Lin Ma 0 siblings, 0 replies; 7+ messages in thread From: Lin Ma @ 2016-11-07 16:45 UTC (permalink / raw) To: Daniel P. Berrange; +Cc: qemu-devel, imammedo, mst >>> "Daniel P. Berrange" berrange@redhat.com> 2016/11/8 星期二 上午 12:31 >> ( mailto:berrange@redhat.com) ...... >> > >Code identation is this patch looks totally mangled. > How about this one: (From the code style's perspective, it should be no big problem) http://lists.nongnu.org/archive/html/qemu-devel/2016-09/msg01024.html Thanks, Lin ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] smbios: Add 1 terminator if there is any string field defined in given table. 2016-09-06 8:28 [Qemu-devel] [PATCH] smbios: Add 1 terminator if there is any string field defined in given table Lin Ma 2016-11-07 16:25 ` [Qemu-devel] 答复： " Lin Ma @ 2016-11-10 15:09 ` Michael S. Tsirkin 2016-11-10 17:18 ` Laszlo Ersek 1 sibling, 1 reply; 7+ messages in thread From: Michael S. Tsirkin @ 2016-11-10 15:09 UTC (permalink / raw) To: Lin Ma; +Cc: imammedo, qemu-devel, famz, lersek 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 <lma@suse.com> Seems to make sense superficially Acked-by: Michael S. Tsirkin <mst@redhat.com> Fam, would you like to take this? > --- > 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}; > + table_str_field_count = sizeof(tables_str_field_offset) / \ > + sizeof(tables_str_field_offset[0]); > + 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; > + } > + > + for (i = 0; i < table_str_field_count; i++) { > + if (*(uint8_t *)(smbios_tables + tables_str_field_offset[i]) > 0) { > + terminator_count = 1; > + break; > + } > + } > + > smbios_tables_len += size; > + smbios_tables_len += terminator_count; > if (size > smbios_table_max) { > smbios_table_max = size; > } > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] smbios: Add 1 terminator if there is any string field defined in given table. 2016-11-10 15:09 ` [Qemu-devel] " Michael S. Tsirkin @ 2016-11-10 17:18 ` Laszlo Ersek 2016-11-10 17:31 ` Laszlo Ersek 0 siblings, 1 reply; 7+ messages in thread From: Laszlo Ersek @ 2016-11-10 17:18 UTC (permalink / raw) To: Michael S. Tsirkin, Lin Ma; +Cc: imammedo, qemu-devel, famz 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 <lma@suse.com> > > Seems to make sense superficially > > Acked-by: Michael S. Tsirkin <mst@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:) >> 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] smbios: Add 1 terminator if there is any string field defined in given table. 2016-11-10 17:18 ` Laszlo Ersek @ 2016-11-10 17:31 ` Laszlo Ersek 0 siblings, 0 replies; 7+ messages in thread From: Laszlo Ersek @ 2016-11-10 17:31 UTC (permalink / raw) To: Michael S. Tsirkin, Lin Ma; +Cc: imammedo, qemu-devel, famz 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 <lma@suse.com> >> >> Seems to make sense superficially >> >> Acked-by: Michael S. Tsirkin <mst@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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-11-10 17:31 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-09-06 8:28 [Qemu-devel] [PATCH] smbios: Add 1 terminator if there is any string field defined in given table Lin Ma 2016-11-07 16:25 ` [Qemu-devel] 答复： " Lin Ma 2016-11-07 16:31 ` Daniel P. Berrange 2016-11-07 16:45 ` Lin Ma 2016-11-10 15:09 ` [Qemu-devel] " Michael S. Tsirkin 2016-11-10 17:18 ` Laszlo Ersek 2016-11-10 17:31 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).