Re: [PATCH v40 10/24] mm: Add 'mprotect' hook to struct vm_operations_struct

[Dr. Greg]

On Sat, Nov 07, 2020 at 11:16:25AM -0800, Dave Hansen wrote:

Good afternoon, I hope the week is going well for everyone.

> On 11/7/20 7:09 AM, Dr. Greg wrote:
> > In all of these discussions there hasn't been a refutation of my point
> > that the only reason this hook is needed is to stop the potential for
> > anonymous code execution on SGX2 capable hardware.  So we will assume,
> > that while unspoken, this is the rationale for the hook.

> Unspoken?  See from the cover letter:

The complexity of the issues involved and the almost complete lack of
understanding of the technology in the Linux user community would
suggest that everyone would benefit from a more detailed explanation
of the issues at hand.

> > 3. Enclave page permissions are dynamic (just like normal permissions) and
> >    can be adjusted at runtime with mprotect().

> I explicitly chose not to name the instructions, nor the exact
> version of the SGX ISA that introduces them.  They're supported in
> the series, and that's all that matters.

When it comes to security issues and risk assessment it always seems
that more information is better, but of course opinions vary, wildly
it would seem in the case of this technology.

I've been told countless times in my career: "What happens if you get
hit by a bus".  I've tried to address those issues by generating
copious amounts of documentation on everything I do.  Having the
relevant issues with respect to the security considerations and
implications of this technology clearly documented would seem to
address the 'hit by a bus' issue for other developers that may need to
look at and understand the code down the road.

> If you want to advocate for something different to be done, patches
> are accepted.

I'm including a patch below that addresses the mprotect vulnerability
as simplistically as I believe it can be addressed and provides some
background information on the issues at hand.  I will let people more
wise then I am decide whether or not the world at large is worse off
for having the information available.

I tested this with our runtime, which is of a significantly different
design then Intel's.  After testing multiple adversarial approaches to
changing page permissions, I'm left struggling to understand what the
page walking code accomplishes, even in the case of mmap.

The ultimate decision with respect to allowed page permissions is
cryptographically encoded in the enclave measurement.  The enclave
won't initialize if changes are made to the desired EPCM permissions.
If an attempt is made to use mmap to alter those permissions at the OS
level they will be inhibited by the EPCM permission verifications.

If one reads the EDMM papers by the Intel engineering team that
designed the technology, they were very concerned about an enclave
having to agree to any virtual memory changes, hence the need for
ENCLU[EACCEPT] and ENCLU[EACCEPTCOPY].  In that respect the behavior
of ENCLU[EMODPE] is somewhat interesting in that it gives untrusted
userspace the ability to thwart the intentions of enclave code.

They may not, however, have had any other choice given that SGX was
designed as a virtual memory play in order to make it an 'easy'
add-on.

Given all of this, it seems to be the case that the only thing needed
to block anonymous code execution is to block mprotect on an
initialized enclave, which the attached patch does.  If and when the
driver supports EDMM there is, I believe, a very open question as to
what type of introspection capability the kernel should have.

More on that in a subsequent post/patch.

Have a good evening.

Dr. Greg

Cut here. -----------------------------------------------------------------