linux-kernel - Re: [PATCH] x86/ioremap: Fix off-by-one in e820 check in memremap_should_map

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]

Message-ID: <aAKCaQRYmjNaz175@google.com>
Date: Fri, 18 Apr 2025 18:48:41 +0200
From: Dmytro Maluka <dmaluka@...omium.org>
To: Tom Lendacky <thomas.lendacky@....com>
Cc: 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>,
	"maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@...nel.org>,
	"H. Peter Anvin" <hpa@...or.com>,
	"open list:X86 MM" <linux-kernel@...r.kernel.org>,
	Grzegorz Jaszczyk <jaszczyk@...omium.org>
Subject: Re: [PATCH] x86/ioremap: Fix off-by-one in e820 check in
 memremap_should_map_decrypted()

On Fri, Apr 18, 2025 at 09:43:43AM -0500, Tom Lendacky wrote:
> On 4/18/25 08:59, Dmytro Maluka wrote:
> > The end address in e820__get_entry_type() is exclusive, not inclusive.
> > 
> > Note: untested, bug found by code inspection.
> > 
> > Signed-off-by: Dmytro Maluka <dmaluka@...omium.org>
> > ---
> >  arch/x86/mm/ioremap.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> > index 331e101bf801..a44800a6196e 100644
> > --- a/arch/x86/mm/ioremap.c
> > +++ b/arch/x86/mm/ioremap.c
> > @@ -578,7 +578,7 @@ static bool memremap_should_map_decrypted(resource_size_t phys_addr,
> >  	}
> >  
> >  	/* Check if the address is outside kernel usable area */
> > -	switch (e820__get_entry_type(phys_addr, phys_addr + size - 1)) {
> > +	switch (e820__get_entry_type(phys_addr, phys_addr + size)) {
> 
> I don't think removing the " - 1" is correct. For example, if you are
> mapping a page-aligned value for page-size (4K), then not subtracting 1
> will place you on the next page, which is incorrect, because you are not
> mapping that page.

But __e820__mapped_all(), called by e820__get_entry_type(), handles
'end' non-inclusively:

		...

		/* Is the region (part) in overlap with the current region? */
		if (entry->addr >= end || entry->addr + entry->size <= start)
			continue;

		...

		/*
		 * If 'start' is now at or beyond 'end', we're done, full
		 * coverage of the desired range exists:
		 */
		if (start >= end)
			return entry;

		...

i.e. if we don't subtract 1, phys_addr + size is still outside the
checked region, so all good?

Whereas if we subtract 1, then we have a problem, since
phys_addr + size - 1 is also outside the checked region, i.e. we check
[phys_addr, phys_addr + size - 2] only?

Am I missing something?

...Besides that, after looking closer at e820__get_entry_type(), it
seems buggy on its own (independently of this off-by-one): it is
supposed to return the e820 type of the given range, but if this range
is not covered by a single e820 region but is covered by several
e820 regions, it doesn't check if they all have the same type, it just
returns the type of the last region.

So e.g. if half of the range is E820_TYPE_RAM and only the second half
is E820_TYPE_RESERVED, e820__get_entry_type() will wrongly return
E820_TYPE_RESERVED, so memremap_should_map_decrypted() will cause the
the whole range mapped decrypted?

> 
> Thanks,
> Tom
> 
> >  	case E820_TYPE_RESERVED:
> >  	case E820_TYPE_ACPI:
> >  	case E820_TYPE_NVS: