RE: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks

["Xing, Cedric" <cedric.xing@intel.com>]

> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
> owner@vger.kernel.org] On Behalf Of Casey Schaufler
> Sent: Monday, July 01, 2019 12:54 PM
> > If you understand the purpose,
> 
> The purpose is to support the SGX hardware, is it not?
> If you don't have SGX hardware (e.g. MIPS, ARM, s390) you don't need
> this code.

No, it is NOT to support SGX - i.e. SGX doesn't require this piece of code to work.

And as Dr. Greg pointed out, it can be used for other TEEs than SGX. It doesn't contain SGX h/w specifics. It is compiled out because there's no module calling it on other architectures today. But it doesn't conflict with any h/w and may be useful (for other TEEs) on other architectures in future.

> 
> > then why are you objecting the lsm_ prefix as they are APIs to be used
> by all LSM modules?
> 
> We name interfaces based on what they provide, not who consumes them.
> As your code provides enclave services, that is how they should be named.
> 
> >  Or what should be the prefix in your mind?
> 
> I'm pretty sure that I've consistently suggested "enclave".
> 
> > Or what kind of APIs do you think can qualify the lsm_ prefix?
> 
> Code that implements the LSM infrastructure. There is one LSM blob
> allocation interface, lsm_inode_alloc(), that is used in early set-up
> that is exported. As I've mentioned more than once, enclave/page
> management is not an LSM infrastructure function, it's a memory
> management function.

It doesn't manage anything. The reason it appears in the infrastructure is because the decision of inserting an EMA depends on the decisions from *all* active LSMs. That is NOT new either, as you can see it in security_file_permission() and security_vm_enough_memory_mm(), both do something after all LSM modules make their decisions.

Would you please explain why you don't see those as problems but calling EMA functions in security_enclave_load() is a problem?

> 
> > And I'd like to clarify that it doesn't break anything, but is just a
> bit different, in that security_enclave_load() and security_file_free()
> call into those APIs.
> 
> There should be nothing in security_enclave_load() except calls to the
> enclave_load() hooks (e.g. selinux_enclave_load()). There should be
> nothing in security_file_free() except file blob management calls to the
> file_free() hooks (e.g. apparmor_file_free()).

As above, there are examples in security/security.c where the hook does more than just calling registered hooks from LSMs.

> 
> > But there's a need for them because otherwise code/data would have to
> > be duplicated among LSMs
> 
> There's plenty of code duplication among the LSMs, because a lot of what
> they do is the same thing. Someday there may be an effort to address
> some of that, but I don't think it's on anybody's radar.
> As for data duplication, there's a reason we use lots of pointers.

As stated above, security_enclave_load() needs to do something extra after all LSMs make their decisions. How can pointers help here?
 
> 
> > and the logic would be harder to comprehend.
> 
> Keeping the layering clean is critical to comprehension.
> There's a lot of VFS code that could have been implemented within the
> LSM infrastructure, but I don't think that anyone would argue that it
> should have been.
> 
> > So that's a trade-off.
> 
> I remain completely unconvinced that your proposal represents a good way
> to implement you scheme.
> 
> > Then what's the practical drawback of doing that?
> 
> Name space pollution.

Alright, I can fix the names.

> Layering violation.

Not sure what you are referring to. 

If you are referring to buffers allocated in one layer and freed in elsewhere, you have got the code wrong. Buffers allocated in security_enclave_load() is freed in security_file_free(). Whatever else allocated in LSMs are not seen or taken care of by the infrastructure. The purpose of allocating EMAs in enclave_load() is trying to minimize overhead for non-enclave files, otherwise it could be done in file_alloc() to be more "paired" with file_free(). But I don't see it necessary. 

> Architecture specific implementation detail in a general infrastructure.

Stated earlier, it doesn't contain any h/w specifics but just a TEE abstraction. It could be left on all the time or controlled by a different config macro. It is contingent to CONFIG_INTEL_SGX just for convenience, as SGX is the first (and only so far) TEE that needs attention from LSM, but there could be more in future. 

> 
> > If no, why would we want to pay for the cost for not doing that?
> 
> Modularity and maintainability come directly to mind.

Putting it elsewhere will incur more maintenance cost.

> 
> >> ... and the notion that you allocate data for one blob that gets
> >> freed relative to another breaks the data management model.
> > What do you mean here?
> 
> You're freeing the EMA data from security_file_free().
> If selinux wants to free EMA data it has allocated in
> selinux_enclave_load() in selinux_file_free() that's fine, but the LSM
> infrastructure has no need to know about it.
> EMA needs to manage its own data, just like VFS does.
> The LSM infrastructure provides blob management so that the security
> modules can extend data if they want to.

You've got the code wrong. selinux_enclave_load() doesn't allocate any memory.  selinux_file_mprotect() may, due to EMA split. But that's transparent to all LSMs.

The LSM infrastructure doesn't know anything about what LSM modules do, nor does it manage any buffers allocated by any LSM modules.

EMA is currently managing its own data. What's needed is the trigger - to let EMA know when to update its states. The trigger could be placed in LSM infrastructure or inside individual LSMs. The reason to put it in the infrastructure, is that it depends on the decision of *all* LSMs whether to insert a new EMA. That's similar to vm_enough_memory() where the final __vm_enough_memory() call is made by the infrastructure but not individual LSMs.

> 
> > EMA blobs are allocated/freed *not* relative to any other blobs.
> 
> In the code you proposed they are freed in security_file_free().
> That is for file blob management.

Yes. EMA contributes to the file blob. But it only frees memory allocated by the infrastructure itself, not anything from any LSM modules.