diff options
author | Bill Richardson <wfrichar@chromium.org> | 2011-11-10 17:08:23 -0800 |
---|---|---|
committer | Bill Richardson <wfrichar@chromium.org> | 2011-11-14 10:45:56 -0800 |
commit | 931728a003a417bcb29cc1c203ce36d23feee9e8 (patch) | |
tree | e3d49f48a6ccc6d2edc838599069fd76a3180ee8 | |
parent | 01bf572be8a04b2c4c32b9c6118a084061b42b48 (diff) | |
download | vboot-931728a003a417bcb29cc1c203ce36d23feee9e8.tar.gz |
Address security concerns for vboot_audio.c
Based on the compile-time constants, I don't think we were in any danger,
but I've added the checks anyway. It never hurts to be certain!
BUG=chromium-os:22786
TEST=none
Change-Id: I469dda19b4589e484a41ca9bae1e107787f3cf4d
Reviewed-on: https://gerrit.chromium.org/gerrit/11516
Tested-by: Bill Richardson <wfrichar@chromium.org>
Reviewed-by: Gaurav Shah <gauravsh@chromium.org>
-rw-r--r-- | firmware/lib/vboot_audio.c | 27 |
1 files changed, 25 insertions, 2 deletions
diff --git a/firmware/lib/vboot_audio.c b/firmware/lib/vboot_audio.c index b689bbd4..8841a5e7 100644 --- a/firmware/lib/vboot_audio.c +++ b/firmware/lib/vboot_audio.c @@ -13,6 +13,10 @@ #include "vboot_audio_private.h" #include "vboot_common.h" +/* BIOS doesn't have /usr/include */ +#ifndef UINT_MAX +#define UINT_MAX 4294967295U /* 0xffffffff */ +#endif #define DEV_LOOP_TIME 10 /* Minimum note granularity in msecs */ @@ -47,7 +51,7 @@ static void VbGetDevMusicNotes(VbAudioContext *audio, int use_short) { VbDevMusicNote *notebuf = 0; VbDevMusicNote *builtin = 0; VbDevMusic *hdr = CUSTOM_MUSIC_NOTES; - uint32_t maxsize = CUSTOM_MUSIC_MAXSIZE; + uint32_t maxsize = CUSTOM_MUSIC_MAXSIZE; /* always <= flash size (8M) */ uint32_t maxnotes, mysum, mylen, i; uint64_t on_loops, total_loops, min_loops; uint32_t this_loops; @@ -77,6 +81,9 @@ static void VbGetDevMusicNotes(VbAudioContext *audio, int use_short) { goto nope; } + /* How many notes will fit in the flash region? One more than you'd think, + * because there's one note in the header itself. + */ maxnotes = 1 + (maxsize - sizeof(VbDevMusic)) / sizeof(VbDevMusicNote); if (hdr->count == 0 || hdr->count > maxnotes) { VBDEBUG(("VbGetDevMusicNotes: count=%d maxnotes=%d\n", @@ -84,6 +91,16 @@ static void VbGetDevMusicNotes(VbAudioContext *audio, int use_short) { goto nope; } + /* CUSTOM_MUSIC_MAXSIZE can't be larger than the size of the flash (around 8M + * or so) so this isn't really necessary, but let's be safe anyway. + */ + if ((sizeof(VbDevMusicNote) > UINT_MAX / hdr->count) || + (sizeof(hdr->count) > UINT_MAX - hdr->count * sizeof(VbDevMusicNote))) { + VBDEBUG(("VbGetDevMusicNotes: count=%d, just isn't right\n")); + goto nope; + } + + /* Now we know this won't overflow */ mylen = (uint32_t)(sizeof(hdr->count) + hdr->count * sizeof(VbDevMusicNote)); mysum = Crc32(&(hdr->count), mylen); @@ -128,7 +145,13 @@ static void VbGetDevMusicNotes(VbAudioContext *audio, int use_short) { goto nope; } - /* Okay, it looks good. Allocate the space (plus one) and copy it over */ + /* One more check, just to be paranoid. */ + if (hdr->count > (UINT_MAX / sizeof(VbDevMusicNote) - 1)) { + VBDEBUG(("VbGetDevMusicNotes: they're all out to get me!\n")); + goto nope; + } + + /* Okay, it looks good. Allocate the space (plus one) and copy it over. */ notebuf = VbExMalloc((hdr->count + 1) * sizeof(VbDevMusicNote)); Memcpy(notebuf, hdr->notes, hdr->count * sizeof(VbDevMusicNote)); count = hdr->count; |