On Fri, Dec 21, 2012 at 10:47:34AM +0100, Laszlo Ersek wrote:
On 12/21/12 04:06, Amos Kong wrote:
In the v2->v3 change of what would become commit 37676f83 http://www.seabios.org/pipermail/seabios/2012-December/005166.html, the defense against an initial "addr > end" condition ("wraparound") was erroneously loosened.
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Acked-by: Amos Kong akong@redhat.com
src/smbios.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
| void | display_uuid(void) | { | u32 addr, end; | u8 *uuid; | u8 empty_uuid[16] = { 0 }; | | if (SMBiosAddr == NULL) | return; | | addr = SMBiosAddr->structure_table_address; | end = addr + SMBiosAddr->structure_table_length;
structure_table_length is unsigned, end always >= addr here.
That's the point exactly:
| /* the following takes care of any initial wraparound too */ | while (addr < end) {
the "initial wraparound" referred to in the comment is when "structure_table_address" (an u32) is so big (= garbled) that even adding a small "structure_table_length" wraps it around (sth. like (UINT32_MAX-10)+30). This can result in "addr > end" and cause the while loop not to be entered (preparing for this was my explicit intent).
What I messed up in v2->v3 and would like to address with this patch is the after-loop exit check, for when such a wraparound happens before the loop.
Got it, thanks.
| /* parsing finished, UUID not found */ | if (addr == end) | return;
This only works if we enter the loop (and your analysis precisely describes what happens then).
(See also the new comment "parsing finished or skipped entirely, UUID not found".)
In practice, if *SMBiosAddr is so messed up as to trigger the initial wraparound, everything might be lost already. But I don't like to rely on that; all parsers should be 100% paranoid as a principle.
Thanks! Laszlo