linux-kernel - Re: [PATCH 1/1] x86/ioremap: Use is_vmalloc

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: <87ikw6rrau.ffs@tglx>
Date: Mon, 12 Aug 2024 12:03:53 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Alistair Popple <apopple@...dia.com>
Cc: Dan Williams <dan.j.williams@...el.com>, dave.hansen@...ux.intel.com,
 luto@...nel.org, peterz@...radead.org, max8rr8@...il.com,
 linux-kernel@...r.kernel.org, x86@...nel.org, jhubbard@...dia.com, Kees
 Cook <keescook@...omium.org>
Subject: Re: [PATCH 1/1] x86/ioremap: Use is_vmalloc_addr in iounmap

On Mon, Aug 12 2024 at 17:41, Alistair Popple wrote:
> Thomas Gleixner <tglx@...utronix.de> writes:
>> ---
>> --- a/arch/x86/include/asm/pgtable_64_types.h
>> +++ b/arch/x86/include/asm/pgtable_64_types.h
>> @@ -187,6 +187,8 @@ extern unsigned int ptrs_per_p4d;
>>  #define KMSAN_MODULES_ORIGIN_START	(KMSAN_MODULES_SHADOW_START + MODULES_LEN)
>>  #endif /* CONFIG_KMSAN */
>>  
>> +#define DIRECT_MAP_END		(VMALLOC_START - 1)
>
> If I'm understanding the KASLR implementation correctly then this
> doesn't seem quite right - the entropy means there is a hole from the
> end of the direct map (ie. the range PAGE_OFFSET to
> PAGE_OFFSET+kaslr_regions[0].size_tb) and VMALLOC_START which shouldn't
> be used.
>
> In practice hotplugging memory into that range probably works, but it
> seems like it would set us up for future bugs. It also means memory
> hotplug could fail intermittently based on the per-boot entropy.
>
> For example with CONFIG_RANDOMIZE_MEMORY_PHYSICAL_PADDING set to 10T one
> could imagine hotplugging 16T would mostly work except on some system
> boots if KASLR happens to randomly place VMALLOC_START close to the end
> of the direct map.
>
> Therefore to keep memory hotplug deterministic I think it would be
> better to define DIRECT_MAP_END as a variable that KASLR sets/updates.

Right. I forgot about the hole.

> This does not look right to me - isn't DIRECT_MAP_END a virtual address
> where as the resource ranges are physical addresses? Ie. I think this
> should be:
>
> +		end = min_t(resource_size_t, base->end, __pa(DIRECT_MAP_END));
>
> The same applies to the rest of the DIRECT_MAP_END users here. Perhaps
> it would be better to define this as DIRECT_MAP_SIZE and calculate this
> based off PAGE_OFFSET instead?

Duh, yes. I shouldn't try to write patches at 30C :)