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

[Chris Li]

Hi Michal,

That change looks correct to me, thanks for the explanation of the
execution behavior.

Acked-by: <chrisl@xxxxxxxxxx>

We can add some test input cases to help better explain the issue. I
quote from your offline message:
-------------- quote -----------
 The old code worked for inputs like this:
"memmap=something,something,something,something,something"
but didn't work for inputs like this:
"memmap=something memmap=something memmap=something memmap=something
memmap=something".
------------ quote -----------

Such kind of example test input in the commit message would help
understand the patch.

On Sat, Jun 21, 2025 at 2:51 AM Michał Cłapiński <mclapinski@xxxxxxxxxx> wrote:
>
> Hi Ard,
>
> On Sat, Jun 21, 2025 at 10:19 AM Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
> >
> > Hi Michal,
> >
> > On Tue, 10 Jun 2025 at 23:42, Michal Clapinski <mclapinski@xxxxxxxxxx> 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.
> > > This change fixes it.
> > >
> > > Signed-off-by: Michal Clapinski <mclapinski@xxxxxxxxxx>
> > > ---
> > > I would like KASLR to support more than 4 memmap args. Do you think
> > > I can just increase the MAX_MEMMAP_REGIONS or should I implement
> > > support for the general case?
> > >
> > >  arch/x86/boot/compressed/kaslr.c | 3 ---
> > >  1 file changed, 3 deletions(-)
> > >
> > > diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> > > index f03d59ea6e40..4aa9c9781ca7 100644
> > > --- a/arch/x86/boot/compressed/kaslr.c
> > > +++ b/arch/x86/boot/compressed/kaslr.c
> > > @@ -162,9 +162,6 @@ static void mem_avoid_memmap(char *str)
> > >  {
> > >         static int i;
> > >
> > > -       if (i >= MAX_MEMMAP_REGIONS)
> > > -               return;
> > > -
> >
> > It isn't obvious at all why simply dropping this condition is fine.
> > Could you elaborate?

Right, this code is very tricky due to the static variable. One of my
nitpicks is that the code structure does not reflect well the
execution flow. That is a pre-existing condition before this patch.

> Of course. Let's look at the whole function without my change:
>
> 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;
Just want to point out that "str" gets reassigned a new value,
possibly NULL here.
I was wondering why it needs to repeat testing str in the later part
of if statements.

>
>                 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)

Just want to note that we test "i" vs MAX_MEMMAP_REGIONS here.

>                 memmap_too_large = true;
> }
>
> This function is called for every memmap= param. Let's say we supply
> only separate memmap= params (instead of comma delimited). Then on the
> 4th param, `i` will be equal to MAX_MEMMAP_REGIONS but the last `if`
> won't execute since `str` will be null. Then on the 5th param the
> first `if` (the one I want to remove) will execute and
> `memmap_too_large` will never be set.

The test input for the above explanation is "memmap=something
memmap=something memmap=something memmap=something memmap=something".

>
> With my change, while parsing the 5th param, the `while` loop will be
> skipped since `i` is not smaller than MAX_MEMMAP_REGIONS and the last
> `if` will execute setting `memmap_too_large`. Basically, my change is
> safe because the `if` I want to remove is already baked into the
> `while` loop condition.
>
>
> >
> > >         while (str && (i < MAX_MEMMAP_REGIONS)) {

And we test "i" vs MAX_MEMMAP_REGIONS again.

I did attempt to reduce the repeat test condition and make the flow
more straight forward.
The bloat-o-meter said it is a tiny little bit better, mostly due to
avoiding repeat testing "i" again MAX_MEMMAP_REGIONS.

$ ./scripts/bloat-o-meter arch/x86/boot/compressed/kaslr.o /tmp/kasl-new.o
add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-8 (-8)
Function                                     old     new   delta
mem_avoid_memmap                             403     395      -8
Total: Before=7992, After=7984, chg -0.10%

The flow is slightly better there, but that is much a bigger patch to review.

Chris

diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 948da3b01cac..f25704f1e1a6 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -189,14 +189,16 @@ static void mem_avoid_memmap(enum parse_mode
mode, 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)
+ goto max_memmap;

+ k = strchr(str, ',');
  if (k)
  *k++ = 0;

@@ -217,9 +219,11 @@ static void mem_avoid_memmap(enum parse_mode
mode, char *str)
  mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].size = size;
  i++;
  }
+ return;

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