* Kees Cook <keesc...@chromium.org> wrote:
> From: Thomas Garnier <thgar...@google.com>
>
> Minor change that allows early boot physical mapping of PUD level virtual
> addresses. The current implementation expects the virtual address to be
> PUD aligned. For KASLR memory randomization, we need to be able to
> randomize the offset used on the PUD table.
>
> It has no impact on current usage.
>
> Signed-off-by: Thomas Garnier <thgar...@google.com>
> Signed-off-by: Kees Cook <keesc...@chromium.org>
> ---
> arch/x86/mm/init_64.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
> index bce2e5d9edd4..f205f39bd808 100644
> --- a/arch/x86/mm/init_64.c
> +++ b/arch/x86/mm/init_64.c
> @@ -454,10 +454,10 @@ phys_pud_init(pud_t *pud_page, unsigned long addr,
> unsigned long end,
> {
> unsigned long pages = 0, next;
> unsigned long last_map_addr = end;
> - int i = pud_index(addr);
> + int i = pud_index((unsigned long)__va(addr));
>
>
> for (; i < PTRS_PER_PUD; i++, addr = next) {
> - pud_t *pud = pud_page + pud_index(addr);
> + pud_t *pud = pud_page + pud_index((unsigned long)__va(addr));
> pmd_t *pmd;
> pgprot_t prot = PAGE_KERNEL;
So I really dislike two things about this code.
Firstly a pre-existing problem is that the parameter names to phys_pud_init()
suck:
static unsigned long __meminit
phys_pud_init(pud_t *pud_page, unsigned long addr, unsigned long end,
unsigned long page_size_mask)
so 'unsigned long addr' is usually the signature of a virtual address - but
that's
no true here: it's a physical address.
Same goes for 'unsigned long end'. Plus it's unclear what the connection
between
'addr' and 'end' - it's not at all obvious 'at a glance' that they are the
start
and end addresses of a physical memory range.
All of these problems can be solved by renaming them to 'paddr_start' and
'paddr_end'.
Btw., I believe this misnomer and confusing code resulted in the buggy
'pud_index(addr)' not being noticed to begin with ...
Secondly, and that's a new problem introduced by this patch:
> + int i = pud_index((unsigned long)__va(addr));
> + pud_t *pud = pud_page + pud_index((unsigned long)__va(addr));
... beyond the repetition, using type casts is fragile. Type casts should be a
red
flag to anyone involved in low level, security relevant code! So I'm pretty
unhappy about seeing such a problem in such a patch.
This code should be doing something like:
unsigned long vaddr_start = __va(paddr_start);
... which gets rid of the type cast, the repetition and documents the code much
better as well. Also see how easily the connection between the variables is
self-documented just by picking names carefully:
paddr_start
paddr_end
vaddr_start
vaddr_end
Also, _please_ add a comment to phys_pud_init() that explains what the function
does.
Thanks,
Ingo
