arm64 kernel W^X

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

arm64 kernel W^X

Mark Kettenis
The diff below introduces kernel W^X on arm64.  It changes the linker
script to align the RX, R and RW segments on a 2MB boundary such that
we can continue to use block mappings for the kernel.  Then it adds
some additional linker generated symbols that allow us to determine
whether a block is .text, .rodata or .data/.bss.

ok?


Index: arch/arm64/arm64/pmap.c
===================================================================
RCS file: /cvs/src/sys/arch/arm64/arm64/pmap.c,v
retrieving revision 1.26
diff -u -p -r1.26 pmap.c
--- arch/arm64/arm64/pmap.c 16 Mar 2017 20:15:07 -0000 1.26
+++ arch/arm64/arm64/pmap.c 17 Mar 2017 17:39:05 -0000
@@ -255,6 +255,27 @@ const struct kmem_pa_mode kp_lN = {
  .kp_zero = 1,
 };
 
+const uint64_t ap_bits_user[8] = {
+ [PROT_NONE] = ATTR_nG|ATTR_PXN|ATTR_UXN|ATTR_AP(2),
+ [PROT_READ] = ATTR_nG|ATTR_PXN|ATTR_UXN|ATTR_AF|ATTR_AP(3),
+ [PROT_WRITE] = ATTR_nG|ATTR_PXN|ATTR_UXN|ATTR_AF|ATTR_AP(1),
+ [PROT_WRITE|PROT_READ] = ATTR_nG|ATTR_PXN|ATTR_UXN|ATTR_AF|ATTR_AP(1),
+ [PROT_EXEC] = ATTR_nG|ATTR_PXN|ATTR_AF|ATTR_AP(2),
+ [PROT_EXEC|PROT_READ] = ATTR_nG|ATTR_PXN|ATTR_AF|ATTR_AP(3),
+ [PROT_EXEC|PROT_WRITE] = ATTR_nG|ATTR_PXN|ATTR_AF|ATTR_AP(1),
+ [PROT_EXEC|PROT_WRITE|PROT_READ]= ATTR_nG|ATTR_PXN|ATTR_AF|ATTR_AP(1),
+};
+
+const uint64_t ap_bits_kern[8] = {
+ [PROT_NONE] = ATTR_PXN|ATTR_UXN|ATTR_AP(2),
+ [PROT_READ] = ATTR_PXN|ATTR_UXN|ATTR_AF|ATTR_AP(2),
+ [PROT_WRITE] = ATTR_PXN|ATTR_UXN|ATTR_AF|ATTR_AP(0),
+ [PROT_WRITE|PROT_READ] = ATTR_PXN|ATTR_UXN|ATTR_AF|ATTR_AP(0),
+ [PROT_EXEC] = ATTR_UXN|ATTR_AF|ATTR_AP(2),
+ [PROT_EXEC|PROT_READ] = ATTR_UXN|ATTR_AF|ATTR_AP(2),
+ [PROT_EXEC|PROT_WRITE] = ATTR_UXN|ATTR_AF|ATTR_AP(0),
+ [PROT_EXEC|PROT_WRITE|PROT_READ] = ATTR_UXN|ATTR_AF|ATTR_AP(0),
+};
 
 /*
  * This is used for pmap_kernel() mappings, they are not to be removed
@@ -1222,7 +1243,9 @@ pmap_bootstrap(long kvo, paddr_t lpt1,  
  // enable mappings for existing 'allocated' mapping in the bootstrap
  // page tables
  extern uint64_t *pagetable;
- extern int *_end;
+ extern char __text_start[], _etext[];
+ extern char __rodata_start[], _erodata[];
+ extern char _end[];
  vp2 = (void *)((long)&pagetable + kvo);
  struct mem_region *mp;
  ssize_t size;
@@ -1234,11 +1257,21 @@ pmap_bootstrap(long kvo, paddr_t lpt1,  
  {
  paddr_t mappa = pa & ~(L2_SIZE-1);
  vaddr_t mapva = mappa - kvo;
- if (mapva < (vaddr_t)&_end)
+ int prot = PROT_READ | PROT_WRITE;
+
+ if (mapva < (vaddr_t)_end)
  continue;
+
+ if (mapva >= (vaddr_t)__text_start &&
+    mapva < (vaddr_t)_etext)
+ prot = PROT_READ | PROT_EXEC;
+ else if (mapva >= (vaddr_t)__rodata_start &&
+ mapva < (vaddr_t)_erodata)
+ prot = PROT_READ;
+
  vp2->l2[VP_IDX2(mapva)] = mappa | L2_BLOCK |
-    ATTR_AF | ATTR_IDX(PTE_ATTR_WB) |
-    ATTR_SH(SH_INNER);
+    ATTR_IDX(PTE_ATTR_WB) | ATTR_SH(SH_INNER) |
+    ap_bits_kern[prot];
  }
  }
 
@@ -1572,28 +1605,6 @@ pmap_proc_iflush(struct process *pr, vad
  if (pr == curproc->p_p)
  cpu_icache_sync_range(va, len);
 }
-
-STATIC uint64_t ap_bits_user[8] = {
- [PROT_NONE] = ATTR_nG|ATTR_PXN|ATTR_UXN|ATTR_AP(2),
- [PROT_READ] = ATTR_nG|ATTR_PXN|ATTR_UXN|ATTR_AF|ATTR_AP(3),
- [PROT_WRITE] = ATTR_nG|ATTR_PXN|ATTR_UXN|ATTR_AF|ATTR_AP(1),
- [PROT_WRITE|PROT_READ] = ATTR_nG|ATTR_PXN|ATTR_UXN|ATTR_AF|ATTR_AP(1),
- [PROT_EXEC] = ATTR_nG|ATTR_PXN|ATTR_AF|ATTR_AP(2),
- [PROT_EXEC|PROT_READ] = ATTR_nG|ATTR_PXN|ATTR_AF|ATTR_AP(3),
- [PROT_EXEC|PROT_WRITE] = ATTR_nG|ATTR_PXN|ATTR_AF|ATTR_AP(1),
- [PROT_EXEC|PROT_WRITE|PROT_READ]= ATTR_nG|ATTR_PXN|ATTR_AF|ATTR_AP(1),
-};
-
-STATIC uint64_t ap_bits_kern[8] = {
- [PROT_NONE] = ATTR_PXN|ATTR_UXN|ATTR_AP(2),
- [PROT_READ] = ATTR_PXN|ATTR_UXN|ATTR_AF|ATTR_AP(2),
- [PROT_WRITE] = ATTR_PXN|ATTR_UXN|ATTR_AF|ATTR_AP(0),
- [PROT_WRITE|PROT_READ] = ATTR_PXN|ATTR_UXN|ATTR_AF|ATTR_AP(0),
- [PROT_EXEC] = ATTR_UXN|ATTR_AF|ATTR_AP(2),
- [PROT_EXEC|PROT_READ] = ATTR_UXN|ATTR_AF|ATTR_AP(2),
- [PROT_EXEC|PROT_WRITE] = ATTR_UXN|ATTR_AF|ATTR_AP(0),
- [PROT_EXEC|PROT_WRITE|PROT_READ] = ATTR_UXN|ATTR_AF|ATTR_AP(0),
-};
 
 void
 pmap_pte_insert(struct pte_desc *pted)
Index: arch/arm64/conf/kern.ldscript
===================================================================
RCS file: /cvs/src/sys/arch/arm64/conf/kern.ldscript,v
retrieving revision 1.4
diff -u -p -r1.4 kern.ldscript
--- arch/arm64/conf/kern.ldscript 23 Jan 2017 14:11:21 -0000 1.4
+++ arch/arm64/conf/kern.ldscript 17 Mar 2017 17:39:05 -0000
@@ -12,34 +12,35 @@ PHDRS
  openbsd_randomize PT_OPENBSD_RANDOMIZE;
 }
 
-__ALIGN_SIZE = 0x1000;
+__ALIGN_SIZE = 0x200000;
 __kernel_base = @KERNEL_BASE_VIRT@;
 
 ENTRY(_start)
 SECTIONS
 {
  . = __kernel_base;
+ PROVIDE (__text_start = .);
  .text :
  {
  *(.text .text.*)
  *(.stub)
  *(.glue_7t) *(.glue_7)
  } :text =0
- PROVIDE (__etext = .);
  PROVIDE (_etext = .);
  PROVIDE (etext = .);
 
  /* Move rodata to the next page, so we can nuke X and W bit on it */
  . = ALIGN(__ALIGN_SIZE);
+ PROVIDE (__rodata_start = .);
  .rodata :
  {
  *(.rodata .rodata.*)
  } :rodata
- PROVIDE (erodata = .);
- _erodata = .;
+ PROVIDE (_erodata = .);
 
  /* Move .random to the next page, so we can add W bit on it and .data */
  . = ALIGN(__ALIGN_SIZE);
+ PROVIDE (__data_start = .);
  .openbsd.randomdata :
  {
  *(.openbsd.randomdata)
@@ -52,8 +53,7 @@ SECTIONS
  {
  *(.sdata .sdata.*)
  } :data
- PROVIDE (edata = .);
- _edata = .;
+ PROVIDE (_edata = .);
 
  PROVIDE (__bss_start = .);
  .sbss :
@@ -74,6 +74,6 @@ SECTIONS
    .bss section disappears because there are no input sections.  */
  . = ALIGN(64 / 8);
  } :data
+ PROVIDE (_end = .);
  PROVIDE (end = .);
- _end = .;
 }

Reply | Threaded
Open this post in threaded view
|

Re: arm64 kernel W^X

Patrick Wildt-3
On Fri, Mar 17, 2017 at 06:42:38PM +0100, Mark Kettenis wrote:

> The diff below introduces kernel W^X on arm64.  It changes the linker
> script to align the RX, R and RW segments on a 2MB boundary such that
> we can continue to use block mappings for the kernel.  Then it adds
> some additional linker generated symbols that allow us to determine
> whether a block is .text, .rodata or .data/.bss.
>
> ok?
>
>
> Index: arch/arm64/arm64/pmap.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/arm64/arm64/pmap.c,v
> retrieving revision 1.26
> diff -u -p -r1.26 pmap.c
> --- arch/arm64/arm64/pmap.c 16 Mar 2017 20:15:07 -0000 1.26
> +++ arch/arm64/arm64/pmap.c 17 Mar 2017 17:39:05 -0000
> @@ -255,6 +255,27 @@ const struct kmem_pa_mode kp_lN = {
>   .kp_zero = 1,
>  };
>  
> +const uint64_t ap_bits_user[8] = {
> + [PROT_NONE] = ATTR_nG|ATTR_PXN|ATTR_UXN|ATTR_AP(2),
> + [PROT_READ] = ATTR_nG|ATTR_PXN|ATTR_UXN|ATTR_AF|ATTR_AP(3),
> + [PROT_WRITE] = ATTR_nG|ATTR_PXN|ATTR_UXN|ATTR_AF|ATTR_AP(1),
> + [PROT_WRITE|PROT_READ] = ATTR_nG|ATTR_PXN|ATTR_UXN|ATTR_AF|ATTR_AP(1),
> + [PROT_EXEC] = ATTR_nG|ATTR_PXN|ATTR_AF|ATTR_AP(2),
> + [PROT_EXEC|PROT_READ] = ATTR_nG|ATTR_PXN|ATTR_AF|ATTR_AP(3),
> + [PROT_EXEC|PROT_WRITE] = ATTR_nG|ATTR_PXN|ATTR_AF|ATTR_AP(1),
> + [PROT_EXEC|PROT_WRITE|PROT_READ]= ATTR_nG|ATTR_PXN|ATTR_AF|ATTR_AP(1),
> +};
> +
> +const uint64_t ap_bits_kern[8] = {
> + [PROT_NONE] = ATTR_PXN|ATTR_UXN|ATTR_AP(2),
> + [PROT_READ] = ATTR_PXN|ATTR_UXN|ATTR_AF|ATTR_AP(2),
> + [PROT_WRITE] = ATTR_PXN|ATTR_UXN|ATTR_AF|ATTR_AP(0),
> + [PROT_WRITE|PROT_READ] = ATTR_PXN|ATTR_UXN|ATTR_AF|ATTR_AP(0),
> + [PROT_EXEC] = ATTR_UXN|ATTR_AF|ATTR_AP(2),
> + [PROT_EXEC|PROT_READ] = ATTR_UXN|ATTR_AF|ATTR_AP(2),
> + [PROT_EXEC|PROT_WRITE] = ATTR_UXN|ATTR_AF|ATTR_AP(0),
> + [PROT_EXEC|PROT_WRITE|PROT_READ] = ATTR_UXN|ATTR_AF|ATTR_AP(0),
> +};

Is this only a move of the lists or is there also a change involved?
It's a bit hard to compare.

>  
>  /*
>   * This is used for pmap_kernel() mappings, they are not to be removed
> @@ -1222,7 +1243,9 @@ pmap_bootstrap(long kvo, paddr_t lpt1,  
>   // enable mappings for existing 'allocated' mapping in the bootstrap
>   // page tables
>   extern uint64_t *pagetable;
> - extern int *_end;
> + extern char __text_start[], _etext[];
> + extern char __rodata_start[], _erodata[];
> + extern char _end[];
>   vp2 = (void *)((long)&pagetable + kvo);
>   struct mem_region *mp;
>   ssize_t size;
> @@ -1234,11 +1257,21 @@ pmap_bootstrap(long kvo, paddr_t lpt1,  
>   {
>   paddr_t mappa = pa & ~(L2_SIZE-1);
>   vaddr_t mapva = mappa - kvo;
> - if (mapva < (vaddr_t)&_end)
> + int prot = PROT_READ | PROT_WRITE;
> +
> + if (mapva < (vaddr_t)_end)
>   continue;
> +
> + if (mapva >= (vaddr_t)__text_start &&
> +    mapva < (vaddr_t)_etext)
> + prot = PROT_READ | PROT_EXEC;
> + else if (mapva >= (vaddr_t)__rodata_start &&
> + mapva < (vaddr_t)_erodata)

The spacing looks off (but at least aligned to the line above).
Reads fine to me otherwise.

> + prot = PROT_READ;
> +
>   vp2->l2[VP_IDX2(mapva)] = mappa | L2_BLOCK |
> -    ATTR_AF | ATTR_IDX(PTE_ATTR_WB) |
> -    ATTR_SH(SH_INNER);
> +    ATTR_IDX(PTE_ATTR_WB) | ATTR_SH(SH_INNER) |
> +    ap_bits_kern[prot];
>   }
>   }
>  
> @@ -1572,28 +1605,6 @@ pmap_proc_iflush(struct process *pr, vad
>   if (pr == curproc->p_p)
>   cpu_icache_sync_range(va, len);
>  }
> -
> -STATIC uint64_t ap_bits_user[8] = {
> - [PROT_NONE] = ATTR_nG|ATTR_PXN|ATTR_UXN|ATTR_AP(2),
> - [PROT_READ] = ATTR_nG|ATTR_PXN|ATTR_UXN|ATTR_AF|ATTR_AP(3),
> - [PROT_WRITE] = ATTR_nG|ATTR_PXN|ATTR_UXN|ATTR_AF|ATTR_AP(1),
> - [PROT_WRITE|PROT_READ] = ATTR_nG|ATTR_PXN|ATTR_UXN|ATTR_AF|ATTR_AP(1),
> - [PROT_EXEC] = ATTR_nG|ATTR_PXN|ATTR_AF|ATTR_AP(2),
> - [PROT_EXEC|PROT_READ] = ATTR_nG|ATTR_PXN|ATTR_AF|ATTR_AP(3),
> - [PROT_EXEC|PROT_WRITE] = ATTR_nG|ATTR_PXN|ATTR_AF|ATTR_AP(1),
> - [PROT_EXEC|PROT_WRITE|PROT_READ]= ATTR_nG|ATTR_PXN|ATTR_AF|ATTR_AP(1),
> -};
> -
> -STATIC uint64_t ap_bits_kern[8] = {
> - [PROT_NONE] = ATTR_PXN|ATTR_UXN|ATTR_AP(2),
> - [PROT_READ] = ATTR_PXN|ATTR_UXN|ATTR_AF|ATTR_AP(2),
> - [PROT_WRITE] = ATTR_PXN|ATTR_UXN|ATTR_AF|ATTR_AP(0),
> - [PROT_WRITE|PROT_READ] = ATTR_PXN|ATTR_UXN|ATTR_AF|ATTR_AP(0),
> - [PROT_EXEC] = ATTR_UXN|ATTR_AF|ATTR_AP(2),
> - [PROT_EXEC|PROT_READ] = ATTR_UXN|ATTR_AF|ATTR_AP(2),
> - [PROT_EXEC|PROT_WRITE] = ATTR_UXN|ATTR_AF|ATTR_AP(0),
> - [PROT_EXEC|PROT_WRITE|PROT_READ] = ATTR_UXN|ATTR_AF|ATTR_AP(0),
> -};
>  
>  void
>  pmap_pte_insert(struct pte_desc *pted)
> Index: arch/arm64/conf/kern.ldscript
> ===================================================================
> RCS file: /cvs/src/sys/arch/arm64/conf/kern.ldscript,v
> retrieving revision 1.4
> diff -u -p -r1.4 kern.ldscript
> --- arch/arm64/conf/kern.ldscript 23 Jan 2017 14:11:21 -0000 1.4
> +++ arch/arm64/conf/kern.ldscript 17 Mar 2017 17:39:05 -0000
> @@ -12,34 +12,35 @@ PHDRS
>   openbsd_randomize PT_OPENBSD_RANDOMIZE;
>  }
>  
> -__ALIGN_SIZE = 0x1000;
> +__ALIGN_SIZE = 0x200000;
>  __kernel_base = @KERNEL_BASE_VIRT@;
>  
>  ENTRY(_start)
>  SECTIONS
>  {
>   . = __kernel_base;
> + PROVIDE (__text_start = .);
>   .text :
>   {
>   *(.text .text.*)
>   *(.stub)
>   *(.glue_7t) *(.glue_7)
>   } :text =0
> - PROVIDE (__etext = .);
>   PROVIDE (_etext = .);
>   PROVIDE (etext = .);
>  
>   /* Move rodata to the next page, so we can nuke X and W bit on it */
>   . = ALIGN(__ALIGN_SIZE);
> + PROVIDE (__rodata_start = .);
>   .rodata :
>   {
>   *(.rodata .rodata.*)
>   } :rodata
> - PROVIDE (erodata = .);
> - _erodata = .;
> + PROVIDE (_erodata = .);
>  
>   /* Move .random to the next page, so we can add W bit on it and .data */
>   . = ALIGN(__ALIGN_SIZE);
> + PROVIDE (__data_start = .);
>   .openbsd.randomdata :
>   {
>   *(.openbsd.randomdata)
> @@ -52,8 +53,7 @@ SECTIONS
>   {
>   *(.sdata .sdata.*)
>   } :data
> - PROVIDE (edata = .);
> - _edata = .;
> + PROVIDE (_edata = .);
>  
>   PROVIDE (__bss_start = .);
>   .sbss :
> @@ -74,6 +74,6 @@ SECTIONS
>     .bss section disappears because there are no input sections.  */
>   . = ALIGN(64 / 8);
>   } :data
> + PROVIDE (_end = .);
>   PROVIDE (end = .);
> - _end = .;
>  }
>

Reply | Threaded
Open this post in threaded view
|

Re: arm64 kernel W^X

Mark Kettenis
> Date: Fri, 17 Mar 2017 19:07:16 +0100
> From: Patrick Wildt <[hidden email]>
>
> On Fri, Mar 17, 2017 at 06:42:38PM +0100, Mark Kettenis wrote:
> > The diff below introduces kernel W^X on arm64.  It changes the linker
> > script to align the RX, R and RW segments on a 2MB boundary such that
> > we can continue to use block mappings for the kernel.  Then it adds
> > some additional linker generated symbols that allow us to determine
> > whether a block is .text, .rodata or .data/.bss.
> >
> > ok?
> >
> >
> > Index: arch/arm64/arm64/pmap.c
> > ===================================================================
> > RCS file: /cvs/src/sys/arch/arm64/arm64/pmap.c,v
> > retrieving revision 1.26
> > diff -u -p -r1.26 pmap.c
> > --- arch/arm64/arm64/pmap.c 16 Mar 2017 20:15:07 -0000 1.26
> > +++ arch/arm64/arm64/pmap.c 17 Mar 2017 17:39:05 -0000
> > @@ -255,6 +255,27 @@ const struct kmem_pa_mode kp_lN = {
> >   .kp_zero = 1,
> >  };
> >  
> > +const uint64_t ap_bits_user[8] = {
> > + [PROT_NONE] = ATTR_nG|ATTR_PXN|ATTR_UXN|ATTR_AP(2),
> > + [PROT_READ] = ATTR_nG|ATTR_PXN|ATTR_UXN|ATTR_AF|ATTR_AP(3),
> > + [PROT_WRITE] = ATTR_nG|ATTR_PXN|ATTR_UXN|ATTR_AF|ATTR_AP(1),
> > + [PROT_WRITE|PROT_READ] = ATTR_nG|ATTR_PXN|ATTR_UXN|ATTR_AF|ATTR_AP(1),
> > + [PROT_EXEC] = ATTR_nG|ATTR_PXN|ATTR_AF|ATTR_AP(2),
> > + [PROT_EXEC|PROT_READ] = ATTR_nG|ATTR_PXN|ATTR_AF|ATTR_AP(3),
> > + [PROT_EXEC|PROT_WRITE] = ATTR_nG|ATTR_PXN|ATTR_AF|ATTR_AP(1),
> > + [PROT_EXEC|PROT_WRITE|PROT_READ]= ATTR_nG|ATTR_PXN|ATTR_AF|ATTR_AP(1),
> > +};
> > +
> > +const uint64_t ap_bits_kern[8] = {
> > + [PROT_NONE] = ATTR_PXN|ATTR_UXN|ATTR_AP(2),
> > + [PROT_READ] = ATTR_PXN|ATTR_UXN|ATTR_AF|ATTR_AP(2),
> > + [PROT_WRITE] = ATTR_PXN|ATTR_UXN|ATTR_AF|ATTR_AP(0),
> > + [PROT_WRITE|PROT_READ] = ATTR_PXN|ATTR_UXN|ATTR_AF|ATTR_AP(0),
> > + [PROT_EXEC] = ATTR_UXN|ATTR_AF|ATTR_AP(2),
> > + [PROT_EXEC|PROT_READ] = ATTR_UXN|ATTR_AF|ATTR_AP(2),
> > + [PROT_EXEC|PROT_WRITE] = ATTR_UXN|ATTR_AF|ATTR_AP(0),
> > + [PROT_EXEC|PROT_WRITE|PROT_READ] = ATTR_UXN|ATTR_AF|ATTR_AP(0),
> > +};
>
> Is this only a move of the lists or is there also a change involved?
> It's a bit hard to compare.

Only a move, but I did make it const and dropped the STATIC.

> >  /*
> >   * This is used for pmap_kernel() mappings, they are not to be removed
> > @@ -1222,7 +1243,9 @@ pmap_bootstrap(long kvo, paddr_t lpt1,  
> >   // enable mappings for existing 'allocated' mapping in the bootstrap
> >   // page tables
> >   extern uint64_t *pagetable;
> > - extern int *_end;
> > + extern char __text_start[], _etext[];
> > + extern char __rodata_start[], _erodata[];
> > + extern char _end[];
> >   vp2 = (void *)((long)&pagetable + kvo);
> >   struct mem_region *mp;
> >   ssize_t size;
> > @@ -1234,11 +1257,21 @@ pmap_bootstrap(long kvo, paddr_t lpt1,  
> >   {
> >   paddr_t mappa = pa & ~(L2_SIZE-1);
> >   vaddr_t mapva = mappa - kvo;
> > - if (mapva < (vaddr_t)&_end)
> > + int prot = PROT_READ | PROT_WRITE;
> > +
> > + if (mapva < (vaddr_t)_end)
> >   continue;
> > +
> > + if (mapva >= (vaddr_t)__text_start &&
> > +    mapva < (vaddr_t)_etext)
> > + prot = PROT_READ | PROT_EXEC;
> > + else if (mapva >= (vaddr_t)__rodata_start &&
> > + mapva < (vaddr_t)_erodata)
>
> The spacing looks off (but at least aligned to the line above).
> Reads fine to me otherwise.

Good catch.