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 <[email protected]>
>>
>> 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;
_______________________________________________
stable mailing list
[email protected]
http://linux.kernel.org/mailman/listinfo/stable
