[PATCH v2 1/1] x86/boot/compressed: Fix avoiding memmap in physical KASLR

[PATCH v2 1/1] x86/boot/compressed: Fix avoiding memmap in physical KASLR

[Michal Clapinski]

The intent of the code was to cancel KASLR if there are more than 4
memmap args. Unfortunately, it was only doing that if the memmap args
were comma delimited, not if they were entirely separate.
So it would disable physical KASLR for:
memmap=1G!4G,1G!5G,1G!6G,1G!7G,1G!8G
since the whole function is just called once and we hit the `if` at
the end of the function.

But it would not disable physical KASLR for:
memmap=1G!4G memmap=1G!5G memmap=1G!6G memmap=1G!7G memmap=1G!8G
since the whole function would be called 5 times and the last `if`
would never trigger.

For the second input, the code would avoid the first 4 memmap regions
but not the last one (it could put the kernel there).

The new code disables physical KASLR for both of those inputs.

Signed-off-by: Michal Clapinski <mclapinski@google.com>
Suggested-by: Chris Li <chrisl@kernel.org>
Fixes: d52e7d5a952c ("x86/KASLR: Parse all 'memmap=' boot option entries")
---
The patch was suggested by Chris and I modified it a little without his
knowledge. I don't know which tags are appropriate.
---
 arch/x86/boot/compressed/kaslr.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 3b0948ad449f..649264503ce6 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -162,14 +162,18 @@ static void mem_avoid_memmap(char *str)
 {
 	static int i;
 
-	if (i >= MAX_MEMMAP_REGIONS)
-		return;
-
-	while (str && (i < MAX_MEMMAP_REGIONS)) {
+	while (str) {
 		int rc;
 		u64 start, size;
-		char *k = strchr(str, ',');
+		char *k;
+
+		if (i >= MAX_MEMMAP_REGIONS) {
+			/* Too many memmap regions, disable physical KASLR. */
+			memmap_too_large = true;
+			return;
+		}
 
+		k = strchr(str, ',');
 		if (k)
 			*k++ = 0;
 
@@ -190,10 +194,6 @@ static void mem_avoid_memmap(char *str)
 		mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].size = size;
 		i++;
 	}
-
-	/* More than 4 memmaps, fail kaslr */
-	if ((i >= MAX_MEMMAP_REGIONS) && str)
-		memmap_too_large = true;
 }
 
 /* Store the number of 1GB huge pages which users specified: */
-- 
2.51.1.814.gb8fa24458f-goog

Re: [PATCH v2 1/1] x86/boot/compressed: Fix avoiding memmap in physical KASLR

[Ard Biesheuvel]

Hi Michal,

Thanks for the patch.

On Thu, 23 Oct 2025 at 01:37, Michal Clapinski <mclapinski@google.com> wrote:
>
> The intent of the code was to cancel KASLR if there are more than 4
> memmap args. Unfortunately, it was only doing that if the memmap args
> were comma delimited, not if they were entirely separate.
> So it would disable physical KASLR for:
> memmap=1G!4G,1G!5G,1G!6G,1G!7G,1G!8G
> since the whole function is just called once and we hit the `if` at
> the end of the function.
>
> But it would not disable physical KASLR for:
> memmap=1G!4G memmap=1G!5G memmap=1G!6G memmap=1G!7G memmap=1G!8G
> since the whole function would be called 5 times and the last `if`
> would never trigger.
>
> For the second input, the code would avoid the first 4 memmap regions
> but not the last one (it could put the kernel there).
>
> The new code disables physical KASLR for both of those inputs.
>

Should we just disable physical KASLR if memmap= appears at all?

> Signed-off-by: Michal Clapinski <mclapinski@google.com>
> Suggested-by: Chris Li <chrisl@kernel.org>
> Fixes: d52e7d5a952c ("x86/KASLR: Parse all 'memmap=' boot option entries")
> ---
> The patch was suggested by Chris and I modified it a little without his
> knowledge. I don't know which tags are appropriate.

I think this is fine, unless Chris has a different opinion? In any
case, you might add a link to the original submission.


> ---
>  arch/x86/boot/compressed/kaslr.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index 3b0948ad449f..649264503ce6 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -162,14 +162,18 @@ static void mem_avoid_memmap(char *str)
>  {
>         static int i;
>
> -       if (i >= MAX_MEMMAP_REGIONS)
> -               return;
> -
> -       while (str && (i < MAX_MEMMAP_REGIONS)) {
> +       while (str) {
>                 int rc;
>                 u64 start, size;
> -               char *k = strchr(str, ',');
> +               char *k;
> +
> +               if (i >= MAX_MEMMAP_REGIONS) {
> +                       /* Too many memmap regions, disable physical KASLR. */
> +                       memmap_too_large = true;
> +                       return;
> +               }
>
> +               k = strchr(str, ',');
>                 if (k)
>                         *k++ = 0;
>
> @@ -190,10 +194,6 @@ static void mem_avoid_memmap(char *str)
>                 mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].size = size;
>                 i++;
>         }
> -
> -       /* More than 4 memmaps, fail kaslr */
> -       if ((i >= MAX_MEMMAP_REGIONS) && str)
> -               memmap_too_large = true;
>  }
>
>  /* Store the number of 1GB huge pages which users specified: */
> --
> 2.51.1.814.gb8fa24458f-goog
>

Re: [PATCH v2 1/1] x86/boot/compressed: Fix avoiding memmap in physical KASLR

[Chris Li]

On Thu, Oct 23, 2025 at 1:25 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> Hi Michal,
>
> Thanks for the patch.
>
> On Thu, 23 Oct 2025 at 01:37, Michal Clapinski <mclapinski@google.com> wrote:
>
> > Signed-off-by: Michal Clapinski <mclapinski@google.com>
> > Suggested-by: Chris Li <chrisl@kernel.org>
> > Fixes: d52e7d5a952c ("x86/KASLR: Parse all 'memmap=' boot option entries")
> > ---
> > The patch was suggested by Chris and I modified it a little without his
> > knowledge. I don't know which tags are appropriate.
>
> I think this is fine, unless Chris has a different opinion? In any

No objection. I recall I had a discussion with Michal in which I
wanted to simplify the global variable "memmap_too_large" state
machine but failed at keeping it meaningfully simpler and also covered
all the corner cases. Nice try on my side I guess.

Right now the memmap_too_large state machine is still a bit tricky to
understand the precise execution flow, but it is the best we can do.

Acked-by: Chris Li <chrisl@kernel.org>

Chris

> case, you might add a link to the original submission.
>
>
> > ---
> >  arch/x86/boot/compressed/kaslr.c | 18 +++++++++---------
> >  1 file changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> > index 3b0948ad449f..649264503ce6 100644
> > --- a/arch/x86/boot/compressed/kaslr.c
> > +++ b/arch/x86/boot/compressed/kaslr.c
> > @@ -162,14 +162,18 @@ static void mem_avoid_memmap(char *str)
> >  {
> >         static int i;
> >
> > -       if (i >= MAX_MEMMAP_REGIONS)
> > -               return;
> > -
> > -       while (str && (i < MAX_MEMMAP_REGIONS)) {
> > +       while (str) {
> >                 int rc;
> >                 u64 start, size;
> > -               char *k = strchr(str, ',');
> > +               char *k;
> > +
> > +               if (i >= MAX_MEMMAP_REGIONS) {
> > +                       /* Too many memmap regions, disable physical KASLR. */
> > +                       memmap_too_large = true;
> > +                       return;
> > +               }
> >
> > +               k = strchr(str, ',');
> >                 if (k)
> >                         *k++ = 0;
> >
> > @@ -190,10 +194,6 @@ static void mem_avoid_memmap(char *str)
> >                 mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].size = size;
> >                 i++;
> >         }
> > -
> > -       /* More than 4 memmaps, fail kaslr */
> > -       if ((i >= MAX_MEMMAP_REGIONS) && str)
> > -               memmap_too_large = true;
> >  }
> >
> >  /* Store the number of 1GB huge pages which users specified: */
> > --
> > 2.51.1.814.gb8fa24458f-goog
> >

Re: [PATCH v2 1/1] x86/boot/compressed: Fix avoiding memmap in physical KASLR

[Dave Hansen]

On 10/22/25 16:37, Michal Clapinski wrote:
...
> But it would not disable physical KASLR for:
> memmap=1G!4G memmap=1G!5G memmap=1G!6G memmap=1G!7G memmap=1G!8G
> since the whole function would be called 5 times and the last `if`
> would never trigger.

I'm missing something about how this works.

The:

        static int i;

is static so should be keeping state across function calls. For the
purposes of checking 'i', why does it matter if the function is called
one time with 5 arguments or 5 times with 1? Doesn't 'i' end up at the
same value either way?

Re: [PATCH v2 1/1] x86/boot/compressed: Fix avoiding memmap in physical KASLR

[Michał Cłapiński]

On Thu, Oct 23, 2025 at 1:29 PM Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 10/22/25 16:37, Michal Clapinski wrote:
> ...
> > But it would not disable physical KASLR for:
> > memmap=1G!4G memmap=1G!5G memmap=1G!6G memmap=1G!7G memmap=1G!8G
> > since the whole function would be called 5 times and the last `if`
> > would never trigger.
>
> I'm missing something about how this works.
>
> The:
>
>         static int i;
>
> is static so should be keeping state across function calls. For the
> purposes of checking 'i', why does it matter if the function is called
> one time with 5 arguments or 5 times with 1? Doesn't 'i' end up at the
> same value either way?

Sorry for not explaining it better (again).

Let's look at the original function:
static void mem_avoid_memmap(char *str)
{
        static int i;

        if (i >= MAX_MEMMAP_REGIONS)
                return;

        while (str && (i < MAX_MEMMAP_REGIONS)) {
                int rc;
                u64 start, size;
                char *k = strchr(str, ',');

                if (k)
                        *k++ = 0;

                rc = parse_memmap(str, &start, &size);
                if (rc < 0)
                        break;
                str = k;

                if (start == 0) {
                        /* Store the specified memory limit if size > 0 */
                        if (size > 0 && size < mem_limit)
                                mem_limit = size;

                        continue;
                }

                mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].start = start;
                mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].size = size;
                i++;
        }

        /* More than 4 memmaps, fail kaslr */
        if ((i >= MAX_MEMMAP_REGIONS) && str)
                memmap_too_large = true;
}

If called once, the `i` gets to 4 and the while loop exits. Then the
last if executes, since `i` is equal to MAX_MEMMAP_REGIONS and `str`
is non-null (if there are more than 4 memmap regions provided). So
memmap_too_large is set and kaslr is disabled.

If called 5 times, on the 4th time `i` will indeed be also equal to 4
but the last `if` never executes since `str` is null. On the 5th time,
we exit the function via the first `if` and memmap_too_large never
gets set.

Re: [PATCH v2 1/1] x86/boot/compressed: Fix avoiding memmap in physical KASLR

[Michał Cłapiński]

On Thu, Oct 23, 2025 at 1:25 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> Hi Michal,
>
> Thanks for the patch.
>
> On Thu, 23 Oct 2025 at 01:37, Michal Clapinski <mclapinski@google.com> wrote:
> >
> > The intent of the code was to cancel KASLR if there are more than 4
> > memmap args. Unfortunately, it was only doing that if the memmap args
> > were comma delimited, not if they were entirely separate.
> > So it would disable physical KASLR for:
> > memmap=1G!4G,1G!5G,1G!6G,1G!7G,1G!8G
> > since the whole function is just called once and we hit the `if` at
> > the end of the function.
> >
> > But it would not disable physical KASLR for:
> > memmap=1G!4G memmap=1G!5G memmap=1G!6G memmap=1G!7G memmap=1G!8G
> > since the whole function would be called 5 times and the last `if`
> > would never trigger.
> >
> > For the second input, the code would avoid the first 4 memmap regions
> > but not the last one (it could put the kernel there).
> >
> > The new code disables physical KASLR for both of those inputs.
> >
>
> Should we just disable physical KASLR if memmap= appears at all?

It would indeed fix my issue. I don't know how much security physical
KASLR provides but I'm not sure disabling security features instead of
fixing them is a good pattern.

I would actually prefer to significantly increase the
MAX_MEMMAP_REGIONS to something like 2 * MAX_NUMNODES but I'm not sure
if creating such a big static array is okay.

> > Signed-off-by: Michal Clapinski <mclapinski@google.com>
> > Suggested-by: Chris Li <chrisl@kernel.org>
> > Fixes: d52e7d5a952c ("x86/KASLR: Parse all 'memmap=' boot option entries")
> > ---
> > The patch was suggested by Chris and I modified it a little without his
> > knowledge. I don't know which tags are appropriate.
>
> I think this is fine, unless Chris has a different opinion? In any
> case, you might add a link to the original submission.

Will do in v2 if otherwise people are okay with this patch.

[snip]

Re: [PATCH v2 1/1] x86/boot/compressed: Fix avoiding memmap in physical KASLR

[Dave Hansen]

On 10/23/25 16:23, Michał Cłapiński wrote:
...
> If called 5 times, on the 4th time `i` will indeed be also equal to 4
> but the last `if` never executes since `str` is null. On the 5th time,
> we exit the function via the first `if` and memmap_too_large never
> gets set.

Ahh, thanks for the explanation. The implication of the first `if` is
what I was missing.

How about we just move the `if` you suggested. Let's put it right next
to the i++ so that they're *obviously* connected and the moment `i` gets
too big, the very next thing that happens is setting `memmap_too_large`
and jumping out of the function.

Re: [PATCH v2 1/1] x86/boot/compressed: Fix avoiding memmap in physical KASLR

[Michał Cłapiński]

On Fri, Oct 24, 2025 at 11:21 AM Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 10/23/25 16:23, Michał Cłapiński wrote:
> ...
> > If called 5 times, on the 4th time `i` will indeed be also equal to 4
> > but the last `if` never executes since `str` is null. On the 5th time,
> > we exit the function via the first `if` and memmap_too_large never
> > gets set.
>
> Ahh, thanks for the explanation. The implication of the first `if` is
> what I was missing.
>
> How about we just move the `if` you suggested. Let's put it right next
> to the i++ so that they're *obviously* connected and the moment `i` gets
> too big, the very next thing that happens is setting `memmap_too_large`
> and jumping out of the function.

I might be misunderstanding but I don't see how that would work. When
`i` gets to 4, it's perfectly fine. That means we parsed 4 memmap
regions. I can't fail then. But if `i` gets to 5, it's already too
late. It would mean I've committed an out-of-bounds write.

We should fail only if `i` gets to 4 and there is more data. In the
case of calling the function 5 separate times, we don't know whether
there will be more data.

The check would have to be before `i++`. But then we're unnecessarily
parsing the argument. So we should move the check before the parsing.
And then we arrive at the code I proposed.

Re: [PATCH v2 1/1] x86/boot/compressed: Fix avoiding memmap in physical KASLR

[Michał Cłapiński]

On Thu, Oct 23, 2025 at 1:37 AM Michal Clapinski <mclapinski@google.com> wrote:
...

Ping. It's been 2 weeks. I'm sure there are ways to improve this code
even more but I truly believe it's better than it was since it's just
a bugfix.

Are there any changes I should make to this?

Re: [PATCH v2 1/1] x86/boot/compressed: Fix avoiding memmap in physical KASLR

[Dave Hansen]

On 11/6/25 06:59, Michał Cłapiński wrote:
> On Thu, Oct 23, 2025 at 1:37 AM Michal Clapinski <mclapinski@google.com> wrote:
> ...
> 
> Ping. It's been 2 weeks. I'm sure there are ways to improve this code
> even more but I truly believe it's better than it was since it's just
> a bugfix.
> 
> Are there any changes I should make to this?

You've gotten feedback and a couple of questions. You also indicated you
were going to post a v2. The next step would be to post a v2, I think.