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@xxxxxxxxx>
> > ---
> > 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
