| Message ID | 20190531233159.30992-7-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | security: x86/sgx: SGX vs. LSM | expand |
> From: Christopherson, Sean J > Sent: Friday, May 31, 2019 4:32 PM > > ...to support (the equivalent) of existing Linux Security Module functionality. > > Because SGX manually manages EPC memory, all enclave VMAs are backed by the same vm_file, > i.e. /dev/sgx/enclave, so that SGX can implement the necessary hooks to move pages in/out > of the EPC. And because EPC pages for any given enclave are fundamentally shared between > processes, i.e. > CoW semantics are not possible with EPC pages, /dev/sgx/enclave must always be MAP_SHARED. > Lastly, all real world enclaves will need read, write and execute permissions to EPC pages. > As a result, SGX does not play nice with existing LSM behavior as it is impossible to > apply policies to enclaves with any reasonable granularity, e.g. an LSM can deny access to > EPC altogether, but can't deny potentially dangerous behavior such as mapping pages RW->RW > or RWX. > > To give LSMs enough information to implement their policies without having to resort to > ugly things, e.g. holding a reference to the vm_file of each enclave page, require > userspace to explicitly state the allowed protections for each page (region), i.e. take > ALLOW_{READ,WRITE,EXEC} in the ADD_PAGES ioctl. > > The ALLOW_* flags will be passed to LSMs so that they can make informed decisions when the > enclave is being built, i.e. when the source vm_file is available. For example, SELinux's > EXECMOD permission can be required if an enclave is requesting both ALLOW_WRITE and > ALLOW_EXEC. > > Update the mmap()/mprotect() hooks to enforce the ALLOW_* protections, a la the standard > VM_MAY{READ,WRITE,EXEC} flags. > > The ALLOW_EXEC flag also has a second (important) use in that it can be used to prevent > loading an enclave from a noexec file system, on > SGX2 hardware (regardless of kernel support for SGX2), userspace could EADD from a noexec > path using read-only permissions and later mprotect() and ENCLU[EMODPE] the page to gain > execute permissions. By requiring ALLOW_EXEC up front, SGX will be able to enforce noexec > paths when building the enclave. ALLOW_* flags shall be kept internal to LSM. This patch is completely unnecessary.
On Fri, May 31, 2019 at 04:31:56PM -0700, Sean Christopherson wrote: > ...to support (the equivalent) of existing Linux Security Module > functionality. Long and short descriptions should be separate. Also this does not make any sense. LSM is a framework with a set of hook to make access decisions and there various implementations of it. How this replicates LSMs and why that even would be a goal? My guess is that you are trying to do something else. I'm just saying that the idea to do equivalent of LSMs to another subsystems would be insane if it was done. > always be MAP_SHARED. Lastly, all real world enclaves will need read, > write and execute permissions to EPC pages. As a result, SGX does not > play nice with existing LSM behavior as it is impossible to apply > policies to enclaves with any reasonable granularity, e.g. an LSM can > deny access to EPC altogether, but can't deny potentially dangerous > behavior such as mapping pages RW->RW or RWX. The mapping must be shared given that it is iomem but why enclave pages would need RWX for all pages? The information that is missing from this paragraph is the explanation why an LSM could not deny dangerous behavior in PTE level. > To give LSMs enough information to implement their policies without > having to resort to ugly things, e.g. holding a reference to the vm_file > of each enclave page, require userspace to explicitly state the allowed > protections for each page (region), i.e. take ALLOW_{READ,WRITE,EXEC} > in the ADD_PAGES ioctl. I would keep descriptions such as "ugly things" away from commit messages as it is easy way to be not clear and explicit what you are trying to say. > The ALLOW_* flags will be passed to LSMs so that they can make informed > decisions when the enclave is being built, i.e. when the source vm_file > is available. For example, SELinux's EXECMOD permission can be > required if an enclave is requesting both ALLOW_WRITE and ALLOW_EXEC. There should be some explanation what ALLOW_* flag are. It is now like as it was in common knowledge. SECINFO already has protection flags to name an example and without any explanation all of this is just very confusing. This should address SECINFO and ALLOW_* relationship and differences. > Update the mmap()/mprotect() hooks to enforce the ALLOW_* protections, > a la the standard VM_MAY{READ,WRITE,EXEC} flags. > > The ALLOW_EXEC flag also has a second (important) use in that it can > be used to prevent loading an enclave from a noexec file system, on > SGX2 hardware (regardless of kernel support for SGX2), userspace could > EADD from a noexec path using read-only permissions and later mprotect() > and ENCLU[EMODPE] the page to gain execute permissions. By requiring > ALLOW_EXEC up front, SGX will be able to enforce noexec paths when > building the enclave. > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > arch/x86/include/uapi/asm/sgx.h | 9 ++++++++- > arch/x86/kernel/cpu/sgx/driver/ioctl.c | 23 +++++++++++++++++------ > arch/x86/kernel/cpu/sgx/encl.c | 2 +- > arch/x86/kernel/cpu/sgx/encl.h | 1 + > 4 files changed, 27 insertions(+), 8 deletions(-) > > diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h > index 4a12d6abbcb7..4489e92fa0dc 100644 > --- a/arch/x86/include/uapi/asm/sgx.h > +++ b/arch/x86/include/uapi/asm/sgx.h > @@ -31,6 +31,11 @@ struct sgx_enclave_create { > __u64 src; > }; > > +/* Supported flags for struct sgx_enclave_add_pages. */ > +#define SGX_ALLOW_READ VM_READ > +#define SGX_ALLOW_WRITE VM_WRITE > +#define SGX_ALLOW_EXEC VM_EXEC Why these flags are even defined if they are the same as VM_* flags? > + > /** > * struct sgx_enclave_add_pages - parameter structure for the > * %SGX_IOC_ENCLAVE_ADD_PAGES ioctl > @@ -39,6 +44,7 @@ struct sgx_enclave_create { > * @secinfo: address for the SECINFO data (common to all pages) > * @nr_pages: number of pages (must be virtually contiguous) > * @mrmask: bitmask for the measured 256 byte chunks (common to all pages) > + * @flags: flags, e.g. SGX_ALLOW_{READ,WRITE,EXEC} (common to all pages) > */ > struct sgx_enclave_add_pages { > __u64 addr; > @@ -46,7 +52,8 @@ struct sgx_enclave_add_pages { > __u64 secinfo; > __u32 nr_pages; > __u16 mrmask; > -} __attribute__((__packed__)); > + __u16 flags; > +}; > > /** > * struct sgx_enclave_init - parameter structure for the > diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c > index 6acfcbdeca9a..c30acd3fbbdd 100644 > --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c > +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c > @@ -235,7 +235,8 @@ static int sgx_validate_secs(const struct sgx_secs *secs, > } > > static struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl, > - unsigned long addr) > + unsigned long addr, > + unsigned long allowed_prot) > { > struct sgx_encl_page *encl_page; > int ret; > @@ -247,6 +248,7 @@ static struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl, > return ERR_PTR(-ENOMEM); > encl_page->desc = addr; > encl_page->encl = encl; > + encl_page->allowed_prot = allowed_prot; > ret = radix_tree_insert(&encl->page_tree, PFN_DOWN(encl_page->desc), > encl_page); > if (ret) { > @@ -530,7 +532,7 @@ static int sgx_encl_queue_page(struct sgx_encl *encl, > > static int __sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr, > void *data, struct sgx_secinfo *secinfo, > - unsigned int mrmask) > + unsigned int mrmask, unsigned long allowed_prot) > { > u64 page_type = secinfo->flags & SGX_SECINFO_PAGE_TYPE_MASK; > struct sgx_encl_page *encl_page; > @@ -556,7 +558,7 @@ static int __sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr, > goto out; > } > > - encl_page = sgx_encl_page_alloc(encl, addr); > + encl_page = sgx_encl_page_alloc(encl, addr, allowed_prot); > if (IS_ERR(encl_page)) { > ret = PTR_ERR(encl_page); > goto out; > @@ -576,12 +578,20 @@ static int __sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr, > > static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr, > unsigned long src, struct sgx_secinfo *secinfo, > - unsigned int mrmask) > + unsigned int mrmask, unsigned int flags) > { > + unsigned long prot = secinfo->flags & (VM_READ | VM_WRITE | VM_EXEC); Even if the secinfo flags have the exactly the same values you should not do this as they are kind of from different type. This is confusing to read. > + unsigned long allowed_prot = flags & (VM_READ | VM_WRITE | VM_EXEC); Why you take the trouble defining those macros and do not then use them even yourself? > struct page *data_page; > void *data; > int ret; > > + BUILD_BUG_ON(SGX_SECINFO_R != VM_READ || SGX_SECINFO_W != VM_WRITE || > + SGX_SECINFO_X != VM_EXEC); Why this check? > + > + if (prot & ~allowed_prot) > + return -EACCES; > + > data_page = alloc_page(GFP_HIGHUSER); > if (!data_page) > return -ENOMEM; > @@ -593,7 +603,8 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr, > goto out; > } > > - ret = __sgx_encl_add_page(encl, addr, data, secinfo, mrmask); > + ret = __sgx_encl_add_page(encl, addr, data, secinfo, mrmask, > + allowed_prot); > out: > kunmap(data_page); > __free_page(data_page); > @@ -645,7 +656,7 @@ static long sgx_ioc_enclave_add_pages(struct file *filep, unsigned int cmd, > > ret = sgx_encl_add_page(encl, addp->addr + i*PAGE_SIZE, > addp->src + i*PAGE_SIZE, > - &secinfo, addp->mrmask); > + &secinfo, addp->mrmask, addp->flags); > } > return ret; > } > diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c > index 955d4f430adc..e5847571a265 100644 > --- a/arch/x86/kernel/cpu/sgx/encl.c > +++ b/arch/x86/kernel/cpu/sgx/encl.c > @@ -249,7 +249,7 @@ int sgx_map_allowed(struct sgx_encl *encl, unsigned long start, > > for (addr = start; addr < end; addr += PAGE_SIZE) { > page = radix_tree_lookup(&encl->page_tree, addr >> PAGE_SHIFT); > - if (!page) > + if (!page || (prot & ~page->allowed_prot)) > return -EACCES; > } However this goes it would be good idea to have only ony patch in the patch set that fully defines this function. Impossible to review properly with this split. > > > diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h > index 6e310e3b3fff..7cca076a4987 100644 > --- a/arch/x86/kernel/cpu/sgx/encl.h > +++ b/arch/x86/kernel/cpu/sgx/encl.h > @@ -41,6 +41,7 @@ enum sgx_encl_page_desc { > > struct sgx_encl_page { > unsigned long desc; > + unsigned long allowed_prot; > struct sgx_epc_page *epc_page; > struct sgx_va_page *va_page; > struct sgx_encl *encl; > -- > 2.21.0 > This patch left me very confused. I don't get it. /Jarkko
On Tue, Jun 04, 2019 at 07:23:06PM +0300, Jarkko Sakkinen wrote: > On Fri, May 31, 2019 at 04:31:56PM -0700, Sean Christopherson wrote: > > ...to support (the equivalent) of existing Linux Security Module > > functionality. > > Long and short descriptions should be separate. Also this does not > make any sense. LSM is a framework with a set of hook to make access > decisions and there various implementations of it. > > How this replicates LSMs and why that even would be a goal? > > My guess is that you are trying to do something else. I'm just saying > that the idea to do equivalent of LSMs to another subsystems would be > insane if it was done. Heh, yeah, it's not duplicating LSM functionality. What I was trying to say is that this patch allows LSMs to implement policies that are equivalent to their existing functionality, e.g. paves the way to add security_enclave_load() as an equivalent to security_file_mprotect(). > > always be MAP_SHARED. Lastly, all real world enclaves will need read, > > write and execute permissions to EPC pages. As a result, SGX does not > > play nice with existing LSM behavior as it is impossible to apply > > policies to enclaves with any reasonable granularity, e.g. an LSM can > > deny access to EPC altogether, but can't deny potentially dangerous > > behavior such as mapping pages RW->RW or RWX. > > The mapping must be shared given that it is iomem but why enclave pages > would need RWX for all pages? The information that is missing from this > paragraph is the explanation why an LSM could not deny dangerous > behavior in PTE level. I'll add that. > > To give LSMs enough information to implement their policies without > > having to resort to ugly things, e.g. holding a reference to the vm_file > > of each enclave page, require userspace to explicitly state the allowed > > protections for each page (region), i.e. take ALLOW_{READ,WRITE,EXEC} > > in the ADD_PAGES ioctl. > > I would keep descriptions such as "ugly things" away from commit > messages as it is easy way to be not clear and explicit what you are > trying to say. > > > The ALLOW_* flags will be passed to LSMs so that they can make informed > > decisions when the enclave is being built, i.e. when the source vm_file > > is available. For example, SELinux's EXECMOD permission can be > > required if an enclave is requesting both ALLOW_WRITE and ALLOW_EXEC. > > There should be some explanation what ALLOW_* flag are. It is now like > as it was in common knowledge. SECINFO already has protection flags to > name an example and without any explanation all of this is just very > confusing. Noted. > This should address SECINFO and ALLOW_* relationship and differences. > > > Update the mmap()/mprotect() hooks to enforce the ALLOW_* protections, > > a la the standard VM_MAY{READ,WRITE,EXEC} flags. > > > > The ALLOW_EXEC flag also has a second (important) use in that it can > > be used to prevent loading an enclave from a noexec file system, on > > SGX2 hardware (regardless of kernel support for SGX2), userspace could > > EADD from a noexec path using read-only permissions and later mprotect() > > and ENCLU[EMODPE] the page to gain execute permissions. By requiring > > ALLOW_EXEC up front, SGX will be able to enforce noexec paths when > > building the enclave. > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > --- > > arch/x86/include/uapi/asm/sgx.h | 9 ++++++++- > > arch/x86/kernel/cpu/sgx/driver/ioctl.c | 23 +++++++++++++++++------ > > arch/x86/kernel/cpu/sgx/encl.c | 2 +- > > arch/x86/kernel/cpu/sgx/encl.h | 1 + > > 4 files changed, 27 insertions(+), 8 deletions(-) > > > > diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h > > index 4a12d6abbcb7..4489e92fa0dc 100644 > > --- a/arch/x86/include/uapi/asm/sgx.h > > +++ b/arch/x86/include/uapi/asm/sgx.h > > @@ -31,6 +31,11 @@ struct sgx_enclave_create { > > __u64 src; > > }; > > > > +/* Supported flags for struct sgx_enclave_add_pages. */ > > +#define SGX_ALLOW_READ VM_READ > > +#define SGX_ALLOW_WRITE VM_WRITE > > +#define SGX_ALLOW_EXEC VM_EXEC > > Why these flags are even defined if they are the same as VM_* flags? Brain fart. Flags can just take PROT_{READ,WRITE,EXEC}. > > + > > /** > > * struct sgx_enclave_add_pages - parameter structure for the > > * %SGX_IOC_ENCLAVE_ADD_PAGES ioctl > > @@ -39,6 +44,7 @@ struct sgx_enclave_create { > > * @secinfo: address for the SECINFO data (common to all pages) > > * @nr_pages: number of pages (must be virtually contiguous) > > * @mrmask: bitmask for the measured 256 byte chunks (common to all pages) > > + * @flags: flags, e.g. SGX_ALLOW_{READ,WRITE,EXEC} (common to all pages) > > */ > > struct sgx_enclave_add_pages { > > __u64 addr; > > @@ -46,7 +52,8 @@ struct sgx_enclave_add_pages { > > __u64 secinfo; > > __u32 nr_pages; > > __u16 mrmask; > > -} __attribute__((__packed__)); > > + __u16 flags; > > +}; > > ... > > @@ -576,12 +578,20 @@ static int __sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr, > > > > static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr, > > unsigned long src, struct sgx_secinfo *secinfo, > > - unsigned int mrmask) > > + unsigned int mrmask, unsigned int flags) > > { > > + unsigned long prot = secinfo->flags & (VM_READ | VM_WRITE | VM_EXEC); > > Even if the secinfo flags have the exactly the same values you should > not do this as they are kind of from different type. This is confusing > to read. I can add a dummy helper to translate flags and encapsulate the below assert. > > + unsigned long allowed_prot = flags & (VM_READ | VM_WRITE | VM_EXEC); > > Why you take the trouble defining those macros and do not then use them > even yourself? The original thought was to define them for userspace, but that's broken because VM_* aren't defined for userspace. > > struct page *data_page; > > void *data; > > int ret; > > > > + BUILD_BUG_ON(SGX_SECINFO_R != VM_READ || SGX_SECINFO_W != VM_WRITE || > > + SGX_SECINFO_X != VM_EXEC); > > Why this check? To assert that the hardware defined SECINFO flags are interchangeable with Linux's software defined flags, i.e. don't need to be translated. > > > + > > + if (prot & ~allowed_prot) > > + return -EACCES; > > + > > data_page = alloc_page(GFP_HIGHUSER); > > if (!data_page) > > return -ENOMEM; > > @@ -593,7 +603,8 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr, > > goto out; > > } > > > > - ret = __sgx_encl_add_page(encl, addr, data, secinfo, mrmask); > > + ret = __sgx_encl_add_page(encl, addr, data, secinfo, mrmask, > > + allowed_prot); > > out: > > kunmap(data_page); > > __free_page(data_page); > > @@ -645,7 +656,7 @@ static long sgx_ioc_enclave_add_pages(struct file *filep, unsigned int cmd, > > > > ret = sgx_encl_add_page(encl, addp->addr + i*PAGE_SIZE, > > addp->src + i*PAGE_SIZE, > > - &secinfo, addp->mrmask); > > + &secinfo, addp->mrmask, addp->flags); > > } > > return ret; > > } > > diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c > > index 955d4f430adc..e5847571a265 100644 > > --- a/arch/x86/kernel/cpu/sgx/encl.c > > +++ b/arch/x86/kernel/cpu/sgx/encl.c > > @@ -249,7 +249,7 @@ int sgx_map_allowed(struct sgx_encl *encl, unsigned long start, > > > > for (addr = start; addr < end; addr += PAGE_SIZE) { > > page = radix_tree_lookup(&encl->page_tree, addr >> PAGE_SHIFT); > > - if (!page) > > + if (!page || (prot & ~page->allowed_prot)) > > return -EACCES; > > } > > However this goes it would be good idea to have only ony patch in the > patch set that fully defines this function. Impossible to review > properly with this split. Sorry, I don't understand what you're suggesting. > > > > > > > diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h > > index 6e310e3b3fff..7cca076a4987 100644 > > --- a/arch/x86/kernel/cpu/sgx/encl.h > > +++ b/arch/x86/kernel/cpu/sgx/encl.h > > @@ -41,6 +41,7 @@ enum sgx_encl_page_desc { > > > > struct sgx_encl_page { > > unsigned long desc; > > + unsigned long allowed_prot; > > struct sgx_epc_page *epc_page; > > struct sgx_va_page *va_page; > > struct sgx_encl *encl; > > -- > > 2.21.0 > > > > This patch left me very confused. I don't get it. > > /Jarkko
On Fri, May 31, 2019 at 4:32 PM Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > ...to support (the equivalent) of existing Linux Security Module > functionality. > > Because SGX manually manages EPC memory, all enclave VMAs are backed by > the same vm_file, i.e. /dev/sgx/enclave, so that SGX can implement the > necessary hooks to move pages in/out of the EPC. And because EPC pages > for any given enclave are fundamentally shared between processes, i.e. > CoW semantics are not possible with EPC pages, /dev/sgx/enclave must > always be MAP_SHARED. Lastly, all real world enclaves will need read, > write and execute permissions to EPC pages. As a result, SGX does not > play nice with existing LSM behavior as it is impossible to apply > policies to enclaves with any reasonable granularity, e.g. an LSM can > deny access to EPC altogether, but can't deny potentially dangerous > behavior such as mapping pages RW->RW or RWX. > > To give LSMs enough information to implement their policies without > having to resort to ugly things, e.g. holding a reference to the vm_file > of each enclave page, require userspace to explicitly state the allowed > protections for each page (region), i.e. take ALLOW_{READ,WRITE,EXEC} > in the ADD_PAGES ioctl. > > The ALLOW_* flags will be passed to LSMs so that they can make informed > decisions when the enclave is being built, i.e. when the source vm_file > is available. For example, SELinux's EXECMOD permission can be > required if an enclave is requesting both ALLOW_WRITE and ALLOW_EXEC. > > Update the mmap()/mprotect() hooks to enforce the ALLOW_* protections, > a la the standard VM_MAY{READ,WRITE,EXEC} flags. > > The ALLOW_EXEC flag also has a second (important) use in that it can > be used to prevent loading an enclave from a noexec file system, on > SGX2 hardware (regardless of kernel support for SGX2), userspace could > EADD from a noexec path using read-only permissions and later mprotect() > and ENCLU[EMODPE] the page to gain execute permissions. By requiring > ALLOW_EXEC up front, SGX will be able to enforce noexec paths when > building the enclave. > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > arch/x86/include/uapi/asm/sgx.h | 9 ++++++++- > arch/x86/kernel/cpu/sgx/driver/ioctl.c | 23 +++++++++++++++++------ > arch/x86/kernel/cpu/sgx/encl.c | 2 +- > arch/x86/kernel/cpu/sgx/encl.h | 1 + > 4 files changed, 27 insertions(+), 8 deletions(-) > > diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h > index 4a12d6abbcb7..4489e92fa0dc 100644 > --- a/arch/x86/include/uapi/asm/sgx.h > +++ b/arch/x86/include/uapi/asm/sgx.h > @@ -31,6 +31,11 @@ struct sgx_enclave_create { > __u64 src; > }; > > +/* Supported flags for struct sgx_enclave_add_pages. */ > +#define SGX_ALLOW_READ VM_READ > +#define SGX_ALLOW_WRITE VM_WRITE > +#define SGX_ALLOW_EXEC VM_EXEC > + > /** > * struct sgx_enclave_add_pages - parameter structure for the > * %SGX_IOC_ENCLAVE_ADD_PAGES ioctl > @@ -39,6 +44,7 @@ struct sgx_enclave_create { > * @secinfo: address for the SECINFO data (common to all pages) > * @nr_pages: number of pages (must be virtually contiguous) > * @mrmask: bitmask for the measured 256 byte chunks (common to all pages) > + * @flags: flags, e.g. SGX_ALLOW_{READ,WRITE,EXEC} (common to all pages) > */ > struct sgx_enclave_add_pages { > __u64 addr; > @@ -46,7 +52,8 @@ struct sgx_enclave_add_pages { > __u64 secinfo; > __u32 nr_pages; > __u16 mrmask; > -} __attribute__((__packed__)); > + __u16 flags; Minor nit: I would use at least u32 for flags. It's not like the size saving is important. > static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr, > unsigned long src, struct sgx_secinfo *secinfo, > - unsigned int mrmask) > + unsigned int mrmask, unsigned int flags) > { > + unsigned long prot = secinfo->flags & (VM_READ | VM_WRITE | VM_EXEC); > + unsigned long allowed_prot = flags & (VM_READ | VM_WRITE | VM_EXEC); ... > + if (prot & ~allowed_prot) > + return -EACCES; This seems like a well-intentioned sanity check, but it doesn't really accomplish anything with SGX2, so I tend to think it would be better to omit it.
> From: Christopherson, Sean J > Sent: Saturday, June 01, 2019 02:32 > > /** > * struct sgx_enclave_add_pages - parameter structure for the > * %SGX_IOC_ENCLAVE_ADD_PAGES ioctl > @@ -39,6 +44,7 @@ struct sgx_enclave_create { > * @secinfo: address for the SECINFO data (common to all pages) > * @nr_pages: number of pages (must be virtually contiguous) > * @mrmask: bitmask for the measured 256 byte chunks (common to all > pages) > + * @flags: flags, e.g. SGX_ALLOW_{READ,WRITE,EXEC} (common to all > pages) > */ > struct sgx_enclave_add_pages { > __u64 addr; > @@ -46,7 +52,8 @@ struct sgx_enclave_add_pages { > __u64 secinfo; > __u32 nr_pages; > __u16 mrmask; > -} __attribute__((__packed__)); > + __u16 flags; > +}; You are adding a flags member. The secinfo structure has already a flags member in it. Why do you need both - they are both coming from user mode. What kind of scenario would require having different values. Seems confusing. --------------------------------------------------------------------- Intel Israel (74) Limited This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
On Tue, Jun 04, 2019 at 09:45:14AM -0700, Sean Christopherson wrote: > Heh, yeah, it's not duplicating LSM functionality. What I was trying to > say is that this patch allows LSMs to implement policies that are > equivalent to their existing functionality, e.g. paves the way to add > security_enclave_load() as an equivalent to security_file_mprotect(). I would suggest describing explicitly in the commit message what you want to do, which you said here e.g. "I do this because I want to add LSM hooks". This also relevant information for the LKM discussion. Lets see how the next version looks like now that you have some feedback. In the whole scope of the patch set, in order to make it more readable, I'll give following suggestions on how it is organized: 1. Leave out anything that is not strictly necessary (cosmetic fix, batch operation if possible). Better to focus one thing at a time. 2. Try to organize it so that each function is fully defined in the scope of one patch even if it would mean larger patches. 3. Do not add one call site helpers unless there is a good reason to do so. A good reason would be something like needing to extensive work in error rollback, which would make the caller a mess. /Jarkko
On Wed, Jun 05, 2019 at 04:10:44AM -0700, Ayoun, Serge wrote: > > From: Christopherson, Sean J > > Sent: Saturday, June 01, 2019 02:32 > > > > /** > > * struct sgx_enclave_add_pages - parameter structure for the > > * %SGX_IOC_ENCLAVE_ADD_PAGES ioctl > > @@ -39,6 +44,7 @@ struct sgx_enclave_create { > > * @secinfo: address for the SECINFO data (common to all pages) > > * @nr_pages: number of pages (must be virtually contiguous) > > * @mrmask: bitmask for the measured 256 byte chunks (common to all > > pages) > > + * @flags: flags, e.g. SGX_ALLOW_{READ,WRITE,EXEC} (common to all > > pages) > > */ > > struct sgx_enclave_add_pages { > > __u64 addr; > > @@ -46,7 +52,8 @@ struct sgx_enclave_add_pages { > > __u64 secinfo; > > __u32 nr_pages; > > __u16 mrmask; > > -} __attribute__((__packed__)); > > + __u16 flags; > > +}; > > You are adding a flags member. The secinfo structure has already a flags member in it. > Why do you need both - they are both coming from user mode. What kind of scenario would > require having different values. Seems confusing. The format of SECINFO.FLAGS is hardware defined, e.g. we can't add a flag to tag the page as being a zero page for optimization purposes, at least not without breaking future compatibility or doing tricky overloading. If you're specifically talking about SECINFO.FLAGS.RWX, due to SGX2 there are scenarios where userspace will initially want the page to be RW, and will later want to convert the page to RX. Making decisions based solely on the initial EPCM permissions would either create a security hole or force SGX to track "dirty" pages along with a implementing a pre-check scheme for LSMs (or restricting LSMs to tieing permissions to the host process and not the enclave).
diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h index 4a12d6abbcb7..4489e92fa0dc 100644 --- a/arch/x86/include/uapi/asm/sgx.h +++ b/arch/x86/include/uapi/asm/sgx.h @@ -31,6 +31,11 @@ struct sgx_enclave_create { __u64 src; }; +/* Supported flags for struct sgx_enclave_add_pages. */ +#define SGX_ALLOW_READ VM_READ +#define SGX_ALLOW_WRITE VM_WRITE +#define SGX_ALLOW_EXEC VM_EXEC + /** * struct sgx_enclave_add_pages - parameter structure for the * %SGX_IOC_ENCLAVE_ADD_PAGES ioctl @@ -39,6 +44,7 @@ struct sgx_enclave_create { * @secinfo: address for the SECINFO data (common to all pages) * @nr_pages: number of pages (must be virtually contiguous) * @mrmask: bitmask for the measured 256 byte chunks (common to all pages) + * @flags: flags, e.g. SGX_ALLOW_{READ,WRITE,EXEC} (common to all pages) */ struct sgx_enclave_add_pages { __u64 addr; @@ -46,7 +52,8 @@ struct sgx_enclave_add_pages { __u64 secinfo; __u32 nr_pages; __u16 mrmask; -} __attribute__((__packed__)); + __u16 flags; +}; /** * struct sgx_enclave_init - parameter structure for the diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c index 6acfcbdeca9a..c30acd3fbbdd 100644 --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c @@ -235,7 +235,8 @@ static int sgx_validate_secs(const struct sgx_secs *secs, } static struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl, - unsigned long addr) + unsigned long addr, + unsigned long allowed_prot) { struct sgx_encl_page *encl_page; int ret; @@ -247,6 +248,7 @@ static struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl, return ERR_PTR(-ENOMEM); encl_page->desc = addr; encl_page->encl = encl; + encl_page->allowed_prot = allowed_prot; ret = radix_tree_insert(&encl->page_tree, PFN_DOWN(encl_page->desc), encl_page); if (ret) { @@ -530,7 +532,7 @@ static int sgx_encl_queue_page(struct sgx_encl *encl, static int __sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr, void *data, struct sgx_secinfo *secinfo, - unsigned int mrmask) + unsigned int mrmask, unsigned long allowed_prot) { u64 page_type = secinfo->flags & SGX_SECINFO_PAGE_TYPE_MASK; struct sgx_encl_page *encl_page; @@ -556,7 +558,7 @@ static int __sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr, goto out; } - encl_page = sgx_encl_page_alloc(encl, addr); + encl_page = sgx_encl_page_alloc(encl, addr, allowed_prot); if (IS_ERR(encl_page)) { ret = PTR_ERR(encl_page); goto out; @@ -576,12 +578,20 @@ static int __sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr, static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr, unsigned long src, struct sgx_secinfo *secinfo, - unsigned int mrmask) + unsigned int mrmask, unsigned int flags) { + unsigned long prot = secinfo->flags & (VM_READ | VM_WRITE | VM_EXEC); + unsigned long allowed_prot = flags & (VM_READ | VM_WRITE | VM_EXEC); struct page *data_page; void *data; int ret; + BUILD_BUG_ON(SGX_SECINFO_R != VM_READ || SGX_SECINFO_W != VM_WRITE || + SGX_SECINFO_X != VM_EXEC); + + if (prot & ~allowed_prot) + return -EACCES; + data_page = alloc_page(GFP_HIGHUSER); if (!data_page) return -ENOMEM; @@ -593,7 +603,8 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr, goto out; } - ret = __sgx_encl_add_page(encl, addr, data, secinfo, mrmask); + ret = __sgx_encl_add_page(encl, addr, data, secinfo, mrmask, + allowed_prot); out: kunmap(data_page); __free_page(data_page); @@ -645,7 +656,7 @@ static long sgx_ioc_enclave_add_pages(struct file *filep, unsigned int cmd, ret = sgx_encl_add_page(encl, addp->addr + i*PAGE_SIZE, addp->src + i*PAGE_SIZE, - &secinfo, addp->mrmask); + &secinfo, addp->mrmask, addp->flags); } return ret; } diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c index 955d4f430adc..e5847571a265 100644 --- a/arch/x86/kernel/cpu/sgx/encl.c +++ b/arch/x86/kernel/cpu/sgx/encl.c @@ -249,7 +249,7 @@ int sgx_map_allowed(struct sgx_encl *encl, unsigned long start, for (addr = start; addr < end; addr += PAGE_SIZE) { page = radix_tree_lookup(&encl->page_tree, addr >> PAGE_SHIFT); - if (!page) + if (!page || (prot & ~page->allowed_prot)) return -EACCES; } diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h index 6e310e3b3fff..7cca076a4987 100644 --- a/arch/x86/kernel/cpu/sgx/encl.h +++ b/arch/x86/kernel/cpu/sgx/encl.h @@ -41,6 +41,7 @@ enum sgx_encl_page_desc { struct sgx_encl_page { unsigned long desc; + unsigned long allowed_prot; struct sgx_epc_page *epc_page; struct sgx_va_page *va_page; struct sgx_encl *encl;
...to support (the equivalent) of existing Linux Security Module functionality. Because SGX manually manages EPC memory, all enclave VMAs are backed by the same vm_file, i.e. /dev/sgx/enclave, so that SGX can implement the necessary hooks to move pages in/out of the EPC. And because EPC pages for any given enclave are fundamentally shared between processes, i.e. CoW semantics are not possible with EPC pages, /dev/sgx/enclave must always be MAP_SHARED. Lastly, all real world enclaves will need read, write and execute permissions to EPC pages. As a result, SGX does not play nice with existing LSM behavior as it is impossible to apply policies to enclaves with any reasonable granularity, e.g. an LSM can deny access to EPC altogether, but can't deny potentially dangerous behavior such as mapping pages RW->RW or RWX. To give LSMs enough information to implement their policies without having to resort to ugly things, e.g. holding a reference to the vm_file of each enclave page, require userspace to explicitly state the allowed protections for each page (region), i.e. take ALLOW_{READ,WRITE,EXEC} in the ADD_PAGES ioctl. The ALLOW_* flags will be passed to LSMs so that they can make informed decisions when the enclave is being built, i.e. when the source vm_file is available. For example, SELinux's EXECMOD permission can be required if an enclave is requesting both ALLOW_WRITE and ALLOW_EXEC. Update the mmap()/mprotect() hooks to enforce the ALLOW_* protections, a la the standard VM_MAY{READ,WRITE,EXEC} flags. The ALLOW_EXEC flag also has a second (important) use in that it can be used to prevent loading an enclave from a noexec file system, on SGX2 hardware (regardless of kernel support for SGX2), userspace could EADD from a noexec path using read-only permissions and later mprotect() and ENCLU[EMODPE] the page to gain execute permissions. By requiring ALLOW_EXEC up front, SGX will be able to enforce noexec paths when building the enclave. Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/include/uapi/asm/sgx.h | 9 ++++++++- arch/x86/kernel/cpu/sgx/driver/ioctl.c | 23 +++++++++++++++++------ arch/x86/kernel/cpu/sgx/encl.c | 2 +- arch/x86/kernel/cpu/sgx/encl.h | 1 + 4 files changed, 27 insertions(+), 8 deletions(-)