Re: [PATCH 05/25] x86/sgx: Introduce runtime protection bits

[Reinette Chatre]

Hi Jarkko,

On 12/4/2021 2:50 PM, Jarkko Sakkinen wrote:
> What about:
>
> "x86/sgx: Add encl_page->vm_run_prot_bits for dynamic permission changes"

Sure.

> On Wed, Dec 01, 2021 at 11:23:03AM -0800, Reinette Chatre wrote:
> > Enclave creators declare their paging permission intent at the time
> > the pages are added to the enclave. These paging permissions are
> > vetted when pages are added to the enclave and stashed off
> > (in sgx_encl_page->vm_max_prot_bits) for later comparison with
> > enclave PTEs.
> >
> > Current permission support assume that enclave page permissions
> > remain static for the lifetime of the enclave. This is about to change
> > with the addition of support for SGX2 where the permissions of enclave
> > pages belonging to an initialized enclave may be changed during the
> > enclave's lifetime.
> >
> > Introduce runtime protection bits in preparation for support of
>
> By writing "Introduce runtime protection bits", instead of simply "Add
> encl_page->vm_run_prot_bits", the only thing you are adding is obfuscation.
>
> Try to refer to the "exact thing", instead of English rephrasing
> whenever possible.
>
> > enclave page permission changes. These bits reflect the active
> > permissions of an enclave page and are not to exceed the maximum
> > protection bits that passed scrutiny during enclave creation.
> >
> > Associate runtime protection bits with each enclave page. Initialize
> > the runtime protection bits to the vetted maximum protection bits
> > on page creation. Use the runtime protection bits for any access
> > checks.
>
> I guess the first sentence in this paragraph is completely redundant
> as the first sentence of the previous paragraph tells the exact
> same story.

The previous paragraph introduces what these bits are and what they mean 
and the second describes how they are used. I can merge the paragraphs.

> > struct sgx_encl_page hosting this information is maintained for each
> > enclave page so the space consumed by the struct is important.
> > The existing vm_max_prot_bits is already unsigned long while only using
> > three bits. Transition to a bitfield for the two members containing
> > protection bits.
> >
> > Signed-off-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>
>
> So this commit message left the most important thing unanswered,
> or I missed it (which happens quite often): why two fields instead
> of one? Why vm_max_port_bits needs to stay constant?
>
> It's something that should be clearly documented.

vm_max_prot_bits is the vetted EPCM permissions an enclave is allowed to 
have (for EADDed pages it is the value from secinfo). Permissions can be 
changed using SGX2 but they should never exceed vm_max_prot_bits.
vm_run_prot_bits reflects the current (from OS perspective) active EPCM 
permissions and replaces the current usages of vm_max_prot_bits in 
runtime (VMA and PTE) permission checks.

Consider this example how vm_max_prot_bits and vm_run_prot_bits are used:

(1) Add enclave page with secinfo of RW to uninitialized enclave
    vm_max_prot_bits = RW
    vm_run_prot_bits = RW

(2) User space runs SGX_IOC_PAGE_MODP (renamed
    to SGX_IOC_ENCLAVE_MOD_PROTECTIONS) to change the permissions to
    read-only. This is allowed because vm_max_prot_bits = RW. Now:
    vm_max_prot_bits = RW
    vm_run_prot_bits = R

    At this point only new read-only VMAs would be allowed to access
    this page and PTEs would not allow write access ... this is guided
    by vm_run_prot_bits.

(3) User space runs SGX_IOC_PAGE_MODP (renamed
    to SGX_IOC_ENCLAVE_MOD_PROTECTIONS) to change the permissions to RX.
    This will be denied because vm_max_prot_bits = RW.

(3) User space runs SGX_IOC_PAGE_MODP (renamed
    to SGX_IOC_ENCLAVE_MOD_PROTECTIONS) to change the permissions to RW.
    This will be allowed because vm_max_prot_bits = RW.

If this is helpful I can add this example to the changelog.

Reinette