linux-kernel - Re: [PATCH] x86/mm: Handle alloc failure in phys_*

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: <aEE9k5XLBWPnHJbu@gmail.com>
Date: Thu, 5 Jun 2025 08:47:47 +0200
From: Ingo Molnar <mingo@...nel.org>
To: Em Sharnoff <sharnoff@...n.tech>
Cc: linux-kernel@...r.kernel.org, x86@...nel.org, linux-mm@...ck.org,
	Dave Hansen <dave.hansen@...ux.intel.com>,
	Andy Lutomirski <luto@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
	"H. Peter Anvin" <hpa@...or.com>,
	"Edgecombe, Rick P" <rick.p.edgecombe@...el.com>,
	Oleg Vasilev <oleg@...n.tech>,
	Arthur Petukhovsky <arthur@...n.tech>,
	Stefan Radig <stefan@...n.tech>, Misha Sakhnov <misha@...n.tech>
Subject: Re: [PATCH] x86/mm: Handle alloc failure in phys_*_init()


* Ingo Molnar <mingo@...nel.org> wrote:

> 
> * Em Sharnoff <sharnoff@...n.tech> wrote:
> 
> > tl;dr:
> > 
> > * When setting up page table mappings for physical addresses after boot,
> >   alloc_low_page() uses GFP_ATOMIC, which is allowed to fail.
> > * This isn't currently handled, and results in a null pointer
> >   dereference when it occurs.
> > * This allocation failure can happen during memory hotplug.
> > 
> > To handle failure, change phys_pud_init() and similar functions to
> > return zero if allocation failed (either directly or transitively), and
> > convert that to -ENOMEM in arch_add_memory().
> 
> > +		/*
> > +		 * Bail only after updating pgd/p4d to keep progress from p4d across retries.
> > +		 */
> > +		if (!paddr_last)
> > +			return 0;
> > +
> >  		pgd_changed = true;
> 
> > -	init_memory_mapping(start, start + size, params->pgprot);
> > +	if (!init_memory_mapping(start, start + size, params->pgprot))
> > +		return -ENOMEM;
> 
> I agree that it makes total sense to fix all this (especially since you 
> are actively triggering it), but have you tried also changing it away 
> from GFP_ATOMIC? There's no real reason why it should be GFP_ATOMIC 
> AFAICS, other than some historic inertia that nobody bothered to fix.
> 
> Plus, could you please change the return flow from this zero 
> special-case over to something like ERR_PTR(-ENOMEM) and IS_ERR()?
> 
> *Technically* zero is a valid physical address, although we 
> intentionally never use it in the kernel AFAIK and wouldn't ever put a 
> page table there either. ERR_PTR()/IS_ERR() is much easier on the eyes 
> than the zero special-case.

Small correction: since this is a PTE pointer, this is a kernel virtual 
address, which can never be zero - only physical addresses can be zero 
(but even they aren't in practice.).

My main point wrt. using ERR_PTR()/IS_ERR() instead of 0 remains.

Thanks,

	Ingo