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

[Michał Cłapiński]

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?

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;

                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;
}

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.

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)) {
> >                 int rc;
> >                 u64 start, size;
> > --
> > 2.50.0.rc0.642.g800a2b2222-goog
> >