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

[Amos Kong]

Hi Laszlo,
> 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>
>
> src/smbios.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/smbios.c b/src/smbios.c
> index aaa99bc..23713a2 100644
> --- a/src/smbios.c
> +++ b/src/smbios.c
> @@ -569,8 +569,8 @@ display_uuid(void)
>          addr += 2;
>      }
> /* parsing finished, UUID not found */
> if (addr == end)
> /* parsing finished or skipped entirely, UUID not found */
> if (addr >= end)

It's a stricter check, but I didn't find it will happen.
>      return;
>
>  uuid = (u8 *)(addr + offsetof(struct smbios_type_1, uuid));

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

|     /* the following takes care of any initial wraparound too */
|     while (addr < end) {
|         const struct smbios_structure_header *hdr;
| 
|         /* partial structure header */
|         if (end - addr < sizeof(struct smbios_structure_header))
|             return;
| 
|         hdr = (struct smbios_structure_header *)addr;
| 
|         /* partial structure */
|         if (end - addr < hdr->length)
|             return;
| 
|         /* any Type 1 structure version will do that has the UUID */
|         if (hdr->type == 1 &&
|             hdr->length >= offsetof(struct smbios_type_1, uuid) + 16)
|             break;

In this point, it just passes the while condition check, and addr
doesn't change.

* addr < end

| 
|         /* done with formatted area, skip string-set */
|         addr += hdr->length;
| 
|         while (end - addr >= 2 &&
|                (*(u8 *)addr     != '\0' ||
|                 *(u8 *)(addr+1) != '\0'))
|             ++addr;
| 
|         /* structure terminator not found */
|         if (end - addr < 2)
|             return;

We have above check here, it means if it passes this check

end >= addr + 2
| 
|         addr += 2;

After above sentence, end >= addr,  there is only one condition that
while condition check fails:

* addr = end

|     }

The above analysis means that we can only get this point by two
conditions:
 * 'break'  (addr < end)
 * not pass while's condition check (addr >= end)

So the 'addr' will never be larger than 'end'.
However, add this stricter check is harmless.

|     /* parsing finished, UUID not found */
|     if (addr == end)
|         return;