From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com [148.163.156.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 3yLl6y62qfzDqhg for ; Tue, 24 Oct 2017 18:20:58 +1100 (AEDT) Received: from pps.filterd (m0098409.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id v9O7JZCk141645 for ; Tue, 24 Oct 2017 03:20:56 -0400 Received: from e33.co.us.ibm.com (e33.co.us.ibm.com [32.97.110.151]) by mx0a-001b2d01.pphosted.com with ESMTP id 2dsxvh6ft8-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Tue, 24 Oct 2017 03:20:56 -0400 Received: from localhost by e33.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 24 Oct 2017 01:20:54 -0600 Date: Tue, 24 Oct 2017 00:20:44 -0700 From: Ram Pai To: "Aneesh Kumar K.V" Cc: Balbir Singh , mpe@ellerman.id.au, linuxppc-dev@lists.ozlabs.org, benh@kernel.crashing.org, paulus@samba.org, khandual@linux.vnet.ibm.com, hbabu@us.ibm.com, mhocko@kernel.org, bauerman@linux.vnet.ibm.com, ebiederm@xmission.com Subject: Re: [PATCH 09/25] powerpc: ability to create execute-disabled pkeys Reply-To: Ram Pai References: <1504910713-7094-1-git-send-email-linuxram@us.ibm.com> <1504910713-7094-18-git-send-email-linuxram@us.ibm.com> <20171018144256.72bdd785@firefly.ozlabs.ibm.com> <20171018051547.GD5617@ram.oc3035372033.ibm.com> <87wp3lvxpg.fsf@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <87wp3lvxpg.fsf@linux.vnet.ibm.com> Message-Id: <20171024072044.GH5454@ram.oc3035372033.ibm.com> List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Tue, Oct 24, 2017 at 12:28:35PM +0530, Aneesh Kumar K.V wrote: > Ram Pai writes: > > > On Wed, Oct 18, 2017 at 02:42:56PM +1100, Balbir Singh wrote: > >> On Fri, 8 Sep 2017 15:44:57 -0700 > >> Ram Pai wrote: > >> > >> > powerpc has hardware support to disable execute on a pkey. > >> > This patch enables the ability to create execute-disabled > >> > keys. > >> > > >> > Signed-off-by: Ram Pai > >> > --- > >> > arch/powerpc/include/uapi/asm/mman.h | 6 ++++++ > >> > arch/powerpc/mm/pkeys.c | 16 ++++++++++++++++ > >> > 2 files changed, 22 insertions(+), 0 deletions(-) > >> > > >> > diff --git a/arch/powerpc/include/uapi/asm/mman.h b/arch/powerpc/include/uapi/asm/mman.h > >> > index ab45cc2..f272b09 100644 > >> > --- a/arch/powerpc/include/uapi/asm/mman.h > >> > +++ b/arch/powerpc/include/uapi/asm/mman.h > >> > @@ -45,4 +45,10 @@ > >> > #define MAP_HUGE_1GB (30 << MAP_HUGE_SHIFT) /* 1GB HugeTLB Page */ > >> > #define MAP_HUGE_16GB (34 << MAP_HUGE_SHIFT) /* 16GB HugeTLB Page */ > >> > > >> > +/* override any generic PKEY Permission defines */ > >> > +#define PKEY_DISABLE_EXECUTE 0x4 > >> > +#undef PKEY_ACCESS_MASK > >> > +#define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS |\ > >> > + PKEY_DISABLE_WRITE |\ > >> > + PKEY_DISABLE_EXECUTE) > >> > #endif /* _UAPI_ASM_POWERPC_MMAN_H */ > >> > diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c > >> > index cc5be6a..2282864 100644 > >> > --- a/arch/powerpc/mm/pkeys.c > >> > +++ b/arch/powerpc/mm/pkeys.c > >> > @@ -24,6 +24,14 @@ void __init pkey_initialize(void) > >> > { > >> > int os_reserved, i; > >> > > >> > + /* > >> > + * we define PKEY_DISABLE_EXECUTE in addition to the arch-neutral > >> > + * generic defines for PKEY_DISABLE_ACCESS and PKEY_DISABLE_WRITE. > >> > + * Ensure that the bits a distinct. > >> > + */ > >> > + BUILD_BUG_ON(PKEY_DISABLE_EXECUTE & > >> > + (PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE)); > >> > >> Will these values every change? It's good to have I guess. > >> > >> > + > >> > /* disable the pkey system till everything > >> > * is in place. A patch further down the > >> > * line will enable it. > >> > @@ -120,10 +128,18 @@ int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey, > >> > unsigned long init_val) > >> > { > >> > u64 new_amr_bits = 0x0ul; > >> > + u64 new_iamr_bits = 0x0ul; > >> > > >> > if (!is_pkey_enabled(pkey)) > >> > return -EINVAL; > >> > > >> > + if ((init_val & PKEY_DISABLE_EXECUTE)) { > >> > + if (!pkey_execute_disable_support) > >> > + return -EINVAL; > >> > + new_iamr_bits |= IAMR_EX_BIT; > >> > + } > >> > + init_iamr(pkey, new_iamr_bits); > >> > + > >> > >> Where do we check the reserved keys? > > > > The main gate keeper against spurious keys are the system calls. > > sys_pkey_mprotect(), sys_pkey_free() and sys_pkey_modify() are the one > > that will check against reserved and unallocated keys. Once it has > > passed the check, all other internal functions trust the key values > > provided to them. I can put in additional checks but that will > > unnecessarily chew a few cpu cycles. > > > > Agree? > > > > BTW: you raise a good point though, I may have missed guarding against > > unallocated or reserved keys in sys_pkey_modify(). That was a power > > specific system call that I have introduced to change the permissions on > > a key. > > Why do you need a power specific syscall? We should ideally not require > anything powerpc specific in the application to use memory keys. If it > is for exectue only key, the programming model should remain same as the > other keys. The programming model has not changed. It continues to be the same. i.e a) allocate a key through sys_pkey_alloc() b) associate the key to a addressspace through sys_pkey_mprotect() c) change the permissions on the key by programming the AMR register as and when needed. d) free the key through sys_pkey_free() when done. the problem is with the programming of execute-permission on the key. x86 does not support the execute-permission and does not have the issue. powerpc supports execute-permission but unfortunately has not exposed that capability to userspace, because IAMR cannot be programmed from userspace. I have filled in that gap, by providing a power-specific system call called sys_pkey_modify(). It is a way to enable the exact same programming model on keys for execute-permissions. > > NOTE: I am not able to find patch that add sys_pkey_modify() Yes that patch was added only recently to my tree after consulting Michael Ellermen. I am yet to send out that patch. Will be doing so in my next version. RP > > -aneesh -- Ram Pai