| Message ID | 20190617222438.2080-3-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | security: x86/sgx: SGX vs. LSM, round 3 | expand |
On Mon, 2019-06-17 at 15:24 -0700, Sean Christopherson wrote: > { > - if (len < 2 * PAGE_SIZE || len & (len - 1) || flags & MAP_PRIVATE) > + if (flags & MAP_PRIVATE) > + return -EINVAL; > + > + if (flags & MAP_FIXED) > + return addr; > + > + if (len < 2 * PAGE_SIZE || len & (len - 1)) > return -EINVAL; > > addr = current->mm->get_unmapped_area(file, addr, 2 * len, pgoff, Just sanity checking that for MAP_FIXED case the mm checks that the area is unmapped before calling this? I don't think we need to check any alignment constraints here anymore. The summarize end result would be: static unsigned long sgx_get_unmapped_area(struct file *file, unsigned long addr, unsigned long len, unsigned long pgoff, unsigned long flags) { if (flags & MAP_PRIVATE) return -EINVAL; if (flags & MAP_FIXED) return addr; return current->mm->get_unmapped_area(file, addr, 2 * len, pgoff, flags); } /Jarkko
On Wed, Jun 19, 2019 at 04:24:05PM +0300, Jarkko Sakkinen wrote: > On Mon, 2019-06-17 at 15:24 -0700, Sean Christopherson wrote: > > { > > - if (len < 2 * PAGE_SIZE || len & (len - 1) || flags & MAP_PRIVATE) > > + if (flags & MAP_PRIVATE) > > + return -EINVAL; > > + > > + if (flags & MAP_FIXED) > > + return addr; > > + > > + if (len < 2 * PAGE_SIZE || len & (len - 1)) > > return -EINVAL; > > > > addr = current->mm->get_unmapped_area(file, addr, 2 * len, pgoff, > > Just sanity checking that for MAP_FIXED case the mm checks that the area is > unmapped before calling this? No, straight MAP_FIXED unmaps any existing mappings. The NOREPLACE variant fails with -EEXIST if there are existing mappings. The MAP_FIXED behavior is actually useful, bordering on mandatory, for the new flow. It allows the loader to keep its initial mmap(PROT_NONE) of ELRANGE while (re)mapping the individual enclave sections, e.g. to prevent a different aspect of the process from mapping the require ELRANGE. > > I don't think we need to check any alignment constraints here anymore. > > The summarize end result would be: > > static unsigned long sgx_get_unmapped_area(struct file *file, > unsigned long addr, > unsigned long len, > unsigned long pgoff, > unsigned long flags) > { > if (flags & MAP_PRIVATE) > return -EINVAL; > > if (flags & MAP_FIXED) > return addr; > > return current->mm->get_unmapped_area(file, addr, 2 * len, pgoff, > flags); > } > > /Jarkko >
On Wed, Jun 19, 2019 at 07:08:53AM -0700, Sean Christopherson wrote: > On Wed, Jun 19, 2019 at 04:24:05PM +0300, Jarkko Sakkinen wrote: > > On Mon, 2019-06-17 at 15:24 -0700, Sean Christopherson wrote: > > > { > > > - if (len < 2 * PAGE_SIZE || len & (len - 1) || flags & MAP_PRIVATE) > > > + if (flags & MAP_PRIVATE) > > > + return -EINVAL; > > > + > > > + if (flags & MAP_FIXED) > > > + return addr; > > > + > > > + if (len < 2 * PAGE_SIZE || len & (len - 1)) > > > return -EINVAL; > > > > > > addr = current->mm->get_unmapped_area(file, addr, 2 * len, pgoff, > > > > Just sanity checking that for MAP_FIXED case the mm checks that the area is > > unmapped before calling this? > > No, straight MAP_FIXED unmaps any existing mappings. The NOREPLACE variant > fails with -EEXIST if there are existing mappings. Ah so it was [1]! > The MAP_FIXED behavior is actually useful, bordering on mandatory, for the > new flow. It allows the loader to keep its initial mmap(PROT_NONE) of > ELRANGE while (re)mapping the individual enclave sections, e.g. to prevent > a different aspect of the process from mapping the require ELRANGE. Yeah, totally agree. [1] http://man7.org/linux/man-pages/man2/mmap.2.html /Jarkko
diff --git a/arch/x86/kernel/cpu/sgx/driver/main.c b/arch/x86/kernel/cpu/sgx/driver/main.c index 03eca4bccee1..d7de4d9aea87 100644 --- a/arch/x86/kernel/cpu/sgx/driver/main.c +++ b/arch/x86/kernel/cpu/sgx/driver/main.c @@ -105,7 +105,13 @@ static unsigned long sgx_get_unmapped_area(struct file *file, unsigned long pgoff, unsigned long flags) { - if (len < 2 * PAGE_SIZE || len & (len - 1) || flags & MAP_PRIVATE) + if (flags & MAP_PRIVATE) + return -EINVAL; + + if (flags & MAP_FIXED) + return addr; + + if (len < 2 * PAGE_SIZE || len & (len - 1)) return -EINVAL; addr = current->mm->get_unmapped_area(file, addr, 2 * len, pgoff,
SGX enclaves have an associated Enclave Linear Range (ELRANGE) that is tracked and enforced by the CPU using a base+mask approach, similar to how hardware range registers such as the variable MTRRs. As a result, the ELRANGE must be naturally sized and aligned. To reduce boilerplate code that would be needed in every userspace enclave loader, the SGX driver naturally aligns the mmap() address and also requires the range to be naturally sized. Unfortunately, SGX fails to grant a waiver to the MAP_FIXED case, e.g. incorrectly rejects mmap() if userspace is attempting to map a small slice of an existing enclave. Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/kernel/cpu/sgx/driver/main.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)