linux-kernel - Re: [PATCH v2 05/18] x86/fixmap: Generalize the GDT fixmap mechanism

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 22 Nov 2017 09:16:00 -0800
From:   Andy Lutomirski <luto@...nel.org>
To:     Borislav Petkov <bp@...e.de>
Cc:     Andy Lutomirski <luto@...nel.org>, X86 ML <x86@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Brian Gerst <brgerst@...il.com>,
        Dave Hansen <dave.hansen@...el.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>
Subject: Re: [PATCH v2 05/18] x86/fixmap: Generalize the GDT fixmap mechanism

On Wed, Nov 22, 2017 at 7:33 AM, Borislav Petkov <bp@...e.de> wrote:
> On Tue, Nov 21, 2017 at 08:44:00PM -0800, Andy Lutomirski wrote:
>> Currently, the GDT is an ad-hoc array of pages, one per CPU, in the
>> fixmap.  Generalize it to be an array of a new struct cpu_entry_area
>> so that we can cleanly add new things to it.
>>
>> Signed-off-by: Andy Lutomirski <luto@...nel.org>
>> ---
>>  arch/x86/include/asm/desc.h   |  9 +--------
>>  arch/x86/include/asm/fixmap.h | 36 ++++++++++++++++++++++++++++++++++--
>>  arch/x86/kernel/cpu/common.c  | 14 +++++++-------
>>  arch/x86/xen/mmu_pv.c         |  2 +-
>>  4 files changed, 43 insertions(+), 18 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/desc.h b/arch/x86/include/asm/desc.h
>> index 95cd95eb7285..194ffab00ebe 100644
>> --- a/arch/x86/include/asm/desc.h
>> +++ b/arch/x86/include/asm/desc.h
>> @@ -60,17 +60,10 @@ static inline struct desc_struct *get_current_gdt_rw(void)
>>       return this_cpu_ptr(&gdt_page)->gdt;
>>  }
>>
>> -/* Get the fixmap index for a specific processor */
>> -static inline unsigned int get_cpu_gdt_ro_index(int cpu)
>> -{
>> -     return FIX_GDT_REMAP_END - cpu;
>> -}
>> -
>>  /* Provide the fixmap address of the remapped GDT */
>>  static inline struct desc_struct *get_cpu_gdt_ro(int cpu)
>>  {
>> -     unsigned int idx = get_cpu_gdt_ro_index(cpu);
>> -     return (struct desc_struct *)__fix_to_virt(idx);
>> +     return (struct desc_struct *)&get_cpu_entry_area(cpu)->gdt;
>>  }
>>
>>  /* Provide the current read-only GDT */
>> diff --git a/arch/x86/include/asm/fixmap.h b/arch/x86/include/asm/fixmap.h
>> index b0c505fe9a95..038b8474c7f8 100644
>> --- a/arch/x86/include/asm/fixmap.h
>> +++ b/arch/x86/include/asm/fixmap.h
>
> Btw, the whole cpu_entry_area machinery looks like it should belong more
> in asm/processor.h and not in fixmap.h

Agreed, except that the fixmap enum needs to know
sizeof(cpu_entry_area), and I'm really hesitant to add yet another
header dependency.

>
>> @@ -44,6 +44,17 @@ extern unsigned long __FIXADDR_TOP;
>>                        PAGE_SIZE)
>>  #endif
>>
>> +/*
>> + * cpu_entry_area is a percpu region in the fixmap that contains things
>> + * needed by the CPU and early entry/exit code.  Real types aren't used
>> + * for all fields here to about circular header dependencies.
>
> s/about/avoid/
>
>> + */
>> +struct cpu_entry_area
>> +{
>
> ERROR: open brace '{' following struct go on the same line
> #67: FILE: arch/x86/include/asm/fixmap.h:53:
> +struct cpu_entry_area
> +{
>
>> +     char gdt[PAGE_SIZE];
>> +};
>> +
>> +#define CPU_ENTRY_AREA_PAGES (sizeof(struct cpu_entry_area) / PAGE_SIZE)
>>
>>  /*
>>   * Here we define all the compile-time 'special' virtual
>> @@ -101,8 +112,8 @@ enum fixed_addresses {
>>       FIX_LNW_VRTC,
>>  #endif
>>       /* Fixmap entries to remap the GDTs, one per processor. */
>> -     FIX_GDT_REMAP_BEGIN,
>> -     FIX_GDT_REMAP_END = FIX_GDT_REMAP_BEGIN + NR_CPUS - 1,
>> +     FIX_CPU_ENTRY_AREA_TOP,
>> +     FIX_CPU_ENTRY_AREA_BOTTOM = FIX_CPU_ENTRY_AREA_TOP + (CPU_ENTRY_AREA_PAGES * NR_CPUS) - 1,
>>
>>  #ifdef CONFIG_ACPI_APEI_GHES
>>       /* Used for GHES mapping from assorted contexts */
>> @@ -191,5 +202,26 @@ void __init *early_memremap_decrypted_wp(resource_size_t phys_addr,
>>  void __early_set_fixmap(enum fixed_addresses idx,
>>                       phys_addr_t phys, pgprot_t flags);
>>
>> +static inline unsigned int __get_cpu_entry_area_page_index(int cpu, int page)
>> +{
>> +     BUILD_BUG_ON(sizeof(struct cpu_entry_area) % PAGE_SIZE != 0);
>
>         BUILD_BUG_ON(sizeof(struct cpu_entry_area) % PAGE_SIZE);
>

My general habit is that I like the != 0 here because I'm doing
arithmetic rather than thinking of % as some kind of logical operator.
I.e. I find it easier to understand the way I wrote it.

>> +
>> +     return FIX_CPU_ENTRY_AREA_BOTTOM - cpu*CPU_ENTRY_AREA_PAGES - page;
>> +}
>> +
>> +#define __get_cpu_entry_area_offset_index(cpu, offset) ({            \
>> +     BUILD_BUG_ON(offset % PAGE_SIZE != 0);                          \
>
>         BUILD_BUG_ON(offset % PAGE_SIZE);

Ditto.

>
>> +     __get_cpu_entry_area_page_index(cpu, offset / PAGE_SIZE);       \
>> +     })
>> +
>> +#define get_cpu_entry_area_index(cpu, field)                         \
>> +     __get_cpu_entry_area_offset_index((cpu), offsetof(struct cpu_entry_area, field))
>> +
>> +static inline struct cpu_entry_area *get_cpu_entry_area(int cpu)
>> +{
>> +     return (struct cpu_entry_area *)
>> +             __fix_to_virt(__get_cpu_entry_area_page_index(cpu, 0));
>
> Just let it stick out.

Sure.

>
> --
> Regards/Gruss,
>     Boris.
>
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
> --