Re: [PATCH v3 1/6] x86/mm/KASLR: Improve code comments about struct kaslr_memory_region

[Kees Cook]

On Sat, Feb 16, 2019 at 6:01 AM Baoquan He <bhe@xxxxxxxxxx> wrote:
>
> The old comment above kaslr_memory_region is not clear enough to explain
> the concepts of memory region KASLR.
>
> [Ingo suggested this and helped to prettify the text]
>
> Signed-off-by: Baoquan He <bhe@xxxxxxxxxx>
> ---
>  arch/x86/mm/kaslr.c | 55 ++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 52 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/mm/kaslr.c b/arch/x86/mm/kaslr.c
> index 3f452ffed7e9..d7c6e4e8e48e 100644
> --- a/arch/x86/mm/kaslr.c
> +++ b/arch/x86/mm/kaslr.c
> @@ -42,10 +42,59 @@
>  static const unsigned long vaddr_end = CPU_ENTRY_AREA_BASE;
>
>  /*
> - * Memory regions randomized by KASLR (except modules that use a separate logic
> - * earlier during boot). The list is ordered based on virtual addresses. This
> - * order is kept after randomization.
> + * 'struct kasl_memory_region' entries represent continuous chunks of

Typo: struct kaslr_memory_region

Also, while you're rewriting this, how about putting it in full
kern-doc format? (You're already using the "@field" style...) I think
you just need the "/**" header...

/**
 * struct name.... - short description...

> + * kernel virtual memory regions, to be randomized by KASLR.
> + *
> + * ( The exception is the module space virtual memory window which
> + *   uses separate logic earlier during bootup. )
> + *
> + * Currently there are three such regions: the physical memory mapping,
> + * vmalloc and vmemmap regions.
> + *
> + * The array below has the entries ordered based on virtual addresses.
> + * The order is kept after randomization, i.e. the randomized
> + * virtual addresses of these regions are still ascending.
> + *
> + * Here are the fields:
> + *
> + * @base: points to a global variable used by the MM to get the
> + * virtual base address of any of the above regions. This allows the
> + * early KASLR code to modify these base addresses early during bootup,
> + * on a per bootup basis, without the MM code even being aware of whether
> + * it got changed and to what value.
> + *
> + * When KASLR is active then the MM code makes sure that for each region
> + * there's such a single, dynamic, global base address 'unsigned long'
> + * variable available for the KASLR code to point to and modify directly:
> + *
> + *     { &page_offset_base, 0 },
> + *     { &vmalloc_base,     0 },
> + *     { &vmemmap_base,     1 },
> + *
> + * @size_tb: size in TB of each memory region. Thereinto, the size of

nit: "Thereinto" is odd. I'd say "Therefore".

> + * the physical memory mapping region is variable, calculated according
> + * to the actual size of system RAM in order to save more space for
> + * randomization. The rest are fixed values related to paging mode.
> + *
> + * @size_tb: is the size of each memory region after randomization, and
> + * its unit is TB.

Redundant lines?

> + *
> + * Physical memory mapping: (actual RAM size + 10 TB padding)
> + * Vmalloc: 32 TB
> + * Vmemmap: 1 TB
> + *
> + * When randomize the layout, their order are kept, still the physical
> + * memory mapping region is handled fistly, next vmalloc and vmemmap.

typo: "first"

> + * E.g the physical memory region, we limit the starting address to be
> + * taken from the 1st 1/3 part of the whole available virtual address
> + * space which is from 0xffff880000000000 to 0xfffffe0000000000, namely
> + * the original starting address of the physical memory mapping region
> + * to the starting address of cpu_entry_area mapping region. Once a random
> + * address is chosen for the physical memory mapping, we jump over the
> + * region and add 1G to begin the next region handling with the remaining
> + * available space.

Should the "operation" comments (rather than the struct field
comments) be moved to the start of the kernel_randomize_memory()
function instead?

-Kees

>   */
> +
>  static __initdata struct kaslr_memory_region {
>         unsigned long *base;
>         unsigned long size_tb;
> --
> 2.17.2
>


-- 
Kees Cook