Re: [SeaBIOS] [PATCH] display_uuid(): fix incomplete check after the loop
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 ak...@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.
___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH] display_uuid(): fix incomplete check after the loop
On Tue, Dec 18, 2012 at 05:11:39AM +0100, Laszlo Ersek 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. Thanks - I pushed this change. -Kevin ___ SeaBIOS mailing list SeaBIOS@seabios.org http://www.seabios.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH] display_uuid(): fix incomplete check after the loop
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));
--
1.7.1
| 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;
--
Amos.
___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios
