linux-kernel - Re: [PATCH] mm,x86: fix span coverage in e820_all

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: <52A7C16A.9040106@huawei.com>
Date:	Wed, 11 Dec 2013 09:35:38 +0800
From:	Xishi Qiu <qiuxishi@...wei.com>
To:	Yinghai Lu <yinghai@...nel.org>
CC:	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	the arch/x86 maintainers <x86@...nel.org>,
	Linn Crosetto <linn@...com>, Pekka Enberg <penberg@...nel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Linux MM <linux-mm@...ck.org>
Subject: Re: [PATCH] mm,x86: fix span coverage in e820_all_mapped()

On 2013/12/11 5:06, Yinghai Lu wrote:

> On Tue, Dec 10, 2013 at 1:06 AM, Xishi Qiu <qiuxishi@...wei.com> wrote:
>> In the following case, e820_all_mapped() will return 1.
>> A < start < B-1 and B < end < C, it means <start, end> spans two regions.
>> <start, end>:           [start - end]
>> e820 addr:          ...[A - B-1][B - C]...
> 
> should be [start, end) right?
> and
> [A, B),[B, C)
> 

Hi Yinghai,

It is right, in this case the function will return 1.

>>
>> Signed-off-by: Xishi Qiu <qiuxishi@...wei.com>
>> ---
>>  arch/x86/kernel/e820.c |   15 +++------------
>>  1 files changed, 3 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
>> index 174da5f..31ecab2 100644
>> --- a/arch/x86/kernel/e820.c
>> +++ b/arch/x86/kernel/e820.c
>> @@ -85,20 +85,11 @@ int __init e820_all_mapped(u64 start, u64 end, unsigned type)
>>
>>                 if (type && ei->type != type)
>>                         continue;
>> -               /* is the region (part) in overlap with the current region ?*/
>> +               /* is the region (part) in overlap with the current region ? */
>>                 if (ei->addr >= end || ei->addr + ei->size <= start)
>>                         continue;
>> -
>> -               /* if the region is at the beginning of <start,end> we move
>> -                * start to the end of the region since it's ok until there
>> -                */
>> -               if (ei->addr <= start)
>> -                       start = ei->addr + ei->size;
> 
> so in your case new start will be B ?
> 
> next run will be C
> 
>> -               /*
>> -                * if start is now at or beyond end, we're done, full
>> -                * coverage
>> -                */
>> -               if (start >= end)
> 
> 
>> +               /* is the region full coverage of <start, end> ? */
>> +               if (ei->addr <= start && ei->addr + ei->size >= end)
>>                         return 1;
>>         }
>>         return 0;
> 
> also e820 should be sanitized already to have [A,C).
> 

Yes, it should be sanitized already, but maybe someone will change the e820
to support some feature, so this function will be a potential bomb.

> or you are talking about [A,B), [B+1, C)
> first run start will be B,  and next run with [B+1, ...), that will be
> skipped...
> will not return 1.
> 
> so old code should be ok.
> 

In this case, old code is right, but I discuss in another one that
you wrote above.

Thanks,
Xishi Qiu

> Thanks
> 
> Yinghai
> 
> .
> 



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/