Messages in this thread | | | Date | Tue, 28 Apr 2015 16:28:07 +0200 | From | Jean Delvare <> | Subject | Re: [PATCH] firmware: dmi_scan: Trim DMI table length before exporting it |
| |
Hi Ivan, Ard,
On Mon, 27 Apr 2015 18:17:15 +0300, Ivan.khoronzhuk wrote: > On 26.04.15 23:47, Jean Delvare wrote: > > The SMBIOS v3 entry points specify a maximum length for the DMI table, > > not the exact length. Thus there may be garbage after the end-of-table > > marker, which we don't want to export to user-space. Adjust dmi_len > > when we find the end-of-table marker, so that only the actual table > > payload is exported. > > According to above it seems that change doesn't affect SMBIOS < 3.
Good point.
When implementing support for the v3 64-bit entry point format, Ard decided we should stop at the first end-of-table marker. The only v3 implementation I currently have access to (AMD Seattle system) suggests this is the right thing to do for tables behind a v3 64-bit entry point.
There is a small risk however that this could result in regressions for older machines. I checked my collection of DMI tables and a few of them have an end-of-table marker followed by OEM or unknown structures, optionally followed by another end-of-table marker. Drivers like acer-dmi, dell-dmi or dell-laptop access OEM DMI structures. And Acer is one of the brands where I spotted such an OEM structure after a first end-of-table marker. It was a different type number, so I have no evidence of an actual regression at this point, but it could be the case, with either in-tree or out-of-tree drivers calling dmi_walk(). So I believe that we should only stop walking the DMI table at end-of-table marker for tables behind a v3 entry point. FWIW this is what dmidecode does. I'll send a patch.
Surprisingly, the SMBIOS specification version 3.0.0 still indicates that "a system implementation should use the end-of-table indicator in a manner similar to the Inactive (Type 126) structure type; the structure table is still reported as a fixed-length, and the entire length of the table is still indexable." This seems contradictory with the table length in the entry point being a maximum rather than the actual length. Maybe this only applies to legacy 32-bit entry points and not to v3 64-bit entry points. Or the DMTF forgot to remove this from the previous version of the specification.
We may still trim legacy tables, but based on other conditions, rather than hitting the end-of-table marker. See below.
> > > > Signed-off-by: Jean Delvare <jdelvare@suse.de> > > Cc: Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com> > > --- > > drivers/firmware/dmi_scan.c | 15 ++++++++++----- > > 1 file changed, 10 insertions(+), 5 deletions(-) > > > > --- linux-4.1-rc0.orig/drivers/firmware/dmi_scan.c 2015-04-26 18:56:15.657111185 +0200 > > +++ linux-4.1-rc0/drivers/firmware/dmi_scan.c 2015-04-26 22:42:59.183547584 +0200 > > @@ -108,15 +108,19 @@ static void dmi_decode_table(u8 *buf, > > if (data - buf < dmi_len - 1) > > decode(dm, private_data); > > > > + data += 2; > > + i++; > > + > > /* > > * 7.45 End-of-Table (Type 127) [SMBIOS reference spec v3.0.0] > > */ > > if (dm->type == DMI_ENTRY_END_OF_TABLE) > > break; > > - > > - data += 2; > > - i++; > > } > > + > > + /* Trim DMI table length if needed */ > > + if (dmi_len > data - buf) > > + dmi_len = data - buf; > > Why is it checked here? Wouldn't be better to simply assign: dmi_len = > data - buf;
If the table is corrupted, it is possible that data - buf is greater than dmi_len. Imagine that the header of the "last" DMI structure fits in dmi_len, but the structure's length (dm->length) is wrong and goes beyond dmi_len. We will not attempt to decode that structure, but data still "points" beyond dmi_len when we leave the loop. We definitely do not want to increase dmi_len from its original value, the condition is the safety which guarantees that this never happens.
If we want to always trim the table to the end of the last DMI structure which we were able to decode, then we need to modify the code to keep track of it. "data" isn't that. I don't know if it's worth it.
> I guess it's supposed to not trim table in case of exit by i<dmi_num or > even !dmi_num.
dmi_num isn't an exit condition.
i == dmi_num is an exit condition. If we have decoded all the structures we were told were present, but there is more data left, then it could be that the rest is garbage, which would justify trimming the table length. It might also be that dmi_num was wrong and there are more entries to decode before the actual end of the table. However with the current implementation the kernel will ignore them, and so will dmidecode, so for consistency it would make sense to not expose the extra data to user-space.
My patch does exactly that, although I admit I did not realize it until now, and apparently neither did you ;-)
> But in this case, probably, it be better to do it in the cycle: > > if (dmi->type == DMI_ENTRY_END_OF_TABLE) { > dmi_len = data - buf; > break; > }
In fact I had it that way originally, then I thought it wouldn't hurt to have it in the common path, to cover for possible corner cases.
> just to avoid redundant check. > > But question is, what if we exit by i<dmi_num? > Wouldn't be better to trim buffer in such case also?
I think so, and I believe my code does that.
Thanks a lot for the review and questions, Ivan. Even if in the end I believe my patch is correct, you made me think more about the various cases which could happen, so I now have a better awareness of what I should be paying attention to.
-- Jean Delvare SUSE L3 Support
| |