Re: [PATCH v38 10/24] mm: Add vm_ops->mprotect()

[Sean Christopherson]

On Tue, Oct 20, 2020 at 05:01:18AM -0500, Dr. Greg wrote:
> On Mon, Oct 19, 2020 at 02:31:05PM -0700, Sean Christopherson wrote:
> 
> Good morning, I hope the day is starting well for everyone.
> 
> > On Sun, Oct 18, 2020 at 03:49:20AM -0500, Dr. Greg wrote:
> > > Is this even a relevant control if we cede the notion of dynamically
> > > loadable enclave code, which is the objective of SGX2 hardware, which
> > > will in all likelihood be the only relevant hardware implementation in
> > > the future?
> 
> > Yes, it's still relevant.  Giving the thumbs up to dynamically
> > loadable code is not a purely binary decision, e.g. a user/admin can
> > allow RW->RX transitions but still disallow full RWX permissions.
> 
> With respect to the security issue at hand, the only relevant issue
> would seem to be if a page had write permissions at one time in its
> trajectory to having execute permisions, isn't this correct?

No.  RW->RX has different properties than RWX.  E.g. an enclave that
dynamically loads code is not the same thing as an enclave that allows
simultaneously writing and executing a page.

> The next paragraph of my reply wasn't included in your reply, but I
> did state that the mprotect hook would be relevant if its purpose was
> to disallow this permission trajectory and in the process disable
> enclave dynamic code loading and execution.
> 
> So to assist everyone in understanding this issue and the security
> implications involved, is the ultimate purpose of the mprotect hook to
> disable dynamic code loading?

No, it's to provide line of sight to enforcing granular LSM checks on enclave
pages.  Jumping back to the RWX thing, as a defense in depth measure, a policy
owner could set an SELinux policy to never allow RWX, even for enclaves that
dynamically load code.

Whether or not having per-page granluarity on enclave permission checks is
valuable/interesting is debatable, e.g. it's why LSM integration is notably
absent from the this series.  But, adding the ->mprotect() hook is relatively
cheap and can always be removed if it's deemed unnecessary in the long run.
The reverse is not true; omitting ->mprotect() commits the kernel to an ABI
that is either ugly and potentially painful (require all enclaves to declare
full RWX permissions), or flat out prevents adding granular LSM support in
the future (do nothing).