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

Message-ID: <alpine.DEB.2.20.1701111246400.3579@nanos>
Date:   Wed, 11 Jan 2017 13:00:24 +0100 (CET)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Dave Jiang <dave.jiang@...el.com>
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 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.

> +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?

> +	/* see if we have any memmap areas */

Sentences start with an upper case letter. Everywhere!

> +	if (cmdline_find_option("memmap", arg, sizeof(arg)) > 0) {

You can spare an indentation level by just returning when the search fails.

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

	for (i = 0; str && (i < MAX_MEMMAP_REGIONS); i++) {

Please.

> +			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.

Thanks,

	tglx