Message-ID: <c091ee8b-04a8-9165-00c1-835bb520e240@intel.com>
Date:   Mon, 13 Dec 2021 14:34:05 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.4.0
Subject: Re: [PATCH 05/25] x86/sgx: Introduce runtime protection bits
Content-Language: en-US
From:   Reinette Chatre <reinette.chatre@intel.com>
To:     Andy Lutomirski <luto@kernel.org>, <dave.hansen@linux.intel.com>,
        <jarkko@kernel.org>, <tglx@linutronix.de>, <bp@alien8.de>,
        <mingo@redhat.com>, <linux-sgx@vger.kernel.org>, <x86@kernel.org>
CC:     <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>
References: <cover.1638381245.git.reinette.chatre@intel.com>
 <2f6b04dd8949591ee6139072c72eb93da3dd07b0.1638381245.git.reinette.chatre@intel.com>
 <db9b7bc9-fdca-4dd2-2c3f-3b7354c165bb@kernel.org>
 <c0aab85a-f6a1-ed5c-5540-b03778ffe24a@intel.com>
 <4f50b3b3-2cd5-30a4-5715-3350ef2174d9@kernel.org>
 <ea7708f6-a4c7-9f00-3c69-39cae0c7ac86@intel.com>
 <67993dd0-827e-5f08-d853-4fb0273adb5f@kernel.org>
 <855ab0ac-1e9b-dd3b-63ec-d15d0f0fda77@intel.com>
In-Reply-To: <855ab0ac-1e9b-dd3b-63ec-d15d0f0fda77@intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UzdDdlcrN0tqRWJsSFYxV2xIV3pZOURjalRVcUZ3R2pQTGwyTTJUOUJHckZq?=
 =?utf-8?B?cDhXYk9zQjZsRE9TanNjUUplNzBpM2FuWFdqaEJYekxDejJTSlFuZ2Y5UHlw?=
 =?utf-8?B?NGVycENqbEh1bHd2VllrVmFUcHhDZHNNSUcxZFlQcTNyMTBOU1VCRklobVVq?=
 =?utf-8?B?dU9mcmgrN2ttMDBaZTBRMEpqVktUY2owSE1JeVJZM2dmUjhNbVp3dWxpOHpu?=
 =?utf-8?B?QjhMVkVQSHVLRHp2aG84d0RTeFJIdzlEQWxPWFAvWEw0ZnNwR3VnRTBCYU5M?=
 =?utf-8?B?UTBCaitCa0xSYkFIUTZWZktReWVsdzRaWWVMQ0xzNzQvUUJ6YTBIY3cwelpz?=
 =?utf-8?B?NmhTdUJ5dCtyWW1GZDhyV25YYzB2SVRmZEdoZWNjVVdBN3M0M25GMVVXV2Ew?=
 =?utf-8?B?dk12ajRRM2lGY01XNU1VNE5WT00vZjNKTTQ2cEpvb2F4T3pCejZmQjFqK25U?=
 =?utf-8?B?cDRNVy9SQytLM1BXK2VOU2Yyem1rbHMvQ2FWc1RsS0x4ZEs2ZzNqWkJLRkEv?=
 =?utf-8?B?alJTZDU4SmZJOURzVWUzdTRidDQza1A5TTd4K0V0L2lXYlQrZGtiSndQRGF6?=
 =?utf-8?B?NnBKMTltRmFCMVQ4SnV3bXFQaFFDSE1HUHNQYUtlZ0psK1dpRS8yU0JyNHpy?=
 =?utf-8?B?TE5nQ1orc0U5TGdFdUozSkxDSGpndHllSXFLQTQwVXFwR0J5SVl1RnZHaUFw?=
 =?utf-8?B?NlJ5U0kzSDNLeWxtYVorcU1LcXZoS2lScTBFam81QzY2SG8rM1hoL2FXNEg5?=
 =?utf-8?B?ZXNKTFUwZGZwOElRRWpXVlVHZHB4c044SHZ0UnVEanhjNFRTQXNwSDE2OXVt?=
 =?utf-8?B?MkM2WHNjTmhITXRkTWdzbGVpa0ljWHJ6a3ZUN29McGR4ZE9TdmQ4K0ltRm5L?=
 =?utf-8?B?dkxCdnNxVlc2ZXJuV25RL1BQS21RU0NJYmUrWVJNTGhibjdraDV3OFEwNmJY?=
 =?utf-8?B?SjNUS0NvcnQ1OWo1YmFRYUp3d1ZvYXpiQS9xNlRSeE9vaEFJaHE2c3dwZUE4?=
 =?utf-8?B?OWdHVXU3TmlRUjIxSVhDRW9zdkkzVEM4bUpCOFI0QWNIbkJhYkMvK3RibWU1?=
 =?utf-8?B?dTR3REttd0dhVGdjOHlUd2h4Y0UzSnZCWE9FeTQ3NVJta1l6K3RmYU94TUxi?=
 =?utf-8?B?ZStQc1Z0WW5SU1NHMkZxK0FKaU9PWllVdkFwcVdDdkFjTTBHbVMvY2lYdUVF?=
 =?utf-8?B?UHVkcTlqTjFvdTA4NVF4OHNoaXE4ZW43dm5xTktrRjVESGpaR2xoVVpaNXhD?=
 =?utf-8?B?V2tzVnV4bGlvRDJQOWd4YllsTlVIWmM3SWR3SzFpQVZhb095SThWOVE2R00w?=
 =?utf-8?B?dm5PblBNTUlUdnBVcEFpOTV2cHhUemF2K3RCMCthaG1BeklUYzdrazUvcEti?=
 =?utf-8?B?MmFWLzdZLzNoWXJxSkpwb2o4VlBEc0F5b1NZV3NLOWkxbXBWM254VFFrZXY3?=
 =?utf-8?B?UnlZTDNueTYrUTZXQUVJektBc0xmY2tCRmMwSWNybG5VSnRUU2E5blo3bXhp?=
 =?utf-8?B?enlGYmdJRHR5OVp2b3RCcUlxSFV4MnBrM0hPY1VSaTdneG1hWDRYNys4aWUz?=
 =?utf-8?B?TzFZNEFDZDVtaDlHNGh1dUswbTllZ3ZORUJ6ZWZtbFgzeno5Mlk4T0ZFQ2p1?=
 =?utf-8?B?Qk9aZkk1Z3JVR0FIUXQ4M2xPUGs2d3YvRHlabU9XRldoa0djYkJLd0YrM0xj?=
 =?utf-8?B?b3dXdDduOExHQndvajdZbXVmNkZETGducDJtR3RKOUYycWJDYm1tTjhRTS80?=
 =?utf-8?B?MXZkV0NMN1pxSU5VVnBsRkxaREp5ZDlKM0l2aTdwdDluTkl3N1lNRzJiU2pw?=
 =?utf-8?B?SnlORUIxcElxaE4yd2xxb3RRbmdBLytzMWFWRTZtK2dsSllYY1Y3cVowTEtW?=
 =?utf-8?B?Wk5DazZFNVRtLzRqZmdQTiszbVdMdS9PMWRmOGVyc3pGMVd6NDl3VXVaV3FQ?=
 =?utf-8?B?Ly8yL3c4WlB2N2JMMVJRUSt5UkRUeUNFWFpUSUtIaHg0Q29oZGdKWXd2WjNq?=
 =?utf-8?B?NXR6aFQ1VXlsQVpzbWUwWFYzUmhQbW1JSzE4VmdqMlNCVmFBazZKelMycjFq?=
 =?utf-8?B?TUdpWnRSbjU5RGtQbTV3Y05xNjA5dXlwa1ZrN3F0anMyNVpONVRoRVlxL3dQ?=
 =?utf-8?B?V3BHQ21pN29pRDhiRTljejVEVzFLQUlWUmpWRytSU2hHZW96L2lhZ1FVYVpV?=
 =?utf-8?B?enltK1Jac1d2Zk1RaFZMc2FNWkZoUG1IcExwUXBxMjV0dFc1azFiZE9GN1Vq?=
 =?utf-8?Q?DsEVwZHtUeuMExV0KCT5EoGokxttziLvJ7ER/FKCdk=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 19b2564e-5c52-4cef-f868-08d9be88ae32
X-MS-Exchange-CrossTenant-AuthSource: BN0PR11MB5744.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Dec 2021 22:34:09.9500
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 8dB1yko1tDzxbh2DsbLZBFP83f7fZetjBbCFsbBq4QB6LBQmJwFXTy/WnjMjoII68Uz7/12dNO4EH53clcu+JNwxrs3erC1egMOkLrGfrW0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR11MB1905
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Andy,

I would like to check in if you had some time to digest my responses 
with a few high level questions below ...

On 12/4/2021 3:55 PM, Reinette Chatre wrote:
> On 12/4/2021 9:56 AM, Andy Lutomirski wrote:
>> On 12/3/21 17:14, Reinette Chatre wrote:
>>> On 12/3/2021 4:38 PM, Andy Lutomirski wrote:
>>>> On 12/3/21 14:12, Reinette Chatre wrote:
>>>>> On 12/3/2021 11:28 AM, 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:
>>>>>
>>>>> Before jumping to the permission masks I would like to step back 
>>>>> and just confirm the context. We need to consider the following 
>>>>> three permissions:
>>>>>
>>>>> EPCM permissions: the enclave page permissions maintained in the 
>>>>> SGX hardware. The OS is constrained here in that it cannot query 
>>>>> the current EPCM permissions. Even so, the OS needs to ensure PTEs 
>>>>> are installed appropriately (we do not want a RW PTE for a 
>>>>> read-only enclave page)
>>>>
>>>> Why not?  What's wrong with an RW PTE for a read-only enclave page?
>>>>
>>>> If you convince me that this is actually important, then I'll read 
>>>> all the stuff below.
>>>
>>> Perhaps it is my misunderstanding/misinterpretation of the current 
>>> implementation? From what I understand the current requirement, as 
>>> enforced in the current mmap(), mprotect() as well as fault() hooks, 
>>> is that mappings are required to have identical or weaker permission 
>>> than the enclave permission.
>>
>> The current implementation does require that, but for a perhaps 
>> counterintuitive reason.  If a SELinux-restricted (or similarly 
>> restricted) process that is *not* permitted to do JIT-like things 
>> loads an enclave, it's entirely okay for it to initialize RW enclave 
>> pages however it likes and it's entirely okay for it to initialize RX 
>> (or XO if that ever becomes a thing) enclave pages from appropriately 
>> files on disk.  But it's not okay for it to create RWX enclave pages 
>> or to initialize RX enclave pages from untrusted application memory. [0]
>>
>> So we have a half-baked implementation right now: the permission to 
>> execute a page is decided based on secinfo (max permissions) when the 
>> enclave is set up, and it's enforced at the PTE level.  The PTE 
>> enforcement is because, on SGX2 hardware, the enclave can do EMODPE 
>> and bypass any supposed restrictions in the EPCM.
>>
>> The only coupling between EPCM and PTE here is that the max_perm is 
>> initialized together with EPCM, but it didn't have to be that way.
>>
>> An SGX2 implementation needs to be more fully baked, because in a 
>> dynamic environment enclaves need to be able to use EMODPE and 
>> actually end up with permissions that exceed the initial secinfo 
>> permissions.  So 
> 
> Could you please elaborate why this is a requirement? In this 
> implementation the secinfo of a page added before enclave initialization 
> (via SGX_IOC_ENCLAVE_ADD_PAGES) would indicate the maximum permissions 
> it may have during its lifetime. Pages needing to be writable and 
> executable during their lifetime can be created with RWX secinfo and 
> during the enclave runtime the pages could obtain all combinations of 
> permissions: RWX, R, RW, RX. A page added with RW secinfo may have R or 
> RW permissions during its lifetime but never RX or RWX.
> 
> So far our inquiries on whether this is acceptable has been positive and 
> is also what Dave attempted to put a spotlight on in:
> https://lore.kernel.org/lkml/94d8d631-5345-66c4-52a3-941e52500f84@intel.com/ 
> 
> 
> This above is specific to pages added before enclave initialization. In 
> this implementation pages added after enclave initialization, those 
> needing the ENCLS[EAUG] SGX2 instruction, are added with max permissions 
> of RW so could only have R or RW permissions during their lifetime. This 
> is an understood limitation and it is understood that integration with 
> user policy is required to support these pages obtaining executable 
> permission. The plan is to handle user policy integration in a series 
> that follows this core SGX2 enabling.

Are you ok with the strategy to support modification of enclave page 
permissions?

> 
>> it needs to be possible to make a page that starts out R (or RW or 
>> whatever) but nonetheless has max_perm=RWX so that the enclave can use 
>> a combination of EMODPE and (ioctl-based) EMODPR to do JIT.  So I 
>> think you should make it possible to set up pages like this, but I see 
>> no reason to couple the PTE and the EPCM permissions.
>>
>>>
>>> Could you please elaborate how you envision PTEs should be managed in 
>>> this implementation?
>>
>> As above: PTE permissions may not exceed max_perm, and EPCM is 
>> entirely separate except to the extent needed for ABI compatibility 
>> with SGX1 runtimes.
> 
> ok, so if I understand correctly you, since PTE permissions may not 
> exceed max_perm and EPCM are separate, this seems to get back to your 
> previous question of "What's wrong with an RW PTE for a read-only 
> enclave page?"
> 
> This is indeed something that we could allow but not doing so (that is 
> PTEs not exceeding EPCM permissions) would better support the SGX 
> runtime. That is why I separated out the addition of the pfn_mkwrite() 
> callback in the previous patch (04/25). Like in your example, there is a 
> RW mapping of a read-only enclave page that first results in a RW PTE 
> for the read-only enclave page. That would result in a #PF with the SGX 
> flag set (0x8007). If the PTE matches the enclave permissions the page 
> fault would have familiar 0x7 error code.
> 
> In either case user space would encounter a #PF so technically there is 
> nothing "wrong" with allowing this - even so, as motivated in the 
> previous patch: accurate exception information supports the SGX runtime, 
> which is virtually always implemented inside a shared library, by 
> providing accurate information in support of its management of the SGX 
> enclave.

Are you ok with managing PTEs in this way? It matches your requirement 
that PTE permissions may not exceed max_perm and ABI is compatible with 
SGX1. Additionally, PTEs are not allowed to exceed EPCM permissions, 
which is not an ABI change since it was not a consideration during SGX1 
where EPCM permissions could not change.


>> [0] I'm not sure anyone actually has a system set up like this or that 
>> the necessary LSM support is in the kernel.  But it's supposed to be 
>> possible without changing the ABI.
>>

Thank you very much

Reinette