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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4DC804A0.4090404@pre-sense.de>
Date:	Mon, 09 May 2011 17:13:36 +0200
From:	Timo Warns <warns@...-sense.de>
To:	Ben Hutchings <ben@...adent.org.uk>
CC:	linux-kernel@...r.kernel.org, stable@...nel.org,
	Eugene Teo <eugeneteo@...nel.sg>,
	Richard Russon <rich@...tcap.org>, akpm@...ux-foundation.org,
	torvalds@...ux-foundation.org, stable-review@...nel.org,
	alan@...rguk.ukuu.org.uk,
	Harvey Harrison <harvey.harrison@...il.com>,
	Greg KH <gregkh@...e.de>
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@...-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

View attachment "patch.diff" of type "text/x-patch" (1076 bytes)