Re: [Stable-review] [patch 35/38] fs/partitions/ldm.c: fix oops caused by corrupted partition table

[Timo Warns <warns@pre-sense.de>]

[-- Attachment #1: Type: text/plain, Size: 1594 bytes --]

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch; name="patch.diff", Size: 1116 bytes --]

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