Reproduced with latest upstream.

The DMI tables are constructed by SeaBIOS.  qemu-kvm sends it the stuff set with -smbios as a blob via fw_cfg entry FW_CFG_SMBIOS_ENTRIES a.k.a. QEMU_CFG_SMBIOS_ENTRIES.  I examined the blob, and it looks good to me.

We need to examine what SeaBIOS does with the blob.

Created attachment 851010 [details]
Patch to help examine the DMI blob built by qemu-kvm

Output produced by the patch with the reporter's -smbios option:

SMBIOS table
	110 bytes
	7 entries
entry#0
	type 0 sz 16
	field type 1 offset 4
	: 0000:  44 65 6c 6c  20 49 6e 63  2e 00
entry#1
	type 0 sz 28
	field type 1 offset 5
	: 0000:  4f 70 74 69  50 6c 65 78  20 37 34 30  20 45 6e 68
	: 0010:  61 6e 63 65  64 00
entry#2
	type 0 sz 7
	field type 1 offset 6
	: 0000:  00
entry#3
	type 0 sz 14
	field type 1 offset 7
	: 0000:  4a 36 37 47  4d 32 58 00
entry#4
	type 0 sz 10
	field type 1 offset 25
	: 0000:  4b 56 4d 00
entry#5
	type 0 sz 11
	field type 1 offset 26
	: 0000:  56 49 52 54  00
entry#6
	type 0 sz 22
	field type 1 offset 8
	: 0000:  44 45 4c 4c  36 00 10 37  80 47 ca c0  4f 4d 32 58

Note: "offset" is in struct smbios_type_1.

Output with offsets explained, and hexdumps decoded:

SMBIOS table
	110 bytes
	7 entries
entry#0
	type 0 sz 16
	field type 1 offset 4				vendor_str
	"Dell Inc."
entry#1
	type 0 sz 28
	field type 1 offset 5				product_name_str
	"OptiPlex 740 Enhanced"
entry#2
	type 0 sz 7
	field type 1 offset 6				version_str
	""
entry#3
	type 0 sz 14
	field type 1 offset 7				serial_number_str
	"J67GM2X"
entry#4
	type 0 sz 10
	field type 1 offset 25				sku_number_str
	"KVM"
entry#5
	type 0 sz 11
	field type 1 offset 26				family_str
	"VIRT"
entry#6
	type 0 sz 22					uuid
	field type 1 offset 8
	44454C4C-3600-1037-8047-CAC04F4D3258

Bug is in seabios most likely.

Zero length version string causes trouble.
Workaround: Use space instead, i.e. version=' '.

Hmm, well, I can see where this is coming from.  version='' is used to clear the default version entry we have added recently.  So, the question is what qemu should do in this case?  It would make sense to not add version to the blob then (i.e. don't generate entry #2 in comment #4) I think.  Markus?

Can empty strings be legitimate values in DMI?

If not, then we can special-case empty string values in qemu-kvm to delete the field instead.  But only then.

Should SeaBIOS's handling of values be more robust?

Found this in the seabios sources:

        /* Entries end with a double NULL char, if there's a string at
         * the end (length is greater than formatted length), the string
         * terminator provides the first NULL. */

So I think what happens is that seabios correctly generates a zero-length string, but with the terminating zero from the string placed before the zero-length string we'll get as side effect an end-of-entry marker before the actual end of the entry.  Which explains why dmidecode throws a "bad index" error.

I think that also pretty much implies that dmi entries can't have zero-length strings and qemu should not generate them.

All right, I looked it up in the System Management BIOS Reference
Specification 2.8.0[*].  Section 6 describes the SMBIOS Entry Point
Structure.  It contains a sequence of SMBIOS structures.

Each SMBIOS structure consists of a formatted section followed by an
unformatted section.

A formatted section starts with a header, and the header contains the
length of the formatted section.

The unformatted section is terminated by two null bytes.

It contains null-terminated strings one after the other, followed by
an additional null byte.

The formatted section refers to these strings by ordinal number,
starting at one.  Null means "no string".

An empty unformatted is encoded as two null bytes.

If you put an empty string in an unformatted section, you get the
following cases:

* The only string

  Two null bytes, indistinguishable from empty unformatted section.
  Consumers may well consider its ordinal number one as invalid.

* First string, more strings follow

  Null byte, then more strings.  This may work.

* Neither first nor last string

  We get two null bytes "in the middle".  Bad.

* Last string, but not first

  We get three null bytes.  Bad.

I'm now inclined to agree with Gerd: empty strings aren't meant to be
in the unformatted section.

We got an empty string there, because qemu-kvm sent one to SeaBIOS,
and SeaBIOS put it in an unformatted section, corrupting it.

Bug: SeaBIOS produces a corrupt unformatted section when it gets an
empty string from qemu-kvm.  It shouldn't do that.  It should either
reject the empty string as bad input, or interpret it as "no string"
(string number 0).

Now what about qemu-kvm?  Alternatives:

1. Reject user's request for empty string.

   Values are either unset or non-empty strings.  Only non-empty
   strings are sent to SeaBIOS.

2. When user requests empty string, unset value.  This lets you cancel
   a qemu-kvm default value, or a value you set yourself before.

   Again, values are either unset or non-empty strings, and only
   non-empty strings are sent to SeaBIOS.

   You cannot cancel SeaBIOS default values.

3. When user requests empty string, accept it.

   Values are either unset (never sent, which means you get the
   SeaBIOS default value), or (possibly empty) strings.  SeaBIOS is to
   interpret an empty string as "cancel your default value, if any".

I prefer 3.  But think this should be discusses on the SeaBIOS list,
cc: qemu-devel.

Gerd, what do you think?


[*] http://www.dmtf.org/sites/default/files/standards/documents/DSP0134_2.8.0.pdf

SeaBIOS sets a few fields only (manufacturer + product name for type 1), and these you most likely want fill with useful information anyway if you care about the smbios entries.

So, is there any practical use case for (3)?

Otherwise I'd go for (2), especially as it is a simple one-liner patch:

--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -212,7 +212,7 @@ static void smbios_add_field(int type, int offset, const void *data, size_t len)
 
 static void smbios_maybe_add_str(int type, int offset, const char *data)
 {
-    if (data) {
+    if (data && strlen(data)) {
         smbios_add_field(type, offset, data, strlen(data) + 1);
     }
 }

I'm preferring (3) not because I have a use case, only because it
feels more regular to me, and doesn't look harder.

Let's examine the difference between (2) and (3).

For (2), we need to

  1. Change qemu-kvm to never send SMBIOS_FIELD_ENTRY with an empty
     string value.  This masks the SeaBIOS bug.

  2. Fix SeaBIOS to ignore SMBIOS_FIELD_ENTRY with an empty string
     value.  Optional.

  We can then get corrupt SMBIOS structures only when both qemu-kvm
  and SeaBIOS predate their respective change.

For (3), we need to

  1. Do nothing to qemu-kvm.

  2. Fix SeaBIOS to encode an empty string value as string number 0,
     and not store value.

  We can then get corrupt SMBIOS structures only with an unfixed
  SeaBIOS.

What do you think?


Instead of your one-liner, I'd do (2) in qemu-kvm like this:

diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index e8f41ad..4d16583 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -213,6 +213,7 @@ static void smbios_add_field(int type, int offset, const void *data, size_t len)
 static void smbios_maybe_add_str(int type, int offset, const char *data)
 {
     if (data) {
+        assert(*data);
         smbios_add_field(type, offset, data, strlen(data) + 1);
     }
 }
@@ -287,7 +288,7 @@ static void save_opt(const char **dest, QemuOpts *opts, const char *name)
     const char *val = qemu_opt_get(opts, name);
 
     if (val) {
-        *dest = val;
+        *dest = *val ? val : NULL;
     }
 }
 

Sketch of SeaBIOS fix for (3):

diff --git a/src/fw/smbios.c b/src/fw/smbios.c
index affb9be..bfd542a 100644
--- a/src/fw/smbios.c
+++ b/src/fw/smbios.c
@@ -133,20 +133,23 @@ get_external(int type, char **p, unsigned *nr_structs,
     do {                                                                \
         size = get_field(type, offsetof(struct smbios_type_##type,      \
                                         field), end);                   \
-        if (size > 0) {                                                 \
-            end += size;                                                \
+        if (size == 1) {						\
+            p->field = 0;						\
+        if (size > 1) {                                                 \
+            end += size;						\
+            p->field = ++str_index;					\
         } else {                                                        \
             memcpy(end, def, sizeof(def));                              \
             end += sizeof(def);                                         \
+            p->field = ++str_index;					\
         }                                                               \
-        p->field = ++str_index;                                         \
     } while (0)
 
 #define load_str_field_or_skip(type, field)                             \
     do {                                                                \
         size = get_field(type, offsetof(struct smbios_type_##type,      \
                                         field), end);                   \
-        if (size > 0) {                                                 \
+        if (size > 1) {                                                 \
             end += size;                                                \
             p->field = ++str_index;                                     \
         } else {                                                        \

http://www.seabios.org/pipermail/seabios/2014-January/007564.html

We fixed this in upstream SeaBIOS (commit 344496f), as sketched in comment#13.  Reassigning accordingly.

Backport posted.

Fix included in seabios-1.7.2.2-11.el7

Reproduce this bug as follow version:
Host:
# uname -r
3.10.0-79.el7.x86_64
# rpm -q qemu-kvm
qemu-kvm-1.5.3-45.el7.x86_64
# rpm -q seabios
seabios-1.7.2.2-10.el7.x86_64

Guest:rhel6 

Steps
1)boot guest with 
...-smbios type=1,manufacturer=Dell Inc.,product=OptiPlex 740 Enhanced,uuid=44454C4C-3600-1037-8047-CAC04F4D3258,serial=J67GM2X,version=,family=VIRT,sku=KVM -bios /usr/share/seabios/bios.bin ..

2.After guest boot up ,in guest
# dmidecode -t 1
# dmidecode 2.11
SMBIOS 2.4 present.

Handle 0x0100, DMI type 1, 27 bytes
System Information
	Manufacturer: Dell Inc.
	Product Name: OptiPlex 740 Enhanced
	Version: <BAD INDEX>
	Serial Number: <BAD INDEX>
	UUID: 44454C4C-3600-1037-8047-CAC04F4D3258
	Wake-up Type: Power Switch
	SKU Number: <BAD INDEX>
	Family: <BAD INDEX>

Wrong DMI structures count: 10 announced, only 4 decoded.
Wrong DMI structures length: 301 bytes announced, structures occupy 418 bytes.


Verify this bug as follow version:

HOst:
# uname -r
3.10.0-79.el7.x86_64
# rpm -q qemu-kvm
qemu-kvm-1.5.3-45.el7.x86_64
# rpm -q seabios
seabios-1.7.2.2-11.el7.x86_64

Steps as same as reproduce

Results:dmi can be decode without any error.

# dmidecode -t 1
# dmidecode 2.11
SMBIOS 2.4 present.

Handle 0x0100, DMI type 1, 27 bytes
System Information
	Manufacturer: Dell Inc.
	Product Name: OptiPlex 740 Enhanced
	Version: Not Specified
	Serial Number: J67GM2X
	UUID: 44454C4C-3600-1037-8047-CAC04F4D3258
	Wake-up Type: Power Switch
	SKU Number: KVM
	Family: VIRT

Addtional info:
1) i also test use other "values",work well.EG:
...-smbios type=1,manufacturer='Dell Inc.',product='OptiPlex 740 Enhanced',uuid='44454C4C-3600-1037-8047-CAC04F4D3258',serial='J67GM2X',version='langfang',family='*VIRT*',sku='*KVM*'...
# dmidecode -t 1
# dmidecode 2.12
SMBIOS 2.4 present.

Handle 0x0100, DMI type 1, 27 bytes
System Information
	Manufacturer: Dell Inc.
	Product Name: OptiPlex 740 Enhanced
	Version: langfang
	Serial Number: J67GM2X
	UUID: 44454C4C-3600-1037-8047-CAC04F4D3258
	Wake-up Type: Power Switch
	SKU Number: *KVM*
	Family: *VIRT*

2)Test rhel7 guest,work well

According to above test ,this bug has been fixed.

This request was resolved in Red Hat Enterprise Linux 7.0.

Contact your manager or support representative in case you have further questions about the request.