Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1424833 > unrolled thread

Re: [PATCH v6 1/3] x86/mm: PUD VA support for physical mapping (x86_64)

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v6 1/3] x86/mm: PUD VA support for physical mapping  (x86_64) Ingo Molnar <mingo@kernel.org> - 2016-06-17 11:10 +0200
    Re: [PATCH v6 1/3] x86/mm: PUD VA support for physical mapping (x86_64) Thomas Garnier <thgarnie@google.com> - 2016-06-20 18:20 +0200
      Re: [PATCH v6 1/3] x86/mm: PUD VA support for physical mapping  (x86_64) Ingo Molnar <mingo@kernel.org> - 2016-06-21 10:40 +0200

#1424833 — Re: [PATCH v6 1/3] x86/mm: PUD VA support for physical mapping (x86_64)

* Kees Cook <keescook@chromium.org> wrote:

> From: Thomas Garnier <thgarnie@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 <thgarnie@google.com>
> Signed-off-by: Kees Cook <keescook@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

[toc] | [next] | [standalone]

#1426771 — Re: [PATCH v6 1/3] x86/mm: PUD VA support for physical mapping (x86_64)

On Fri, Jun 17, 2016 at 2:02 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Kees Cook <keescook@chromium.org> wrote:
>
>> From: Thomas Garnier <thgarnie@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 <thgarnie@google.com>
>> Signed-off-by: Kees Cook <keescook@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 ...
>

I will add a new commit that rename variables as described.

> 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.

Unfortunately, we can't do that because __va return a void*. We will
get this warning on compile:

arch/x86/mm/init_64.c:537:8: warning: assignment makes integer from
pointer without a cast [enabled by default]
  vaddr = __va(paddr_start);

If we used void*, we would need to type cast even more places. What do
you think?

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

Will do on kernel_physical_mapping_init down.

> Also, _please_ add a comment to phys_pud_init() that explains what the function
> does.
>

Will do.

> Thanks,
>
>         Ingo

[toc] | [prev] | [next] | [standalone]

#1427453

* Thomas Garnier <thgarnie@google.com> wrote:

> > 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.
> 
> Unfortunately, we can't do that because __va return a void*. We will
> get this warning on compile:
> 
> arch/x86/mm/init_64.c:537:8: warning: assignment makes integer from
> pointer without a cast [enabled by default]
>   vaddr = __va(paddr_start);
> 
> If we used void*, we would need to type cast even more places. What do
> you think?

Hm, indeed, you are right - so I guess the type cast is OK.

Thanks,

	Ingo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web