Message-ID: <20150428162807.37181b45@endymion.delvare>
Date:	Tue, 28 Apr 2015 16:28:07 +0200
From:	Jean Delvare <jdelvare@...e.de>
To:	"Ivan.khoronzhuk" <ivan.khoronzhuk@...ballogic.com>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Roy Franz <roy.franz@...aro.org>,
	Matt Fleming <matt.fleming@...el.com>,
	Ard Biesheuvel <ard.biesheuvel@...aro.org>
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@...e.de>
> > Cc: Ivan Khoronzhuk <ivan.khoronzhuk@...ballogic.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists