linux-kernel - Re: [PATCH v3] x86/sgx: Do not limit EAUG'd pages by pre-initialization policy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <684930a2-a247-7d5e-90e8-6e80db618c4c@intel.com>
Date:   Fri, 4 Mar 2022 11:09:36 -0800
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     Jarkko Sakkinen <jarkko@...nel.org>, <linux-sgx@...r.kernel.org>
CC:     Nathaniel McCallum <nathaniel@...fian.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        "Ingo Molnar" <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>,
        "open list:X86 ARCHITECTURE (32-BIT AND 64-BIT)" 
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] x86/sgx: Do not limit EAUG'd pages by
 pre-initialization policy

Hi Jarkko,

On 3/3/2022 7:39 PM, Jarkko Sakkinen wrote:
> Pre-initialization policy is meant for EADD'd pages because they are
> part of the enclave identity. It's a good practice to not let touch the
> permissions after initialization, and does provide guarantees to e.g.
> LSM's about the enclave.

I disagree. There are scenarios where it is indeed good practice to
modify the permissions after initialization.  For example, pages that
may be used for relocatable code can start with RWX permissions but once
the pages have been populated with the code they should be able to restrict
permissions to RX only. It is not good practice to require RWX permission
over their entire lifetime. Ideally pages should only have the lowest
permissions possible. 

Supporting the modification of permissions after initialization enables
the security conscious enclave owner to support the security
principle of least privilege.

> 
> For EAUG'd pages it should be sufficient to let mmap(), mprotect() and
> SGX opcodes to control the permissions. Thus effectively disable
> pre-initialization policy by setting vm_max_prot_bits to RWX.

Setting vm_max_prot_bits of EAUG pages to RWX results in dynamically
added (RW) pages to allow RWX PTEs!

Please correct me if I am wrong but I do not see how mmap() and
mprotect() will control permissions here. Will these flows be able to
prevent a RWX mapping of dynamically added RW EPCM pages? From what
I understand SGX controls the permissions here with sgx_encl_may_map()
called during mmap() and mprotect() and it will allow such mappings
after this patch. RWX mapping (VMA) with RWX PTEs of RW EPCM memory
will be allowed, correct?

That means, an enclave using EAUG to expand its heap will have
enclave pages with RW EPCM but yet could have RWX PTEs pointing to
them after the user creates a RWX mmap().

>From what I understand this circumvents the kernel's security mechanisms.

This also breaks one of the original rules of SGX as per:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/x86/sgx.rst#n74

"EPCM permissions are separate from the normal page tables.  This prevents the
kernel from, for instance, allowing writes to data which an enclave wishes to
remain read-only."

In these changes the kernel allows all dynamically added pages to be
executable - even when the enclave wishes them not to be.

> Then, remove vm_run_prot_bits. For EADD'd pages the roof is where
> it was during construction, for EAUG'd we don't simply care. This
> hard to keep in-sync variable adds only a layer of complexity and
> nothing else.
> 
> Without vm_run_prot_bits existing, SGX_IOC_ENCLAVE_RELAX_PERMISSIONS
> does absolutely nothing. Therefore, it can be safely removed.

Removing vm_run_prot_bits cripples SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS.
With this removal SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS will support the
modification of EPCM permissions but PTE and VMA permissions will continue
to allow the maximum access possible for the page, whether the enclave page
supports the permission or not.

I find it risky to circumvent the kernel's security mechanisms and
I am not comfortable signing off on this. Or am I just not understanding it
correctly?

Reinette