[PATCH] x86/ioremap: Fix off-by-one in e820 check in memremap_should_map_decrypted()

[PATCH] x86/ioremap: Fix off-by-one in e820 check in memremap_should_map_decrypted()

[Dmytro Maluka]

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@chromium.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)) {
 	case E820_TYPE_RESERVED:
 	case E820_TYPE_ACPI:
 	case E820_TYPE_NVS:
-- 
2.49.0.805.g082f7c87e0-goog

Re: [PATCH] x86/ioremap: Fix off-by-one in e820 check in memremap_should_map_decrypted()

[Tom Lendacky]

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@chromium.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.

Thanks,
Tom

>  	case E820_TYPE_RESERVED:
>  	case E820_TYPE_ACPI:
>  	case E820_TYPE_NVS:

Re: [PATCH] x86/ioremap: Fix off-by-one in e820 check in memremap_should_map_decrypted()

[Dmytro Maluka]

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@chromium.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: