linux-kernel - Re: [PATCH v6] x86: fix kaslr and memmap collision

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] [day] [month] [year] [list]

Date:   Wed, 11 Jan 2017 10:59:30 -0700
From:   Dave Jiang <dave.jiang@...el.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     mingo@...hat.com, hpa@...or.com, keescook@...omium.org,
        bhe@...hat.com, linux-nvdimm@...1.01.org, x86@...nel.org,
        david@...morbit.com, linux-kernel@...r.kernel.org,
        dan.j.williams@...el.com
Subject: Re: [PATCH v6] x86: fix kaslr and memmap collision



On 01/11/2017 05:00 AM, Thomas Gleixner wrote:
> On Tue, 10 Jan 2017, Dave Jiang wrote:
>> +unsigned long simple_strtoul(const char *cp, char **endp, unsigned int base);
>> +long simple_strtol(const char *cp, char **endp, unsigned int base);
> 
> What are those functions for? They are not used in that patch at all.

My mistake. Those were used for something in earlier versions of the
patch and I forgot to take them out now they aren't being used. Will remove.

> 
>> +static void mem_avoid_memmap(void)
>> +{
>> +	char arg[128];
>> +	int rc = 0;
> 
> What's the point of defining this variable here and zero initializing it?

It was necessary when the function was returning int instead of void. I
will move.

> 
>> +	/* see if we have any memmap areas */
> 
> Sentences start with an upper case letter. Everywhere!

Will fix.

> 
>> +	if (cmdline_find_option("memmap", arg, sizeof(arg)) > 0) {
> 
> You can spare an indentation level by just returning when the search fails.

Will update.

> 
>> +		int i = 0;
>> +		char *str = arg;
>> +
>> +		while (str && (i < MAX_MEMMAP_REGIONS)) {
> 
> 	for (i = 0; str && (i < MAX_MEMMAP_REGIONS); i++) {
> 
> Please.

The reason it's a while is so that when we hit an instance of
memmap=nn@ss we can conditionally skip it without having to increment
the counter 'i'.

> 
>> +			unsigned long long start, size;
>> +			char *k = strchr(str, ',');
>> +
>> +			if (k)
>> +				*k++ = 0;
>> +
>> +			rc = parse_memmap(str, &start, &size);
>> +			if (rc < 0)
>> +				break;
>> +			str = k;
>> +			/* a usable region that should not be skipped */
>> +			if (size == 0)
>> +				continue;
>> +
>> +			mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].start = start;
>> +			mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].size = size;
> 
> So this makes no sense. You parse start/size as unsigned long long and
> then store it in an unsigned long. Works on 64bit, but on 32bit this is
> just wrong:
> 
> Assume a memmap above 4G, which then will create a avoid region below 4G
> due to truncation to unsigned long.

Ok so it looks like an issue for 32bit on the original code? I'm
presuming none of the previous mem avoid regions are over 4G so that
wasn't an issue? I will update the data structure to use unsigned long
long.

Thanks for the review!

> 
> Thanks,
> 
> 	tglx
>