Bug 1052837
Summary: | The wrong DMI structures could not be decoded while booting vm with -smbios params | ||||||
---|---|---|---|---|---|---|---|
Product: | Red Hat Enterprise Linux 7 | Reporter: | xhan | ||||
Component: | seabios | Assignee: | Gerd Hoffmann <kraxel> | ||||
Status: | CLOSED CURRENTRELEASE | QA Contact: | Virtualization Bugs <virt-bugs> | ||||
Severity: | medium | Docs Contact: | |||||
Priority: | medium | ||||||
Version: | 7.0 | CC: | acathrow, areis, armbru, flang, hhuang, huding, juzhang, kraxel, michen, mrezanin, rhod, virt-maint, xfu, xwei | ||||
Target Milestone: | rc | ||||||
Target Release: | --- | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Whiteboard: | |||||||
Fixed In Version: | seabios-1.7.2.2-11.el7 | Doc Type: | Bug Fix | ||||
Doc Text: | Story Points: | --- | |||||
Clone Of: | Environment: | ||||||
Last Closed: | 2014-06-13 09:55:41 UTC | Type: | Bug | ||||
Regression: | --- | Mount Type: | --- | ||||
Documentation: | --- | CRM: | |||||
Verified Versions: | Category: | --- | |||||
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||
Cloudforms Team: | --- | Target Upstream Version: | |||||
Embargoed: | |||||||
Attachments: |
|
Description
xhan
2014-01-14 07:41:05 UTC
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 { \ 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. |