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.
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> --- 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.
| /* 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
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
-- Amos.
participants (2)
-
Amos Kong -
Laszlo Ersek