Suspect loop in dmi_scan_machine()

Suspect loop in dmi_scan_machine()

[Jean Delvare]

Hi Ben,

I am looking at this commit of yours:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/firmware/dmi_scan.c?id=79bae42d51a5d498500c890c19ef76df41d2bf59

and am a little worried about the for loop in dmi_scan_machine()
(non-EFI case):

* The first iteration looks wrong, as you're resetting the lower half of
buf to 0 instead of initializing it with the 16 first bytes of p. So if
a system as its SMBIOS entry point at 0xF0000, your code won't see it.
Thankfully it will get the DMI entry point, but if the version is in the
SMBIOS entry point, it will be missed.

* The last iteration looks wrong too, as you are reading from offsets
0x10000-0x1000F of an iomap area of size 0x10000, i.e. beyond the end of
the area. You'll hit that on a machine without SMBIOS/DMI. I'm not sure
what exactly happens when calling memcpy_fromio() on an unmapped area
but I'm reasonably certain this is bad and shouldn't be attempted.

Also, the code only looks for the DMI entry point in the second half of
the buffer, so it would also miss a legacy DMI entry point at 0xF0000.

Thoughts?

-- 
Jean Delvare
Suse L3

Re: Suspect loop in dmi_scan_machine()

[Ben Hutchings]

On Mon, Jul 08, 2013 at 05:49:14PM +0200, Jean Delvare wrote:
> Hi Ben,
> 
> I am looking at this commit of yours:
> 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/firmware/dmi_scan.c?id=79bae42d51a5d498500c890c19ef76df41d2bf59
> 
> and am a little worried about the for loop in dmi_scan_machine()
> (non-EFI case):
[...]

I don't see any of the bugs you describe.  Let me explain what
I probably should have put in a comment:

We want to find a DMI header at [0xf0000, 0xffff0] and possibly an
SMBIOS header 16 bytes before that.  buf contains a copy of the 32
bytes centred at p.  On the first iteration p - 16 is out of range, so
the first 16 bytes of the buffer are filled with zeroes.

Does that address your concerns?  If not then please explain precisely
how this loop can go wrong.

Ben.

-- 
Ben Hutchings
We get into the habit of living before acquiring the habit of thinking.
                                                              - Albert Camus

Re: Suspect loop in dmi_scan_machine()

[Jean Delvare]

Hi Ben,

Le Monday 08 July 2013 à 17:33 +0100, Ben Hutchings a écrit :
> On Mon, Jul 08, 2013 at 05:49:14PM +0200, Jean Delvare wrote:
> > I am looking at this commit of yours:
> > 
> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/firmware/dmi_scan.c?id=79bae42d51a5d498500c890c19ef76df41d2bf59
> > 
> > and am a little worried about the for loop in dmi_scan_machine()
> > (non-EFI case):
> [...]
> 
> I don't see any of the bugs you describe.  Let me explain what
> I probably should have put in a comment:
> 
> We want to find a DMI header at [0xf0000, 0xffff0] and possibly an
> SMBIOS header 16 bytes before that.  buf contains a copy of the 32
> bytes centred at p.  On the first iteration p - 16 is out of range, so
> the first 16 bytes of the buffer are filled with zeroes.
> 
> Does that address your concerns?  If not then please explain precisely
> how this loop can go wrong.

Wow. I'm not sure what I was up to yesterday evening but I definitely
should have waited a bit before posting. I read your code again this
morning and it is completely correct, and the concerns I raised
yesterday were plain crap. I suppose your code was too smart for my
tired brain...

Sorry for the noise,
-- 
Jean Delvare
Suse L3