On Mon, Jun 17, 2019 at 9:49 AM Sean Christopherson <sean.j.christopherson@xxxxxxxxx> wrote: > > On Sun, Jun 16, 2019 at 03:14:51PM -0700, Andy Lutomirski wrote: > > On Fri, Jun 14, 2019 at 8:38 AM Sean Christopherson > > <sean.j.christopherson@xxxxxxxxx> wrote: > > > > Andy and/or Cedric, can you please weigh in with a concrete (and practical) > > > > use case that will break if we go with #1? The auditing issues for #2/#3 > > > > are complex to say the least... > > > > The most significant issue I see is the following. Consider two > > cases. First, an SGX2 enclave that dynamically allocates memory but > > doesn't execute code from dynamic memory. Second, an SGX2 enclave > > that *does* execute code from dynamic memory. In #1, the untrusted > > stack needs to decide whether to ALLOW_EXEC when the memory is > > allocated, which means that it either needs to assume the worst or it > > needs to know at allocation time whether the enclave ever intends to > > change the permission to X. > > I'm just not convinced that folks running enclaves that can't communicate > their basic functionality will care one whit about SELinux restrictions, > i.e. will happily give EXECMOD even if it's not strictly necessary. At least when permissions are learned, if there's no ALLOW_EXEC for EAUG, then EXECMOD won't get learned if there's no eventual attempt to execute the memory. > > > I suppose there's a middle ground. The driver could use model #1 for > > driver-filled pages and model #2 for dynamic pages. I haven't tried > > to fully work it out, but I think there would be the ALLOW_READ / > > ALLOW_WRITE / ALLOW_EXEC flag for EADD-ed pages but, for EAUG-ed > > pages, there would be a different policy. This might be as simple as > > internally having four flags instead of three: > > > > ALLOW_READ, ALLOW_WRITE, ALLOW_EXEC: as before > > > > ALLOW_EXEC_COND: set implicitly by the driver for EAUG. > > > > As in #1, if you try to mmap or protect a page with neither ALLOW_EXEC > > variant, it fails (-EACCES, perhaps). But, if you try to mmap or > > mprotect an ALLOW_EXEC_COND page with PROT_EXEC, you ask LSM for > > permission. There is no fancy DIRTY tracking here, since it's > > reasonable to just act as though *every* ALLOW_EXEC_COND page is > > dirty. There is no real auditing issue here, since LSM can just log > > what permission is missing. > > > > Does this seem sensible? It might give us the best of #1 and #2. > > It would work and is easy to implement *if* SELinux ties permissions to > the process, as the SIGSTRUCT vma/file won't be available at > EAUG+mprotect(). I already have a set of patches to that effect, I'll > send 'em out in a bit. I'm okay with that. > > FWIW, we still need to differentiate W->X from WX on SGX1, i.e. declaring > ALLOW_WRITE + ALLOW_EXEC shouldn't imply WX. This is also addressed in > the forthcoming updated RFC. Sounds good.
