Messages in this thread |  | | Date | Thu, 16 Apr 2015 16:46:30 +0800 | From | Zhenzhong Duan <> | Subject | Re: [PATCH] firmware: dmi_scan: Fix ordering of product_uuid |
| |
On 2015/4/16 15:09, Jean Delvare wrote: > Hi zduan, > > Thanks for your reply. > > Le Thursday 16 April 2015 à 14:22 +0800, Zhenzhong Duan a écrit : >> On 2015/4/15 17:02, Jean Delvare wrote: >>> In function dmi_present(), dmi_walk_early() calls dmi_table(), which >>> calls dmi_decode(), which ultimately calls dmi_save_uuid(). This last >>> function makes a decision based on the value of global variable >>> dmi_ver. The problem is that this variable is set right _after_ >>> dmi_walk_early() returns. So dmi_save_uuid() always sees dmi_ver == 0 >>> regardless of the actual version implemented. >>> >>> This causes /sys/class/dmi/id/product_uuid to always use the old >>> ordering even on systems implementing DMI/SMBIOS 2.6 or later, which >>> should use the new ordering. >>> >>> This is broken since kernel v3.8 for legacy DMI implementations and >>> since kernel v3.10 for SMBIOS 2 implementations. SMBIOS 3 >>> implementations with the 64-bit entry point are not affected. >>> >>> The first breakage does not matter much as in practice legacy DMI >>> implementations are always for versions older than 2.6, which is when >>> the UUID ordering changed. The second breakage is more problematic as >>> it affects the vast majority of x86 systems manufactured since 2009. >>> >>> Signed-off-by: Jean Delvare <jdelvare@suse.de> >>> Fixes: 9f9c9cbb6057 ("drivers/firmware/dmi_scan.c: fetch dmi version from SMBIOS if it exists") >> I think above line should be removed as dmi_ver is set before >> dmi_walk_early with the commit, see below clip. >> We did get right UUID order with SMBIOS 2.6 per customer test. > I bet your customers tested only with recent SMBIOS implementations that > have the _SM_ entry point. They did not test on systems with only legacy > _DMI_ entry points. As I said above, odds are that such systems would > implement a version of the specification older than 2.6 anyway, so the > bug wouldn't trigger. In fact, I have considered the fact that legacy DMI is always older than 2.6 when design. > > I agree that 9f9c9cbb6057 is not problematic in practice and this is why > I wrote that the fix is only needed for kernels v3.10+, not v3.8+. But I > think it is still interesting to document the first commit which > introduced the bug. I'm pretty sure that the second faulty commit would > not have been faulty if the first commit had been correct. After all, > that second commit aligned the _SM_ code path on the _DMI_ code path, > without realizing that the latter had a bug. Ok, just keep it > >> +static int __init smbios_present(const char __iomem *p) >> +{ >> + u8 buf[32]; >> + int offset = 0; >> + >> + memcpy_fromio(buf, p, 32); >> + if ((buf[5] < 32) && dmi_checksum(buf, buf[5])) { >> + dmi_ver = (buf[6] << 8) + buf[7]; > But look at the _DMI_ code path: > > static int __init dmi_present(const char __iomem *p) > { > (...) > if (dmi_walk_early(dmi_decode) == 0) { > if (dmi_ver) > pr_info("SMBIOS %d.%d present.\n", > dmi_ver >> 8, dmi_ver & 0xFF); > else { > dmi_ver = (buf[14] & 0xF0) << 4 | > (buf[14] & 0x0F); > pr_info("Legacy DMI %d.%d present.\n", > dmi_ver >> 8, dmi_ver & 0xFF); > } > dmi_dump_ids(); > return 0; > } > > Here dmi_ver may be set _after_ dmi_walk_early is called. > >>> Fixes: 79bae42d51a5 ("dmi_scan: refactor dmi_scan_machine(), {smbios,dmi}_present()") >>> Cc: Zhenzhong Duan <zhenzhong.duan@oracle.com> >>> Cc: Ben Hutchings <ben@decadent.org.uk> >>> Cc: Artem Savkov <artem.savkov@gmail.com> >>> Cc: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> >>> Cc: Matt Fleming <matt.fleming@intel.com> >>> Cc: stable@vger.kernel.org [v3.10+] >>> --- >>> drivers/firmware/dmi_scan.c | 7 ++++--- >>> 1 file changed, 4 insertions(+), 3 deletions(-) >>> >>> --- linux-4.0.orig/drivers/firmware/dmi_scan.c 2015-04-13 00:12:50.000000000 +0200 >>> +++ linux-4.0/drivers/firmware/dmi_scan.c 2015-04-15 10:24:37.556994240 +0200 >>> @@ -499,18 +499,19 @@ static int __init dmi_present(const u8 * >>> buf += 16; >>> >>> if (memcmp(buf, "_DMI_", 5) == 0 && dmi_checksum(buf, 15)) { >>> + if (smbios_ver) >>> + dmi_ver = smbios_ver; >>> + else >>> + dmi_ver = (buf[14] & 0xF0) << 4 | (buf[14] & 0x0F); >>> dmi_num = get_unaligned_le16(buf + 12); >>> dmi_len = get_unaligned_le16(buf + 6); >>> dmi_base = get_unaligned_le32(buf + 8); >>> >>> if (dmi_walk_early(dmi_decode) == 0) { >>> if (smbios_ver) { >>> - dmi_ver = smbios_ver; >>> pr_info("SMBIOS %d.%d present.\n", >>> dmi_ver >> 8, dmi_ver & 0xFF); >>> } else { >>> - dmi_ver = (buf[14] & 0xF0) << 4 | >>> - (buf[14] & 0x0F); >>> pr_info("Legacy DMI %d.%d present.\n", >>> dmi_ver >> 8, dmi_ver & 0xFF); >>> } >>> >>> >> The basic idea is right, but you ignore the case dmi_walk_early may >> fail, though looks impossible when bootup. >> >> Better to add below for robust. >> >> @@ -521,6 +521,6 @@ static int __init dmi_present(const u8 * >> >> return 0; >> } >> } >> + dmi_ver = 0; >> return 1; >> } >> > What is the value of this? dmi_ver will never be accessed after this > point anyway, as far as I can see. Same as above, future commit may not realize you bring this faulty when they want to use dmi_ver.
zduan
|  |