Re: [PATCH v6 2/3] x86/mm: Implement ASLR for kernel memory sections (x86_64)

[Kees Cook]

Thanks for the review! I'll let Thomas address the feedback, though
I've got some thoughts below on naming.

On Fri, Jun 17, 2016 at 3:26 AM, Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>
> * Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -1993,6 +1993,23 @@ config PHYSICAL_ALIGN
>>
>>         Don't change this unless you know what you are doing.
>>
>> +config RANDOMIZE_MEMORY
>> +     bool "Randomize the kernel memory sections"
>> +     depends on X86_64
>> +     depends on RANDOMIZE_BASE
>> +     default RANDOMIZE_BASE
>> +     ---help---
>> +        Randomizes the base virtual address of kernel memory sections
>> +        (physical memory mapping, vmalloc & vmemmap). This security feature
>> +        makes exploits relying on predictable memory locations less reliable.
>> +
>> +        The order of allocations remains unchanged. Entropy is generated in
>> +        the same way as RANDOMIZE_BASE. Current implementation in the optimal
>> +        configuration have in average 30,000 different possible virtual
>> +        addresses for each memory section.
>> +
>> +        If unsure, say N.
>
> So this is really poor naming!

I think "RANDOMIZE" should probably be changed, yes, though I have
some caveats...

> The user, in first approximation, will see something like this:
>
>   Randomize the kernel memory sections (RANDOMIZE_MEMORY) [Y/n/?] (NEW)
>
> ... and could naively conclude that this feature will randomize memory contents.
>
> Furthermore, there's no apparent connection to another, related kernel feature:
>
>   Randomize the address of the kernel image (KASLR) (RANDOMIZE_BASE) [Y/n/?]
>
> A better naming would be something like:
>
>      Randomize other static kernel memory addresses (RANDOMIZE_ALL) [Y/n/?] (NEW)
>
> (assuming it's truly 'all')

There's always more to be identified and added. A few thoughts:

"Address Space Layout Randomization" is a recognizable name, though I
tried to avoid it in the descriptive texts because
CONFIG_RANDOMIZE_BASE does not randomize "everything" and it doesn't
randomize link order, etc, it only provides a randomized base address
to the kernel text. Now, this is in line with all the userspace ASLR:
each one of stack, mmap, brk, and text have their ASLR done via base
address offsets. ASLR is a technique, and is frequently confused with
"coverage", which I have been trying to fix. i.e. Kernel ASLR of text
base address is one piece of coverage, but it leaves other things not
yet ASLRed.

So, now that we have something else besides base text address ASLR,
I'm happy to create a configs that carry the name "KASLR".

> But ... I don't see it explained anywhere why a user, once he expressed interest
> in randomization would even want to _not_ randomize as much as possible. I.e. why
> does this new option exist at all, shouldn't we just gradually extend
> randomization to more memory areas and control it via CONFIG_RANDOMIZE_BASE?

I think it's important to maintain a higher granularity of CONFIG
options since the maturity of each given feature will vary from arch
to arch. Also, it becomes hard to compare, for example, an x86 .config
with an arm64 .config to see which features are enabled. If
CONFIG_RANDOMIZE_MEMORY landed, it'd be easy to see that it is missing
on arm64 from looking at .config files.

Now, that said, a lot of people have wanted a "make this build as
secure as possible" CONFIG, so it could be nice to have a CONFIG_KASLR
that selects each of the available sub-configs.

> Btw., CONFIG_RANDOMIZE_BASE is probably a misnomer, and after these patches it
> will be more of a misnomer - so we should rename it to something better. For
> example CONFIG_KASLR would have the advantage of being obviously named at a
> glance, to anyone who knows what 'ASLR' means. To those who don't know the short
> description will tell it:
>
>   Kernel address space layout randomization (KASLR) [Y/n/?]
>
> This would also fit the kernel internal naming of kaslr.c/etc.
>
> What do you think?

How about something like this:

CONFIG_KASLR
    depends on HAVE_ARCH_KASLR_TEXT || HAVE_ARCH_KASLR_MEMORY
CONFIG_KASLR_TEXT
    depends on CONFIG_KASLR && HAVE_ARCH_KASLR_TEXT
    default CONFIG_KASLR
CONFIG_KASLR_MEMORY
    depends on CONFIG_KASLR && HAVE_ARCH_KASLR_MEMORY
    default CONFIG_KASLR

And then we can select HAVE_ARCH_KASLR_TEXT for x86, arm64, and MIPS,
and when this lands, x86 would gain HAVE_ARCH_KASLR_MEMORY.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security