Date:   Sun, 5 Dec 2021 01:57:14 +0200
From:   Jarkko Sakkinen <jarkko@kernel.org>
To:     Andy Lutomirski <luto@kernel.org>
Cc:     Reinette Chatre <reinette.chatre@intel.com>,
        dave.hansen@linux.intel.com, tglx@linutronix.de, bp@alien8.de,
        mingo@redhat.com, linux-sgx@vger.kernel.org, x86@kernel.org,
        seanjc@google.com, kai.huang@intel.com, cathy.zhang@intel.com,
        cedric.xing@intel.com, haitao.huang@intel.com,
        mark.shanahan@intel.com, hpa@zytor.com,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 05/25] x86/sgx: Introduce runtime protection bits
Message-ID: <YawAWmodeNaUbzV8@iki.fi>
References: <cover.1638381245.git.reinette.chatre@intel.com>
 <2f6b04dd8949591ee6139072c72eb93da3dd07b0.1638381245.git.reinette.chatre@intel.com>
 <db9b7bc9-fdca-4dd2-2c3f-3b7354c165bb@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <db9b7bc9-fdca-4dd2-2c3f-3b7354c165bb@kernel.org>
Precedence: bulk

On Fri, Dec 03, 2021 at 11:28:04AM -0800, Andy Lutomirski wrote:
> On 12/1/21 11:23, 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.
> > 
> 
> I'm a bit confused here. ENCLU[EMODPE] allows the enclave to change the EPCM
> permission bits however it likes with no oversight from the kernel.  So we
> end up with a whole bunch of permission masks:
> 
> The PTE: controlled by complex kernel policy
> 
> The VMA: with your series, this is entirely controlled by userspace.  I
> think I'm fine with that.
> 
> vm_max_prot_bits: populated from secinfo at setup time, unless I missed
> something that changes it later.  Maybe I'm confused or missed something in
> one of the patches,
> 
> vm_run_prot_bits: populated from some combination of ioctls.  I'm entirely
> lost as to what this is for.
> 
> EPCM bits: controlled by the guest.  basically useless for any host purpose
> on SGX2 hardware (with or without kernel support -- the enclave can do
> ENCLU[EMODPE] whether we like it or not, even on old kernels)
> 
> So I guess I don't understand the purpose of this patch	or of the rules in
> the later patches, and I feel like this is getting more complicated than
> makes sense.
> 
> 
> Could we perhaps make vm_max_prot_bits dynamic or at least controllable in
> some useful way?  My initial proposal (years ago) was for vm_max_prot_bits
> to be *separately* configured at initial load time instead of being inferred
> from secinfo with the intent being that the user untrusted runtime would set
> it appropriately.  I have no problem with allowing runtime changes as long
> as the security policy makes sense and it's kept consistent with PTEs.

This is a valid question. Since EMODPE exists why not just make things for
EMODPE, and ignore EMODPR altogether?

> Also, I think we need a changelog message or, even better, actual docs in
> kernel, explaining the actual final set of rules and invariants for all
> these masks.
> 
> --Andy

/Jarkko