Re: [PATCH 4/4] x86: switch default mapping attributes to non-executable

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 18/05/15 13:47, Jan Beulich wrote:
> Only a very limited subset of mappings need to be done as executable
> ones; in particular the direct mapping should not be executable to
> limit the damage attackers can cause by exploiting security relevant
> bugs.
>
> The EFI change at once includes an adjustment to set NX only when
> supported by the hardware.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -293,7 +293,7 @@ struct vcpu_guest_context *alloc_vcpu_gu
>              free_vcpu_guest_context(NULL);
>              return NULL;
>          }
> -        __set_fixmap(idx - i, page_to_mfn(pg), __PAGE_HYPERVISOR);
> +        __set_fixmap(idx - i, page_to_mfn(pg), __PAGE_HYPERVISOR_RW);
>          per_cpu(vgc_pages[i], cpu) = pg;
>      }
>      return (void *)fix_to_virt(idx);
> --- a/xen/arch/x86/domain_page.c
> +++ b/xen/arch/x86/domain_page.c
> @@ -160,7 +160,7 @@ void *map_domain_page(unsigned long mfn)
>  
>      spin_unlock(&dcache->lock);
>  
> -    l1e_write(&MAPCACHE_L1ENT(idx), l1e_from_pfn(mfn, __PAGE_HYPERVISOR));
> +    l1e_write(&MAPCACHE_L1ENT(idx), l1e_from_pfn(mfn, __PAGE_HYPERVISOR_RW));
>  
>   out:
>      local_irq_restore(flags);
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4416,7 +4416,7 @@ long set_gdt(struct vcpu *v, 
>      for ( i = 0; i < nr_pages; i++ )
>      {
>          v->arch.pv_vcpu.gdt_frames[i] = frames[i];
> -        l1e_write(&pl1e[i], l1e_from_pfn(frames[i], __PAGE_HYPERVISOR));
> +        l1e_write(&pl1e[i], l1e_from_pfn(frames[i], __PAGE_HYPERVISOR_RW));
>      }
>  
>      xfree(pfns);
> @@ -6004,7 +6004,7 @@ int create_perdomain_mapping(struct doma
>                  if ( !IS_NIL(ppg) )
>                      *ppg++ = pg;
>                  l1tab[l1_table_offset(va)] =
> -                    l1e_from_page(pg, __PAGE_HYPERVISOR | _PAGE_AVAIL0);
> +                    l1e_from_page(pg, __PAGE_HYPERVISOR_RW | _PAGE_AVAIL0);
>                  l2e_add_flags(*pl2e, _PAGE_AVAIL0);
>              }
>              else
> @@ -6133,7 +6133,7 @@ void memguard_init(void)
>          (unsigned long)__va(start),
>          start >> PAGE_SHIFT,
>          (__pa(&_end) + PAGE_SIZE - 1 - start) >> PAGE_SHIFT,
> -        __PAGE_HYPERVISOR|MAP_SMALL_PAGES);
> +        __PAGE_HYPERVISOR_RW|MAP_SMALL_PAGES);
>      BUG_ON(start != xen_phys_start);
>      map_pages_to_xen(
>          XEN_VIRT_START,
> @@ -6146,7 +6146,7 @@ static void __memguard_change_range(void
>  {
>      unsigned long _p = (unsigned long)p;
>      unsigned long _l = (unsigned long)l;
> -    unsigned int flags = __PAGE_HYPERVISOR | MAP_SMALL_PAGES;
> +    unsigned int flags = __PAGE_HYPERVISOR_RW | MAP_SMALL_PAGES;
>  
>      /* Ensure we are dealing with a page-aligned whole number of pages. */
>      ASSERT((_p&~PAGE_MASK) == 0);
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -900,7 +900,7 @@ void __init noreturn __start_xen(unsigne
>              /* The only data mappings to be relocated are in the Xen area. */
>              pl2e = __va(__pa(l2_xenmap));
>              *pl2e++ = l2e_from_pfn(xen_phys_start >> PAGE_SHIFT,
> -                                   PAGE_HYPERVISOR | _PAGE_PSE);
> +                                   PAGE_HYPERVISOR_RWX | _PAGE_PSE);
>              for ( i = 1; i < L2_PAGETABLE_ENTRIES; i++, pl2e++ )
>              {
>                  if ( !(l2e_get_flags(*pl2e) & _PAGE_PRESENT) )
> @@ -1087,7 +1087,7 @@ void __init noreturn __start_xen(unsigne
>              /* This range must not be passed to the boot allocator and
>               * must also not be mapped with _PAGE_GLOBAL. */
>              map_pages_to_xen((unsigned long)__va(map_e), PFN_DOWN(map_e),
> -                             PFN_DOWN(e - map_e), __PAGE_HYPERVISOR);
> +                             PFN_DOWN(e - map_e), __PAGE_HYPERVISOR_RW);
>          }
>          if ( s < map_s )
>          {
> --- a/xen/arch/x86/x86_64/mm.c
> +++ b/xen/arch/x86/x86_64/mm.c
> @@ -895,6 +895,33 @@ void __init subarch_init_memory(void)
>              share_xen_page_with_privileged_guests(page, XENSHARE_readonly);
>          }
>      }
> +
> +    /* Mark low 16Mb of direct map NX if hardware supports it. */
> +    if ( !cpu_has_nx )
> +        return;
> +
> +    v = DIRECTMAP_VIRT_START + (1UL << 20);

What about 0-1MB ?

> +    l3e = l4e_to_l3e(idle_pg_table[l4_table_offset(v)])[l3_table_offset(v)];
> +    ASSERT(l3e_get_flags(l3e) & _PAGE_PRESENT);
> +    do {
> +        l2e = l3e_to_l2e(l3e)[l2_table_offset(v)];
> +        ASSERT(l2e_get_flags(l2e) & _PAGE_PRESENT);
> +        if ( l2e_get_flags(l2e) & _PAGE_PSE )
> +        {
> +            l2e_add_flags(l2e, _PAGE_NX_BIT);
> +            l3e_to_l2e(l3e)[l2_table_offset(v)] = l2e;
> +            v += 1 << L2_PAGETABLE_SHIFT;
> +        }
> +        else
> +        {
> +            l1_pgentry_t l1e = l2e_to_l1e(l2e)[l1_table_offset(v)];
> +
> +            ASSERT(l1e_get_flags(l1e) & _PAGE_PRESENT);
> +            l1e_add_flags(l1e, _PAGE_NX_BIT);
> +            l2e_to_l1e(l2e)[l1_table_offset(v)] = l1e;
> +            v += 1 << L1_PAGETABLE_SHIFT;
> +        }
> +    } while ( v < DIRECTMAP_VIRT_START + (16UL << 20) );

How about just setting l3e.nx and leaving all lower tables alone?

>  }
>  
>  long subarch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> @@ -1359,7 +1386,7 @@ int memory_add(unsigned long spfn, unsig
>          if ( i < spfn )
>              i = spfn;
>          ret = map_pages_to_xen((unsigned long)mfn_to_virt(i), i,
> -                               epfn - i, __PAGE_HYPERVISOR);
> +                               epfn - i, __PAGE_HYPERVISOR_RW);
>          if ( ret )
>              return ret;
>      }
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -1162,7 +1162,7 @@ void __init efi_init_memory(void)
>          EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
>          u64 len = desc->NumberOfPages << EFI_PAGE_SHIFT;
>          unsigned long smfn, emfn;
> -        unsigned int prot = PAGE_HYPERVISOR;
> +        unsigned int prot = PAGE_HYPERVISOR_RWX;
>  
>          printk(XENLOG_INFO " %013" PRIx64 "-%013" PRIx64
>                             " type=%u attr=%016" PRIx64 "\n",
> @@ -1195,7 +1195,7 @@ void __init efi_init_memory(void)
>          if ( desc->Attribute & EFI_MEMORY_WP )
>              prot &= _PAGE_RW;
>          if ( desc->Attribute & EFI_MEMORY_XP )
> -            prot |= _PAGE_NX_BIT;
> +            prot |= _PAGE_NX;
>  
>          if ( pfn_to_pdx(emfn - 1) < (DIRECTMAP_SIZE >> PAGE_SHIFT) &&
>               !(smfn & pfn_hole_mask) &&
> --- a/xen/include/asm-x86/page.h
> +++ b/xen/include/asm-x86/page.h
> @@ -303,7 +303,8 @@ void paging_init(void);
>  #define _PAGE_AVAIL1   _AC(0x400,U)
>  #define _PAGE_AVAIL2   _AC(0x800,U)
>  #define _PAGE_AVAIL    _AC(0xE00,U)
> -#define _PAGE_PSE_PAT _AC(0x1000,U)
> +#define _PAGE_PSE_PAT  _AC(0x1000,U)
> +#define _PAGE_NX       (cpu_has_nx ? _PAGE_NX_BIT : 0)
>  /* non-architectural flags */
>  #define _PAGE_PAGED   0x2000U
>  #define _PAGE_SHARED  0x4000U
> @@ -320,6 +321,9 @@ void paging_init(void);
>  #define _PAGE_GNTTAB   0
>  #endif
>  
> +#define __PAGE_HYPERVISOR_RO      (_PAGE_PRESENT | _PAGE_ACCESSED | _PAGE_NX)
> +#define __PAGE_HYPERVISOR_RW      (__PAGE_HYPERVISOR_RO | \
> +                                   _PAGE_DIRTY | _PAGE_RW)
>  #define __PAGE_HYPERVISOR_RX      (_PAGE_PRESENT | _PAGE_ACCESSED)
>  #define __PAGE_HYPERVISOR         (__PAGE_HYPERVISOR_RX | \
>                                     _PAGE_DIRTY | _PAGE_RW)
> --- a/xen/include/asm-x86/x86_64/page.h
> +++ b/xen/include/asm-x86/x86_64/page.h
> @@ -147,9 +147,20 @@ typedef l4_pgentry_t root_pgentry_t;
>   */
>  #define _PAGE_GUEST_KERNEL (1U<<12)
>  
> -#define PAGE_HYPERVISOR         (__PAGE_HYPERVISOR         | _PAGE_GLOBAL)
> +#define PAGE_HYPERVISOR_RO      (__PAGE_HYPERVISOR_RO      | _PAGE_GLOBAL)
> +#define PAGE_HYPERVISOR_RW      (__PAGE_HYPERVISOR_RW      | _PAGE_GLOBAL)
>  #define PAGE_HYPERVISOR_RX      (__PAGE_HYPERVISOR_RX      | _PAGE_GLOBAL)
> -#define PAGE_HYPERVISOR_NOCACHE (__PAGE_HYPERVISOR_NOCACHE | _PAGE_GLOBAL)
> +#define PAGE_HYPERVISOR_RWX     (__PAGE_HYPERVISOR         | _PAGE_GLOBAL)
> +
> +#ifdef __ASSEMBLY__
> +/* Dependency on NX being available can't be expressed. */
> +# define PAGE_HYPERVISOR         PAGE_HYPERVISOR_RWX
> +# define PAGE_HYPERVISOR_NOCACHE (__PAGE_HYPERVISOR_NOCACHE | _PAGE_GLOBAL)
> +#else
> +# define PAGE_HYPERVISOR         PAGE_HYPERVISOR_RW
> +# define PAGE_HYPERVISOR_NOCACHE (__PAGE_HYPERVISOR_NOCACHE | \
> +                                  _PAGE_GLOBAL | _PAGE_NX)
> +#endif

This patch is clearly an improvement, so my comments can possibly be
deferred to subsequent patches.

After boot, I can't think of a legitimate reason for Xen to have a RWX
mapping.  As such, leaving __PAGE_HYPERVISOR around constitutes a risk
that RWX mappings will be used.  It would be nice if we could remove all
constants (which aren't BOOT_*) which could lead to accidental use of
RWX mappings on NX-enabled hardware.

I think we also want a warning on boot if we find NX unavailable.  The
only 64bit capable CPUs which do not support NX are now 11 years old,
and I don't expect many of those to be running Xen these days.  However,
given that "disable NX" is a common BIOS option for compatibility
reasons, we should press people to alter their settings if possible.

~Andrew

>  
>  #endif /* __X86_64_PAGE_H__ */
>  
>
>