Re: [Xen-devel] [PATCH v2 3/3] x86: Make the GDT remapping read-only on 64 bit

[Ingo Molnar <mingo@xxxxxxxxxx>]

* Thomas Garnier <thgarnie@xxxxxxxxxx> wrote:

> This patch makes the GDT remapped pages read-only to prevent corruption.
> This change is done only on 64 bit.

Please spell '64-bit' consistently through the series. I've seen two variants:

  64 bit
  64bit

> +/*
> + * The LTR instruction marks the TSS GDT entry as busy. In 64bit, the GDT is
> + * a read-only remapping. To prevent a page fault, the GDT is switched to the
> + * original writeable version when needed.

s/In 64bit,
 /On 64-bit kernels,

> + */
> +#ifdef CONFIG_X86_64
> +static inline void native_load_tr_desc(void)
> +{
> +     struct desc_ptr gdt;
> +     int cpu = raw_smp_processor_id();
> +     bool restore = false;
> +     struct desc_struct *fixmap_gdt;
> +
> +     native_store_gdt(&gdt);
> +     fixmap_gdt = get_cpu_fixmap_gdt(cpu);
> +
> +     /*
> +      * If the current GDT is the read-only fixmap, swap to the original
> +      * writeable version. Swap back at the end.
> +      */
> +     if (gdt.address == (unsigned long)fixmap_gdt) {
> +             load_direct_gdt(cpu);
> +             restore = true;
> +     }
> +     asm volatile("ltr %w0"::"q" (GDT_ENTRY_TSS*8));
> +     if (restore)
> +             load_fixmap_gdt(cpu);

Please use bool plus 0/1, it's more readable (to me) than the true/false 
notation.

>  extern void switch_to_new_gdt(int);
> +extern void load_direct_gdt(int);
>  extern void load_fixmap_gdt(int);

> +/* Load the original GDT from the per-cpu structure */
> +void load_direct_gdt(int cpu)
> +{
> +     struct desc_ptr gdt_descr;
> +
> +     gdt_descr.address = (long)get_cpu_direct_gdt(cpu);

Please name the functions in an easier to understand way, such as:

        get_cpu_gdt_rw()
        get_cpu_gdt_ro()

that the GDT is in the direct mappings is less important than the fact the the 
address is writable ...

> +}
> +EXPORT_SYMBOL(load_direct_gdt);

EXPORT_SYMBOL_GPL(), or no export at all.

> +EXPORT_SYMBOL(load_fixmap_gdt);

ditto.

>        * VT restores TR but not its size.  Useless.
>        */
> -     struct desc_ptr *gdt = this_cpu_ptr(&host_gdt);
>       struct desc_struct *descs;
>  
> -     descs = (void *)gdt->address;
> +     descs = (void *)get_current_direct_gdt();

Couldn't the type cast be dropped?

>  
> -     table_base = gdt->address;
> +     table_base = (unsigned long)get_current_direct_gdt();

Instead of spreading these type casts far and wide please introduce another 
accessor the returns 'unsigned long':

        get_cpu_gdt_rw_vaddr()

or such.

Thanks,

        Ingo

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel