[PATCH 0/5] arm/arm64: KVM: Tighten memory protection flags

[PATCH 0/5] arm/arm64: KVM: Tighten memory protection flags

[Marc Zyngier]

So far, the HYP mappings have been fairly relaxed: everything is
RWX. Oddly enough, not everybody is fond of this kind of permissions
at the highest exception level.

This small series tightens it a bit by making:
- the text mapping read-only
- the rodata mapping read-only + no-exec
- everything else read-write + no-exec

Of course, that's only valid when VHE is not in action. Tested on
Seattle and Cubietruck, based on 4.7-rc2.

Thanks,

	M.

Marc Zyngier (5):
  arm/arm64: KVM: Add a protection parameter to create_hyp_mappings
  arm64: Add PTE_HYP_XN page table flag
  arm/arm64: KVM: Enforce HYP read-only mapping of the kernel's rodata
    section
  arm/arm64: KVM: Map the HYP text as read-only
  arm/arm64: KVM: Make default HYP mappings non-excutable

 arch/arm/include/asm/kvm_mmu.h         |  2 +-
 arch/arm/include/asm/pgtable.h         |  4 +++-
 arch/arm/kvm/arm.c                     | 13 +++++++------
 arch/arm/kvm/mmu.c                     |  7 ++++---
 arch/arm64/include/asm/kvm_mmu.h       |  2 +-
 arch/arm64/include/asm/pgtable-hwdef.h |  1 +
 arch/arm64/include/asm/pgtable-prot.h  |  4 +++-
 7 files changed, 20 insertions(+), 13 deletions(-)

-- 
2.1.4

[PATCH 1/5] arm/arm64: KVM: Add a protection parameter to create_hyp_mappings

[Marc Zyngier]

Currently, create_hyp_mappings applies a "one size fits all" page
protection (PAGE_HYP). As we're heading towards separate protections
for different sections, let's make this protection a parameter, and
let the callers pass their prefered protection (PAGE_HYP for everyone
for the time being).

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/include/asm/kvm_mmu.h   |  2 +-
 arch/arm/kvm/arm.c               | 13 +++++++------
 arch/arm/kvm/mmu.c               |  5 +++--
 arch/arm64/include/asm/kvm_mmu.h |  2 +-
 4 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
index a0c6cf4..73c2818 100644
--- a/arch/arm/include/asm/kvm_mmu.h
+++ b/arch/arm/include/asm/kvm_mmu.h
@@ -40,7 +40,7 @@
 #include <asm/pgalloc.h>
 #include <asm/stage2_pgtable.h>
 
-int create_hyp_mappings(void *from, void *to);
+int create_hyp_mappings(void *from, void *to, pgprot_t prot);
 int create_hyp_io_mappings(void *from, void *to, phys_addr_t);
 void free_hyp_pgds(void);
 
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index fe20532..7978e15 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -122,7 +122,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 	if (ret)
 		goto out_fail_alloc;
 
-	ret = create_hyp_mappings(kvm, kvm + 1);
+	ret = create_hyp_mappings(kvm, kvm + 1, PAGE_HYP);
 	if (ret)
 		goto out_free_stage2_pgd;
 
@@ -239,7 +239,7 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, unsigned int id)
 	if (err)
 		goto free_vcpu;
 
-	err = create_hyp_mappings(vcpu, vcpu + 1);
+	err = create_hyp_mappings(vcpu, vcpu + 1, PAGE_HYP);
 	if (err)
 		goto vcpu_uninit;
 
@@ -1285,14 +1285,14 @@ static int init_hyp_mode(void)
 	 * Map the Hyp-code called directly from the host
 	 */
 	err = create_hyp_mappings(kvm_ksym_ref(__hyp_text_start),
-				  kvm_ksym_ref(__hyp_text_end));
+				  kvm_ksym_ref(__hyp_text_end), PAGE_HYP);
 	if (err) {
 		kvm_err("Cannot map world-switch code\n");
 		goto out_err;
 	}
 
 	err = create_hyp_mappings(kvm_ksym_ref(__start_rodata),
-				  kvm_ksym_ref(__end_rodata));
+				  kvm_ksym_ref(__end_rodata), PAGE_HYP);
 	if (err) {
 		kvm_err("Cannot map rodata section\n");
 		goto out_err;
@@ -1303,7 +1303,8 @@ static int init_hyp_mode(void)
 	 */
 	for_each_possible_cpu(cpu) {
 		char *stack_page = (char *)per_cpu(kvm_arm_hyp_stack_page, cpu);
-		err = create_hyp_mappings(stack_page, stack_page + PAGE_SIZE);
+		err = create_hyp_mappings(stack_page, stack_page + PAGE_SIZE,
+					  PAGE_HYP);
 
 		if (err) {
 			kvm_err("Cannot map hyp stack\n");
@@ -1315,7 +1316,7 @@ static int init_hyp_mode(void)
 		kvm_cpu_context_t *cpu_ctxt;
 
 		cpu_ctxt = per_cpu_ptr(kvm_host_cpu_state, cpu);
-		err = create_hyp_mappings(cpu_ctxt, cpu_ctxt + 1);
+		err = create_hyp_mappings(cpu_ctxt, cpu_ctxt + 1, PAGE_HYP);
 
 		if (err) {
 			kvm_err("Cannot map host CPU state: %d\n", err);
diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index 819517d..6023abd 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -660,12 +660,13 @@ static phys_addr_t kvm_kaddr_to_phys(void *kaddr)
  * create_hyp_mappings - duplicate a kernel virtual address range in Hyp mode
  * @from:	The virtual kernel start address of the range
  * @to:		The virtual kernel end address of the range (exclusive)
+ * @prot:	The protection to be applied to this range
  *
  * The same virtual address as the kernel virtual address is also used
  * in Hyp-mode mapping (modulo HYP_PAGE_OFFSET) to the same underlying
  * physical pages.
  */
-int create_hyp_mappings(void *from, void *to)
+int create_hyp_mappings(void *from, void *to, pgprot_t prot)
 {
 	phys_addr_t phys_addr;
 	unsigned long virt_addr;
@@ -685,7 +686,7 @@ int create_hyp_mappings(void *from, void *to)
 		err = __create_hyp_mappings(hyp_pgd, virt_addr,
 					    virt_addr + PAGE_SIZE,
 					    __phys_to_pfn(phys_addr),
-					    PAGE_HYP);
+					    prot);
 		if (err)
 			return err;
 	}
diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
index 5b8a5622..e1f7520 100644
--- a/arch/arm64/include/asm/kvm_mmu.h
+++ b/arch/arm64/include/asm/kvm_mmu.h
@@ -149,7 +149,7 @@ static inline unsigned long __kern_hyp_va(unsigned long v)
 
 #include <asm/stage2_pgtable.h>
 
-int create_hyp_mappings(void *from, void *to);
+int create_hyp_mappings(void *from, void *to, pgprot_t prot);
 int create_hyp_io_mappings(void *from, void *to, phys_addr_t);
 void free_hyp_pgds(void);
 
-- 
2.1.4

[PATCH 2/5] arm64: Add PTE_HYP_XN page table flag

[Marc Zyngier]

EL2 page tables can be configured to deny code from being
executed, which is done by setting bit 54 in the page descriptor.

It is the same bit as PTE_UXN, but the "USER" reference felt odd
in the hypervisor code.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/pgtable-hwdef.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/include/asm/pgtable-hwdef.h b/arch/arm64/include/asm/pgtable-hwdef.h
index 2813748..c3ae239 100644
--- a/arch/arm64/include/asm/pgtable-hwdef.h
+++ b/arch/arm64/include/asm/pgtable-hwdef.h
@@ -164,6 +164,7 @@
 #define PTE_CONT		(_AT(pteval_t, 1) << 52)	/* Contiguous range */
 #define PTE_PXN			(_AT(pteval_t, 1) << 53)	/* Privileged XN */
 #define PTE_UXN			(_AT(pteval_t, 1) << 54)	/* User XN */
+#define PTE_HYP_XN		(_AT(pteval_t, 1) << 54)	/* HYP XN */
 
 /*
  * AttrIndx[2:0] encoding (mapping attributes defined in the MAIR* registers).
-- 
2.1.4

[PATCH 3/5] arm/arm64: KVM: Enforce HYP read-only mapping of the kernel's rodata section

[Marc Zyngier]

In order to be able to use C code in HYP, we're now mapping the kernel's
rodata in HYP. It works absolutely fine, except that we're mapping it RWX,
which is not what it should be.

Add a new HYP_PAGE_RO protection, and pass it as the protection flags
when mapping the rodata section.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/include/asm/pgtable.h        | 1 +
 arch/arm/kvm/arm.c                    | 2 +-
 arch/arm64/include/asm/pgtable-prot.h | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
index 348caab..f332087 100644
--- a/arch/arm/include/asm/pgtable.h
+++ b/arch/arm/include/asm/pgtable.h
@@ -98,6 +98,7 @@ extern pgprot_t		pgprot_s2_device;
 #define PAGE_KERNEL		_MOD_PROT(pgprot_kernel, L_PTE_XN)
 #define PAGE_KERNEL_EXEC	pgprot_kernel
 #define PAGE_HYP		_MOD_PROT(pgprot_kernel, L_PTE_HYP)
+#define PAGE_HYP_RO		_MOD_PROT(pgprot_kernel, L_PTE_HYP | L_PTE_RDONLY | L_PTE_XN)
 #define PAGE_HYP_DEVICE		_MOD_PROT(pgprot_hyp_device, L_PTE_HYP)
 #define PAGE_S2			_MOD_PROT(pgprot_s2, L_PTE_S2_RDONLY)
 #define PAGE_S2_DEVICE		_MOD_PROT(pgprot_s2_device, L_PTE_S2_RDONLY)
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 7978e15..0b5099f 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -1292,7 +1292,7 @@ static int init_hyp_mode(void)
 	}
 
 	err = create_hyp_mappings(kvm_ksym_ref(__start_rodata),
-				  kvm_ksym_ref(__end_rodata), PAGE_HYP);
+				  kvm_ksym_ref(__end_rodata), PAGE_HYP_RO);
 	if (err) {
 		kvm_err("Cannot map rodata section\n");
 		goto out_err;
diff --git a/arch/arm64/include/asm/pgtable-prot.h b/arch/arm64/include/asm/pgtable-prot.h
index 29fcb33..88db58c 100644
--- a/arch/arm64/include/asm/pgtable-prot.h
+++ b/arch/arm64/include/asm/pgtable-prot.h
@@ -56,6 +56,7 @@
 #define PAGE_KERNEL_EXEC_CONT	__pgprot(_PAGE_DEFAULT | PTE_UXN | PTE_DIRTY | PTE_WRITE | PTE_CONT)
 
 #define PAGE_HYP		__pgprot(_PAGE_DEFAULT | PTE_HYP)
+#define PAGE_HYP_RO		__pgprot(_PAGE_DEFAULT | PTE_HYP | PTE_RDONLY | PTE_HYP_XN)
 #define PAGE_HYP_DEVICE		__pgprot(PROT_DEVICE_nGnRE | PTE_HYP)
 
 #define PAGE_S2			__pgprot(PROT_DEFAULT | PTE_S2_MEMATTR(MT_S2_NORMAL) | PTE_S2_RDONLY)
-- 
2.1.4

[PATCH 4/5] arm/arm64: KVM: Map the HYP text as read-only

[Marc Zyngier]

There should be no reason for mapping the HYP text read/write.

As such, let's have a new set of flags (PAGE_HYP_EXEC) that allows
execution, but makes the page as read-only, and update the two call
sites that deal with mapping code.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/include/asm/pgtable.h        | 1 +
 arch/arm/kvm/arm.c                    | 2 +-
 arch/arm/kvm/mmu.c                    | 2 +-
 arch/arm64/include/asm/pgtable-prot.h | 1 +
 4 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
index f332087..7487bf9 100644
--- a/arch/arm/include/asm/pgtable.h
+++ b/arch/arm/include/asm/pgtable.h
@@ -98,6 +98,7 @@ extern pgprot_t		pgprot_s2_device;
 #define PAGE_KERNEL		_MOD_PROT(pgprot_kernel, L_PTE_XN)
 #define PAGE_KERNEL_EXEC	pgprot_kernel
 #define PAGE_HYP		_MOD_PROT(pgprot_kernel, L_PTE_HYP)
+#define PAGE_HYP_EXEC		_MOD_PROT(pgprot_kernel, L_PTE_HYP | L_PTE_RDONLY)
 #define PAGE_HYP_RO		_MOD_PROT(pgprot_kernel, L_PTE_HYP | L_PTE_RDONLY | L_PTE_XN)
 #define PAGE_HYP_DEVICE		_MOD_PROT(pgprot_hyp_device, L_PTE_HYP)
 #define PAGE_S2			_MOD_PROT(pgprot_s2, L_PTE_S2_RDONLY)
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 0b5099f..d7bf2dd 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -1285,7 +1285,7 @@ static int init_hyp_mode(void)
 	 * Map the Hyp-code called directly from the host
 	 */
 	err = create_hyp_mappings(kvm_ksym_ref(__hyp_text_start),
-				  kvm_ksym_ref(__hyp_text_end), PAGE_HYP);
+				  kvm_ksym_ref(__hyp_text_end), PAGE_HYP_EXEC);
 	if (err) {
 		kvm_err("Cannot map world-switch code\n");
 		goto out_err;
diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index 6023abd..8a0aa37 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -1687,7 +1687,7 @@ static int kvm_map_idmap_text(pgd_t *pgd)
 	err = 	__create_hyp_mappings(pgd,
 				      hyp_idmap_start, hyp_idmap_end,
 				      __phys_to_pfn(hyp_idmap_start),
-				      PAGE_HYP);
+				      PAGE_HYP_EXEC);
 	if (err)
 		kvm_err("Failed to idmap %lx-%lx\n",
 			hyp_idmap_start, hyp_idmap_end);
diff --git a/arch/arm64/include/asm/pgtable-prot.h b/arch/arm64/include/asm/pgtable-prot.h
index 88db58c..3802048 100644
--- a/arch/arm64/include/asm/pgtable-prot.h
+++ b/arch/arm64/include/asm/pgtable-prot.h
@@ -56,6 +56,7 @@
 #define PAGE_KERNEL_EXEC_CONT	__pgprot(_PAGE_DEFAULT | PTE_UXN | PTE_DIRTY | PTE_WRITE | PTE_CONT)
 
 #define PAGE_HYP		__pgprot(_PAGE_DEFAULT | PTE_HYP)
+#define PAGE_HYP_EXEC		__pgprot(_PAGE_DEFAULT | PTE_HYP | PTE_RDONLY)
 #define PAGE_HYP_RO		__pgprot(_PAGE_DEFAULT | PTE_HYP | PTE_RDONLY | PTE_HYP_XN)
 #define PAGE_HYP_DEVICE		__pgprot(PROT_DEVICE_nGnRE | PTE_HYP)
 
-- 
2.1.4

[PATCH 4/5] arm/arm64: KVM: Map the HYP text as read-only

[Mark Rutland]

On Mon, Jun 13, 2016 at 03:00:48PM +0100, Marc Zyngier wrote:
> There should be no reason for mapping the HYP text read/write.
> 
> As such, let's have a new set of flags (PAGE_HYP_EXEC) that allows
> execution, but makes the page as read-only, and update the two call
> sites that deal with mapping code.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm/include/asm/pgtable.h        | 1 +
>  arch/arm/kvm/arm.c                    | 2 +-
>  arch/arm/kvm/mmu.c                    | 2 +-
>  arch/arm64/include/asm/pgtable-prot.h | 1 +
>  4 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
> index f332087..7487bf9 100644
> --- a/arch/arm/include/asm/pgtable.h
> +++ b/arch/arm/include/asm/pgtable.h
> @@ -98,6 +98,7 @@ extern pgprot_t		pgprot_s2_device;
>  #define PAGE_KERNEL		_MOD_PROT(pgprot_kernel, L_PTE_XN)
>  #define PAGE_KERNEL_EXEC	pgprot_kernel
>  #define PAGE_HYP		_MOD_PROT(pgprot_kernel, L_PTE_HYP)
> +#define PAGE_HYP_EXEC		_MOD_PROT(pgprot_kernel, L_PTE_HYP | L_PTE_RDONLY)
>  #define PAGE_HYP_RO		_MOD_PROT(pgprot_kernel, L_PTE_HYP | L_PTE_RDONLY | L_PTE_XN)
>  #define PAGE_HYP_DEVICE		_MOD_PROT(pgprot_hyp_device, L_PTE_HYP)
>  #define PAGE_S2			_MOD_PROT(pgprot_s2, L_PTE_S2_RDONLY)
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 0b5099f..d7bf2dd 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -1285,7 +1285,7 @@ static int init_hyp_mode(void)
>  	 * Map the Hyp-code called directly from the host
>  	 */
>  	err = create_hyp_mappings(kvm_ksym_ref(__hyp_text_start),
> -				  kvm_ksym_ref(__hyp_text_end), PAGE_HYP);
> +				  kvm_ksym_ref(__hyp_text_end), PAGE_HYP_EXEC);

As far as I can tell, __hyp_text_{start,end} aren't guaranteed to be
page-aligned. Are we certain that we'll never share a page with
something that we'll subsequently want to map non-executable?

Otherwise this looks good!

Thanks,
Mark.

>  	if (err) {
>  		kvm_err("Cannot map world-switch code\n");
>  		goto out_err;
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index 6023abd..8a0aa37 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -1687,7 +1687,7 @@ static int kvm_map_idmap_text(pgd_t *pgd)
>  	err = 	__create_hyp_mappings(pgd,
>  				      hyp_idmap_start, hyp_idmap_end,
>  				      __phys_to_pfn(hyp_idmap_start),
> -				      PAGE_HYP);
> +				      PAGE_HYP_EXEC);
>  	if (err)
>  		kvm_err("Failed to idmap %lx-%lx\n",
>  			hyp_idmap_start, hyp_idmap_end);
> diff --git a/arch/arm64/include/asm/pgtable-prot.h b/arch/arm64/include/asm/pgtable-prot.h
> index 88db58c..3802048 100644
> --- a/arch/arm64/include/asm/pgtable-prot.h
> +++ b/arch/arm64/include/asm/pgtable-prot.h
> @@ -56,6 +56,7 @@
>  #define PAGE_KERNEL_EXEC_CONT	__pgprot(_PAGE_DEFAULT | PTE_UXN | PTE_DIRTY | PTE_WRITE | PTE_CONT)
>  
>  #define PAGE_HYP		__pgprot(_PAGE_DEFAULT | PTE_HYP)
> +#define PAGE_HYP_EXEC		__pgprot(_PAGE_DEFAULT | PTE_HYP | PTE_RDONLY)
>  #define PAGE_HYP_RO		__pgprot(_PAGE_DEFAULT | PTE_HYP | PTE_RDONLY | PTE_HYP_XN)
>  #define PAGE_HYP_DEVICE		__pgprot(PROT_DEVICE_nGnRE | PTE_HYP)
>  
> -- 
> 2.1.4
> 

[PATCH 4/5] arm/arm64: KVM: Map the HYP text as read-only

[Marc Zyngier]

On 13/06/16 16:02, Mark Rutland wrote:
> On Mon, Jun 13, 2016 at 03:00:48PM +0100, Marc Zyngier wrote:
>> There should be no reason for mapping the HYP text read/write.
>>
>> As such, let's have a new set of flags (PAGE_HYP_EXEC) that allows
>> execution, but makes the page as read-only, and update the two call
>> sites that deal with mapping code.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  arch/arm/include/asm/pgtable.h        | 1 +
>>  arch/arm/kvm/arm.c                    | 2 +-
>>  arch/arm/kvm/mmu.c                    | 2 +-
>>  arch/arm64/include/asm/pgtable-prot.h | 1 +
>>  4 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
>> index f332087..7487bf9 100644
>> --- a/arch/arm/include/asm/pgtable.h
>> +++ b/arch/arm/include/asm/pgtable.h
>> @@ -98,6 +98,7 @@ extern pgprot_t		pgprot_s2_device;
>>  #define PAGE_KERNEL		_MOD_PROT(pgprot_kernel, L_PTE_XN)
>>  #define PAGE_KERNEL_EXEC	pgprot_kernel
>>  #define PAGE_HYP		_MOD_PROT(pgprot_kernel, L_PTE_HYP)
>> +#define PAGE_HYP_EXEC		_MOD_PROT(pgprot_kernel, L_PTE_HYP | L_PTE_RDONLY)
>>  #define PAGE_HYP_RO		_MOD_PROT(pgprot_kernel, L_PTE_HYP | L_PTE_RDONLY | L_PTE_XN)
>>  #define PAGE_HYP_DEVICE		_MOD_PROT(pgprot_hyp_device, L_PTE_HYP)
>>  #define PAGE_S2			_MOD_PROT(pgprot_s2, L_PTE_S2_RDONLY)
>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> index 0b5099f..d7bf2dd 100644
>> --- a/arch/arm/kvm/arm.c
>> +++ b/arch/arm/kvm/arm.c
>> @@ -1285,7 +1285,7 @@ static int init_hyp_mode(void)
>>  	 * Map the Hyp-code called directly from the host
>>  	 */
>>  	err = create_hyp_mappings(kvm_ksym_ref(__hyp_text_start),
>> -				  kvm_ksym_ref(__hyp_text_end), PAGE_HYP);
>> +				  kvm_ksym_ref(__hyp_text_end), PAGE_HYP_EXEC);
> 
> As far as I can tell, __hyp_text_{start,end} aren't guaranteed to be
> page-aligned. Are we certain that we'll never share a page with
> something that we'll subsequently want to map non-executable?

The HYP text sits firmly in the middle of the rest of the text, so I'm
not too worried. We could force some additional alignment just in case
we end-up moving things around, but I don't see it as a problem we can
have today.

> Otherwise this looks good!

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 4/5] arm/arm64: KVM: Map the HYP text as read-only

[Mark Rutland]

On Mon, Jun 13, 2016 at 04:16:25PM +0100, Marc Zyngier wrote:
> On 13/06/16 16:02, Mark Rutland wrote:
> > On Mon, Jun 13, 2016 at 03:00:48PM +0100, Marc Zyngier wrote:
> >> There should be no reason for mapping the HYP text read/write.
> >>
> >> As such, let's have a new set of flags (PAGE_HYP_EXEC) that allows
> >> execution, but makes the page as read-only, and update the two call
> >> sites that deal with mapping code.
> >>
> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> >> ---
> >>  arch/arm/include/asm/pgtable.h        | 1 +
> >>  arch/arm/kvm/arm.c                    | 2 +-
> >>  arch/arm/kvm/mmu.c                    | 2 +-
> >>  arch/arm64/include/asm/pgtable-prot.h | 1 +
> >>  4 files changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
> >> index f332087..7487bf9 100644
> >> --- a/arch/arm/include/asm/pgtable.h
> >> +++ b/arch/arm/include/asm/pgtable.h
> >> @@ -98,6 +98,7 @@ extern pgprot_t		pgprot_s2_device;
> >>  #define PAGE_KERNEL		_MOD_PROT(pgprot_kernel, L_PTE_XN)
> >>  #define PAGE_KERNEL_EXEC	pgprot_kernel
> >>  #define PAGE_HYP		_MOD_PROT(pgprot_kernel, L_PTE_HYP)
> >> +#define PAGE_HYP_EXEC		_MOD_PROT(pgprot_kernel, L_PTE_HYP | L_PTE_RDONLY)
> >>  #define PAGE_HYP_RO		_MOD_PROT(pgprot_kernel, L_PTE_HYP | L_PTE_RDONLY | L_PTE_XN)
> >>  #define PAGE_HYP_DEVICE		_MOD_PROT(pgprot_hyp_device, L_PTE_HYP)
> >>  #define PAGE_S2			_MOD_PROT(pgprot_s2, L_PTE_S2_RDONLY)
> >> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> >> index 0b5099f..d7bf2dd 100644
> >> --- a/arch/arm/kvm/arm.c
> >> +++ b/arch/arm/kvm/arm.c
> >> @@ -1285,7 +1285,7 @@ static int init_hyp_mode(void)
> >>  	 * Map the Hyp-code called directly from the host
> >>  	 */
> >>  	err = create_hyp_mappings(kvm_ksym_ref(__hyp_text_start),
> >> -				  kvm_ksym_ref(__hyp_text_end), PAGE_HYP);
> >> +				  kvm_ksym_ref(__hyp_text_end), PAGE_HYP_EXEC);
> > 
> > As far as I can tell, __hyp_text_{start,end} aren't guaranteed to be
> > page-aligned. Are we certain that we'll never share a page with
> > something that we'll subsequently want to map non-executable?
> 
> The HYP text sits firmly in the middle of the rest of the text, so I'm
> not too worried. We could force some additional alignment just in case
> we end-up moving things around, but I don't see it as a problem we can
> have today.

Sure, jsut though I should check.

Given that, FWIW, for the series:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

[PATCH 5/5] arm/arm64: KVM: Make default HYP mappings non-excutable

[Marc Zyngier]

Structures that can be generally written to don't have any requirement
to be executable (quite the opposite). This includes the kvm and vcpu
structures, as well as the stacks.

Let's change the default to incorporate the XN flag.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/include/asm/pgtable.h        | 2 +-
 arch/arm64/include/asm/pgtable-prot.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
index 7487bf9..e0d76ba 100644
--- a/arch/arm/include/asm/pgtable.h
+++ b/arch/arm/include/asm/pgtable.h
@@ -97,7 +97,7 @@ extern pgprot_t		pgprot_s2_device;
 #define PAGE_READONLY_EXEC	_MOD_PROT(pgprot_user, L_PTE_USER | L_PTE_RDONLY)
 #define PAGE_KERNEL		_MOD_PROT(pgprot_kernel, L_PTE_XN)
 #define PAGE_KERNEL_EXEC	pgprot_kernel
-#define PAGE_HYP		_MOD_PROT(pgprot_kernel, L_PTE_HYP)
+#define PAGE_HYP		_MOD_PROT(pgprot_kernel, L_PTE_HYP | L_PTE_XN)
 #define PAGE_HYP_EXEC		_MOD_PROT(pgprot_kernel, L_PTE_HYP | L_PTE_RDONLY)
 #define PAGE_HYP_RO		_MOD_PROT(pgprot_kernel, L_PTE_HYP | L_PTE_RDONLY | L_PTE_XN)
 #define PAGE_HYP_DEVICE		_MOD_PROT(pgprot_hyp_device, L_PTE_HYP)
diff --git a/arch/arm64/include/asm/pgtable-prot.h b/arch/arm64/include/asm/pgtable-prot.h
index 3802048..39f5252 100644
--- a/arch/arm64/include/asm/pgtable-prot.h
+++ b/arch/arm64/include/asm/pgtable-prot.h
@@ -55,7 +55,7 @@
 #define PAGE_KERNEL_EXEC	__pgprot(_PAGE_DEFAULT | PTE_UXN | PTE_DIRTY | PTE_WRITE)
 #define PAGE_KERNEL_EXEC_CONT	__pgprot(_PAGE_DEFAULT | PTE_UXN | PTE_DIRTY | PTE_WRITE | PTE_CONT)
 
-#define PAGE_HYP		__pgprot(_PAGE_DEFAULT | PTE_HYP)
+#define PAGE_HYP		__pgprot(_PAGE_DEFAULT | PTE_HYP | PTE_HYP_XN)
 #define PAGE_HYP_EXEC		__pgprot(_PAGE_DEFAULT | PTE_HYP | PTE_RDONLY)
 #define PAGE_HYP_RO		__pgprot(_PAGE_DEFAULT | PTE_HYP | PTE_RDONLY | PTE_HYP_XN)
 #define PAGE_HYP_DEVICE		__pgprot(PROT_DEVICE_nGnRE | PTE_HYP)
-- 
2.1.4

[PATCH 5/5] arm/arm64: KVM: Make default HYP mappings non-excutable

[Mark Rutland]

Hi,

Nit: typo in subject "excutable" is missing an 'e'.

Mark.

On Mon, Jun 13, 2016 at 03:00:49PM +0100, Marc Zyngier wrote:
> Structures that can be generally written to don't have any requirement
> to be executable (quite the opposite). This includes the kvm and vcpu
> structures, as well as the stacks.
> 
> Let's change the default to incorporate the XN flag.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm/include/asm/pgtable.h        | 2 +-
>  arch/arm64/include/asm/pgtable-prot.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
> index 7487bf9..e0d76ba 100644
> --- a/arch/arm/include/asm/pgtable.h
> +++ b/arch/arm/include/asm/pgtable.h
> @@ -97,7 +97,7 @@ extern pgprot_t		pgprot_s2_device;
>  #define PAGE_READONLY_EXEC	_MOD_PROT(pgprot_user, L_PTE_USER | L_PTE_RDONLY)
>  #define PAGE_KERNEL		_MOD_PROT(pgprot_kernel, L_PTE_XN)
>  #define PAGE_KERNEL_EXEC	pgprot_kernel
> -#define PAGE_HYP		_MOD_PROT(pgprot_kernel, L_PTE_HYP)
> +#define PAGE_HYP		_MOD_PROT(pgprot_kernel, L_PTE_HYP | L_PTE_XN)
>  #define PAGE_HYP_EXEC		_MOD_PROT(pgprot_kernel, L_PTE_HYP | L_PTE_RDONLY)
>  #define PAGE_HYP_RO		_MOD_PROT(pgprot_kernel, L_PTE_HYP | L_PTE_RDONLY | L_PTE_XN)
>  #define PAGE_HYP_DEVICE		_MOD_PROT(pgprot_hyp_device, L_PTE_HYP)
> diff --git a/arch/arm64/include/asm/pgtable-prot.h b/arch/arm64/include/asm/pgtable-prot.h
> index 3802048..39f5252 100644
> --- a/arch/arm64/include/asm/pgtable-prot.h
> +++ b/arch/arm64/include/asm/pgtable-prot.h
> @@ -55,7 +55,7 @@
>  #define PAGE_KERNEL_EXEC	__pgprot(_PAGE_DEFAULT | PTE_UXN | PTE_DIRTY | PTE_WRITE)
>  #define PAGE_KERNEL_EXEC_CONT	__pgprot(_PAGE_DEFAULT | PTE_UXN | PTE_DIRTY | PTE_WRITE | PTE_CONT)
>  
> -#define PAGE_HYP		__pgprot(_PAGE_DEFAULT | PTE_HYP)
> +#define PAGE_HYP		__pgprot(_PAGE_DEFAULT | PTE_HYP | PTE_HYP_XN)
>  #define PAGE_HYP_EXEC		__pgprot(_PAGE_DEFAULT | PTE_HYP | PTE_RDONLY)
>  #define PAGE_HYP_RO		__pgprot(_PAGE_DEFAULT | PTE_HYP | PTE_RDONLY | PTE_HYP_XN)
>  #define PAGE_HYP_DEVICE		__pgprot(PROT_DEVICE_nGnRE | PTE_HYP)
> -- 
> 2.1.4
> 

[PATCH 0/5] arm/arm64: KVM: Tighten memory protection flags

[Will Deacon]

On Mon, Jun 13, 2016 at 03:00:44PM +0100, Marc Zyngier wrote:
> So far, the HYP mappings have been fairly relaxed: everything is
> RWX. Oddly enough, not everybody is fond of this kind of permissions
> at the highest exception level.
> 
> This small series tightens it a bit by making:
> - the text mapping read-only
> - the rodata mapping read-only + no-exec
> - everything else read-write + no-exec
> 
> Of course, that's only valid when VHE is not in action. Tested on
> Seattle and Cubietruck, based on 4.7-rc2.

Looks good to me. For the series:

Reviewed-by: Will Deacon <will.deacon@arm.com>

Will

[PATCH 0/5] arm/arm64: KVM: Tighten memory protection flags

[Christoffer Dall]

Hi Marc,

On Mon, Jun 13, 2016 at 03:00:44PM +0100, Marc Zyngier wrote:
> So far, the HYP mappings have been fairly relaxed: everything is
> RWX. Oddly enough, not everybody is fond of this kind of permissions
> at the highest exception level.
> 
> This small series tightens it a bit by making:
> - the text mapping read-only
> - the rodata mapping read-only + no-exec
> - everything else read-write + no-exec
> 
> Of course, that's only valid when VHE is not in action. Tested on
> Seattle and Cubietruck, based on 4.7-rc2.
> 

I have applied this to queue with a RB on all the patches.

It seems like this series applies on top of the merged page tables
stuff, but since I think you're going to repost that, I have just
applied this and fixed up the conflicts.

Could you have a look at kvmarm/queue and let me know if you think it
looks good?  (and possibly rebase the merge page tables stuff on top of
there when you rework that series?)

Thanks!
-Christoffer