On 6/2/19 3:29 AM, Xing, Cedric wrote:
Hi Sean,
From: Christopherson, Sean J
Sent: Friday, May 31, 2019 4:32 PM
This series is the result of a rather absurd amount of discussion over how to
get SGX to play
nice with LSM policies, without having to resort to evil shenanigans or put
undue burden on
userspace. The discussion definitely wandered into completely insane territory
at times, but
I think/hope we ended up with something reasonable.
The basic gist of the approach is to require userspace to declare what
protections are
maximally allowed for any given page, e.g. add a flags field for loading
enclave pages that
takes ALLOW_{READ,WRITE,EXEC}. LSMs can then adjust the allowed protections,
e.g. clear
ALLOW_EXEC to prevent ever mapping the page with PROT_EXEC. SGX enforces the
allowed perms
via a new mprotect() vm_ops hook, e.g. like regular mprotect() uses
MAY_{READ,WRITE,EXEC}.
ALLOW_EXEC is used to deny hings like loading an enclave from a noexec file
system or from a
file without EXECUTE permissions, e.g. without the ALLOW_EXEC concept, on SGX2
hardware
(regardless of kernel support) userspace could EADD from a noexec file using
read-only
permissions, and later use mprotect() and ENCLU[EMODPE] to gain execute
permissions.
ALLOW_WRITE is used in conjuction with ALLOW_EXEC to enforce SELinux's EXECMOD
(or EXECMEM).
This is very much an RFC series. It's only compile tested, likely has obvious
bugs, the
SELinux patch could be completely harebrained, etc...
My goal at this point is to get feedback at a macro level, e.g. is the core
concept
viable/acceptable, are there objection to hooking mprotect(), etc...
Andy and Cedric, hopefully this aligns with your general expectations based on
our last
discussion.
I couldn't understand the real intentions of ALLOW_* flags until I saw them in
code. I have to say C is more expressive than English in that regard :)
Generally I agree with your direction but think ALLOW_* flags are completely
internal to LSM because they can be both produced and consumed inside an LSM
module. So spilling them into SGX driver and also user mode code makes the
solution ugly and in some cases impractical because not every enclave host
process has a priori knowledge on whether or not an enclave page would be
EMODPE'd at runtime.
Theoretically speaking, what you really need is a per page flag (let's name it
WRITTEN?) indicating whether a page has ever been written to (or more
precisely, granted PROT_WRITE), which will be used to decide whether to grant
PROT_EXEC when requested in future. Given the fact that all mprotect() goes
through LSM and mmap() is limited to PROT_NONE, it's easy for LSM to capture
that flag by itself instead of asking user mode code to provide it.
That said, here is the summary of what I think is a better approach.
* In hook security_file_alloc(), if @file is an enclave, allocate some data
structure to store for every page, the WRITTEN flag as described above. WRITTEN
is cleared initially for all pages.
Open: Given a file of type struct file *, how to tell if it is an enclave
(i.e. /dev/sgx/enclave)?
* In hook security_mmap_file(), if @file is an enclave, make sure @prot can
only be PROT_NONE. This is to force all protection changes to go through
security_file_mprotect().
* In the newly introduced hook security_enclave_load(), set WRITTEN for pages
that are requested PROT_WRITE.
* In hook security_file_mprotect(), if @vma->vm_file is an enclave, look up and use WRITTEN flags for all pages within @vma, along with other global flags (e.g. PROCESS__EXECMEM/FILE__EXECMOD in the case of SELinux) to decide on
allowing/rejecting @prot.
At this point we have no knowledge of the source vma/file, right? So
what do we check FILE__EXECUTE and/or FILE__EXECMOD against?
vma->vm_file at this point is /dev/sgx/enclave, right?
* In hook security_file_free(), if @file is an enclave, free storage allocated
for WRITTEN flags.
I'll try to make more detailed comments in my replies to individual patches
sometime tomorrow.
Lastly, I added a patch to allow userspace to add multiple pages in a single
ioctl(). It's
obviously not directly related to the security stuff, but the idea tangentially
came up during
earlier discussions and it's something I think the UAPI should provide (it's a
tiny change).
Since I was modifying the UAPI anyways, I threw it in.
Sean Christopherson (9):
x86/sgx: Remove unused local variable in sgx_encl_release()
x86/sgx: Do not naturally align MAP_FIXED address
x86/sgx: Allow userspace to add multiple pages in single ioctl()
mm: Introduce vm_ops->mprotect()
x86/sgx: Restrict mapping without an enclave page to PROT_NONE
x86/sgx: Require userspace to provide allowed prots to ADD_PAGES
x86/sgx: Enforce noexec filesystem restriction for enclaves
LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX
security/selinux: Add enclave_load() implementation
arch/x86/include/uapi/asm/sgx.h | 30 ++++--
arch/x86/kernel/cpu/sgx/driver/ioctl.c | 143 +++++++++++++++++--------
arch/x86/kernel/cpu/sgx/driver/main.c | 13 ++-
arch/x86/kernel/cpu/sgx/encl.c | 31 +++++-
arch/x86/kernel/cpu/sgx/encl.h | 4 +
include/linux/lsm_hooks.h | 16 +++
include/linux/mm.h | 2 +
include/linux/security.h | 2 +
mm/mprotect.c | 15 ++-
security/security.c | 8 ++
security/selinux/hooks.c | 85 +++++++++++++++
11 files changed, 290 insertions(+), 59 deletions(-)
--
2.21.0
-Cedric
