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