[SeaBIOS] [PATCH] display_uuid(): fix incomplete check after the loop

[Amos Kong]

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 at 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

-- 
		Amos.