Messages in this thread Patch in this message | | | Date | Mon, 09 May 2011 17:13:36 +0200 | From | Timo Warns <> | Subject | Re: [Stable-review] [patch 35/38] fs/partitions/ldm.c: fix oops caused by corrupted partition table |
| |
Am 07.05.2011 04:24, schrieb Ben Hutchings: > On Thu, 2011-05-05 at 17:11 -0700, Greg KH wrote: >> 2.6.38-stable review patch. If anyone has any objections, please let us know. >> >> ------------------ >> >> From: Timo Warns <Warns@pre-sense.de> >> >> commit c340b1d640001c8c9ecff74f68fd90422ae2448a upstream. >> [...] > > I don't think this actually fixes the vulnerability, and I don't think > this code works at all. > > [...] > >> + if (rec >= num) { >> + ldm_error("REC value (%d) exceeds NUM value (%d)", rec, num); >> + return false; >> + } > > This is fine for the first fragment we find, when we allocate memory > based on 'num'. However, when we add another fragment, we need to > compare with f->num. So there still seems to be the possibility of a > buffer overflow.
Yes, I agree. I have missed this one. Please consider the attached patch.
>> [...] >> memcpy (f->data+rec*(size-VBLK_SIZE_HEAD)+VBLK_SIZE_HEAD, data, size); >> >> return true; > > The offset used for the destination means that the first VBLK_SIZE_HEAD > bytes of f->data are never initialised! > > I suspect (without any knowledge of LDM) that the intent was to use the > header from the first fragment and drop it from the subsequent > fragments, like this: > > if (rec == 0) > memcpy(f->data, data, VBLK_SIZE_HEAD); > data += VBLK_SIZE_HEAD; > size -= VBLK_SIZE_HEAD; > memcpy(f->data + VBLK_SIZE_HEAD + rec * size, data, size);
The patch that I provided preserves the original behaviour. Hence, I would like to pass this issue to Richard. Richard, could you comment on this?
Cheers, Timo --- linux-2.6.38.5-b/fs/partitions/ldm.c 2011-05-02 18:30:53.000000000 +0200 +++ linux-2.6.38.5-a/fs/partitions/ldm.c 2011-05-09 17:09:07.000000000 +0200 @@ -1299,6 +1299,11 @@ BUG_ON (!data || !frags); + if (size < 2 * VBLK_SIZE_HEAD) { + ldm_error("Value of size is to small."); + return false; + } + group = get_unaligned_be32(data + 0x08); rec = get_unaligned_be16(data + 0x0C); num = get_unaligned_be16(data + 0x0E); @@ -1326,6 +1331,12 @@ list_add_tail (&f->list, frags); found: + if (rec >= f->num) { + ldm_error ("REC value (%d) exceeds NUM value (%d)", rec, f->num); + f->map &= 0x7F; /* Mark the group as broken */ + return false; + } + if (f->map & (1 << rec)) { ldm_error ("Duplicate VBLK, part %d.", rec); f->map &= 0x7F; /* Mark the group as broken */ @@ -1334,10 +1345,9 @@ f->map |= (1 << rec); - if (num > 0) { - data += VBLK_SIZE_HEAD; - size -= VBLK_SIZE_HEAD; - } + data += VBLK_SIZE_HEAD; + size -= VBLK_SIZE_HEAD; + memcpy (f->data+rec*(size-VBLK_SIZE_HEAD)+VBLK_SIZE_HEAD, data, size); return true; | |