Re: [PATCH v5] x86: fix kaslr and memmap collision

[Baoquan He <bhe-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

On 01/06/17 at 01:16pm, Kees Cook wrote:
> On Thu, Jan 5, 2017 at 6:44 PM, Baoquan He <bhe-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >> > +static int mem_avoid_memmap(void)
> >> > +{
> >> > +   char arg[128];
> >> > +   int rc = 0;
> >> > +
> >> > +   /* see if we have any memmap areas */
> >> > +   if (cmdline_find_option("memmap", arg, sizeof(arg)) > 0) {
> >> > +           int i = 0;
> >> > +           char *str = arg;
> >> > +
> >> > +           while (str && (i < MAX_MEMMAP_REGIONS)) {
> >> > +                   unsigned long long start, size;
> >> > +                   char *k = strchr(str, ',');
> >> > +
> >> > +                   if (k)
> >> > +                           *k++ = 0;
> >> > +
> >> > +                   rc = parse_memmap(str, &start, &size);
> >> > +                   if (rc < 0)
> >> > +                           break;
> >> > +                   str = k;
> >> > +                   /* a usable region that should not be skipped */
> >> > +                   if (size == 0)
> >> > +                           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)
> >> > +                   rc = -EINVAL;
> >> > +   }
> >> > +
> >> > +   return rc;
> >> > +}
> >> > +
> >> >  /*
> >> >   * In theory, KASLR can put the kernel anywhere in the range of [16M, 64T).
> >> >   * The mem_avoid array is used to store the ranges that need to be avoided
> >> > @@ -438,6 +559,12 @@ void choose_random_location(unsigned long input,
> >> >             return;
> >> >     }
> >> >
> >> > +   /* Mark the memmap regions we need to avoid */
> >> > +   if (mem_avoid_memmap()) {
> >> > +           warn("KASLR disabled: memmap exceeds limit of 4, giving up.");
> >> > +           return;
> >> > +   }
> >>
> >> theoretically, mem_avoid_memmap is doing the mem_avoid initialization
> >> job, should be called inside mem_avoid_init(). The reason you put it
> >> here is you want to make it cancel kaslr, both physical and virtual
> >> address randomization, right?
> >>
> >> In choose_random_location(), the physical and virtual random are done
> >> separately. You can see that later when find_random_phys_addr failed to
> >> find a suitable random slot, it just prints a warning, virtual
> >> randomization is still be done with calling find_random_virt_addr().
> >> Avoiding memmap reserved region should be physical ram issue, should we
> >> stop the kernel virtual address randomization either?
> >>
> >> Kees, what do you think about this?
> 
> Yeah, good catch. mem_avoid_memmap() should be called from
> mem_avoid_init(). I think likely the cleanest approach to dealing with
> the >4 case would be to set a global flag, similar to slot_area_index,
> that is checked in find_random_phys_addr().
> 
> Maybe something like:
> 
> static bool memmap_too_large;

Yes, this is better.

> 
> static int mem_avoid_memmap(void)
> {
> ...
>            /* more than 4 memmaps, fail kaslr */
>            if ((i >= MAX_MEMMAP_REGIONS) && str) {
>                    memmap_too_large = true;
>                    rc = -EINVAL;
>            }
> ...
> }
> ...
> static unsigned long find_random_phys_addr(unsigned long minimum,
>                                            unsigned long image_size)
> {
>         int i;
>         unsigned long addr;
> 
>         /* Check if we had too many memmaps. */
>         if (memmap_too_large) {
>             debug_putstr("Aborted e820 scan (more than 4 memmap=
> arguments)!\n");
>             return 0;
>         }
> 
>         /* Make sure minimum is aligned. */
>         minimum = ALIGN(minimum, CONFIG_PHYSICAL_ALIGN);
> ...
> 
> 
> And we should likely adjust this warning:
> 
>         if (!random_addr) {
>                 warn("KASLR disabled: could not find suitable E820 region!");
> 
> to something like:
> 
>         if (!random_addr) {
>                 warn("Physical KASLR disabled: no suitable memory region!");
> 
> 
> -Kees
> 
> >>
> >> > +
> >> >     boot_params->hdr.loadflags |= KASLR_FLAG;
> >> >
> >> >     /* Prepare to add new identity pagetables on demand. */