Re: [PATCH v1 2/3] x86: Remap GDT tables in the Fixmap section

[Thomas Garnier]

On Fri, Jan 20, 2017 at 4:57 PM, Andy Lutomirski <luto@xxxxxxxxxxxxxx> wrote:
> On Fri, Jan 20, 2017 at 8:41 AM, Thomas Garnier <thgarnie@xxxxxxxxxx> wrote:
>> Each processor holds a GDT in its per-cpu structure. The sgdt
>> instruction gives the base address of the current GDT. This address can
>> be used to bypass KASLR memory randomization. With another bug, an
>> attacker could target other per-cpu structures or deduce the base of
>> the main memory section (PAGE_OFFSET).
>>
>> This patch relocates the GDT table for each processor inside the
>> Fixmap section. The space is reserved based on number of supported
>> cpus.
>>
>> For consistency, the remapping is done by default on 32 and 64 bit.
>>
>> Each processor switches to its remapped GDT at the end of
>> initialization. For hibernation, the main processor returns with the
>> original GDT and switches back to the remapping at completion.
>>
>> On 32 bit, the maximum number of processors is now 256. The Fixmap
>> section cannot handle the original 512. Additional asserts ensure that
>> the Fixmap section cannot grow beyond the space available.
>>
>> This patch was tested on both architectures. Hibernation and KVM were
>> both tested specially for their usage of the GDT.
>>
>> Signed-off-by: Thomas Garnier <thgarnie@xxxxxxxxxx>
>> ---
>> Based on next-20170119
>> ---
>>  arch/x86/Kconfig                 |  1 +
>>  arch/x86/include/asm/fixmap.h    |  4 ++++
>>  arch/x86/include/asm/processor.h |  1 +
>>  arch/x86/kernel/cpu/common.c     | 18 +++++++++++++++++-
>>  arch/x86/mm/init_32.c            |  2 ++
>>  arch/x86/power/cpu.c             |  3 +++
>>  6 files changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> index f1d4e8f2131f..b4ed35db10a8 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -912,6 +912,7 @@ config MAXSMP
>>  config NR_CPUS
>>         int "Maximum number of CPUs" if SMP && !MAXSMP
>>         range 2 8 if SMP && X86_32 && !X86_BIGSMP
>> +       range 2 256 if SMP && X86_32 && X86_BIGSMP
>>         range 2 512 if SMP && !MAXSMP && !CPUMASK_OFFSTACK
>>         range 2 8192 if SMP && !MAXSMP && CPUMASK_OFFSTACK && X86_64
>>         default "1" if !SMP
>> diff --git a/arch/x86/include/asm/fixmap.h b/arch/x86/include/asm/fixmap.h
>> index c46289799b02..8b913b5e9383 100644
>> --- a/arch/x86/include/asm/fixmap.h
>> +++ b/arch/x86/include/asm/fixmap.h
>> @@ -100,6 +100,10 @@ enum fixed_addresses {
>>  #ifdef CONFIG_X86_INTEL_MID
>>         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,
>> +
>>         __end_of_permanent_fixed_addresses,
>>
>>         /*
>> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
>> index 1be64da0384e..280211ad8be9 100644
>> --- a/arch/x86/include/asm/processor.h
>> +++ b/arch/x86/include/asm/processor.h
>> @@ -705,6 +705,7 @@ extern struct desc_ptr              early_gdt_descr;
>>
>>  extern void cpu_set_gdt(int);
>>  extern void switch_to_new_gdt(int);
>> +extern void load_remapped_gdt(int);
>>  extern void load_percpu_segment(int);
>>  extern void cpu_init(void);
>>
>> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
>> index e97ffc8d29f4..7d940b0e805a 100644
>> --- a/arch/x86/kernel/cpu/common.c
>> +++ b/arch/x86/kernel/cpu/common.c
>> @@ -443,6 +443,19 @@ void load_percpu_segment(int cpu)
>>         load_stack_canary_segment();
>>  }
>>
>> +/* Load a fixmap remapping of the per-cpu GDT */
>> +void load_remapped_gdt(int cpu)
>> +{
>> +       struct desc_ptr gdt_descr;
>> +       unsigned long idx = FIX_GDT_REMAP_BEGIN + cpu;
>> +
>> +       __set_fixmap(idx, __pa(get_cpu_gdt_table(cpu)), PAGE_KERNEL);
>> +
>> +       gdt_descr.address = (long)__fix_to_virt(idx);
>> +       gdt_descr.size = GDT_SIZE - 1;
>> +       load_gdt(&gdt_descr);
>> +}
>
> IMO this should be split into two functions, one to set up the fixmap
> entry and one to load the GDT.
>

That make sense.

> Also, would it be possible to rename the various gdt helpers so that
> their functionality is more obvious?  For example, get_cpu_gdt_table()
> could be get_cpu_direct_gdt_table() and the function to load the gdt
> could be load_fixmap_gdt().
>

Sure no problem.

> --Andy



-- 
Thomas