Message-ID: <fff63bde-94a4-6c0c-3333-4cf392bee50b@intel.com>
Date:   Mon, 6 Dec 2021 13:42:08 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.3.2
Subject: Re: [PATCH 10/25] x86/sgx: Support enclave page permission changes
Content-Language: en-US
To:     Jarkko Sakkinen <jarkko@kernel.org>
CC:     <dave.hansen@linux.intel.com>, <tglx@linutronix.de>,
        <bp@alien8.de>, <luto@kernel.org>, <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>
References: <cover.1638381245.git.reinette.chatre@intel.com>
 <44fe170cfd855760857660b9f56cae8c4747cc15.1638381245.git.reinette.chatre@intel.com>
 <Yav0/3jeJsuT3yEq@iki.fi>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <Yav0/3jeJsuT3yEq@iki.fi>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?c2ZuekFzaVgwQWpJZHdrc2MydmRIdC8ra2w2NW5FUll1bUhScUJ5Ym4wVGhM?=
 =?utf-8?B?S1BubjMyb0VaMmcranBFdVVCdElpc0RrNCtnRExoS1Zvc1ZnYmRqaER2NTJs?=
 =?utf-8?B?cnB5SWRSekJFRDZDN0RtbEFudXAzYXZSNHZmVXVSQ0xCRWg2UXYyWm5sVUNm?=
 =?utf-8?B?ZWVRYUQwbWthaGhzNXIzTWxlUGp4Y1JldEkyMWNwTU1PUi9OakthdjVlQ3dV?=
 =?utf-8?B?d1hKYndNeTNBOEtFOXZFVzF6ekJ1Qkd5N1YzSlg4MzZsaUY3bU5ZenoyVGRr?=
 =?utf-8?B?VEZsZWVuSldzbkgveXRWdTBSdkhibWpJT2FPYmUvUWdmbGtSV1RwWGFIQm9F?=
 =?utf-8?B?aTRtM3ZqVTJvRmIza0RpMGI4K3l3NkVjWEpjU05GQ1VjcFdoS1pmMHN0dEd4?=
 =?utf-8?B?MXFkSmI2VEFYTDVScjNHU25vWk51OXVJakYzMWpKcXlsWUp2enhVZ2ZZK09V?=
 =?utf-8?B?SGhCNjRGS1l2SU12T1E0QkZkeXpuMFlCNnRmR0R5ZVNUcGJrajh2MFZFbnRu?=
 =?utf-8?B?Y0tRNjVCL2F2K2s4dHpNYkNrcW5LZmVtZ1J0bUtJa2RQak1BczIxWnFtSllk?=
 =?utf-8?B?Skh3aWViSTlQVjF4TVphdWZvRkNsWXdqQ3QyUnBQWUNyVVpZSjkweGR0VUlB?=
 =?utf-8?B?NkFCUnVQYTJjZ29vc20xcFgrcHdkUDZzc1ZWNkpBUUc1RUFnaEwzeTdwbHlh?=
 =?utf-8?B?R3h2eXpUakR1RUYxQUtPaXRaaVBwRG1FdXpEUVFMcXZBWHBUbGlNS24yY1JQ?=
 =?utf-8?B?UEloVGdGMU5rVS9Lc3NlOTdBTGxldU9aVXFYWXJZMFRvbVVXelZmOGswdUEx?=
 =?utf-8?B?Uzg2UjVXVXpSeG9PTUQ3YUord2hGeVlRdmdISkdicjNkNDlpbWtCN1daY0tU?=
 =?utf-8?B?VXRpdDh0N3hqZU9DRjNxS3dTOXQ2TWJKSmdzOWJDQmpyVXVPOTNCUGtMT3Z0?=
 =?utf-8?B?Wk1FaktyOXNJRzRjQ0NqQ3pZdDA4T0FXZ2lkMEV1TGNpTWUzTGNaK1lSYnhV?=
 =?utf-8?B?ZkpUUHhhVFpweVpxcklEdzczblJYSm1IRHBud0RDampwcjFIcVNkRXptVXVB?=
 =?utf-8?B?dGhuTXZXWWI3OVVxbXBGUit3dVhSd2pjTHZhNE5yNGhIRHlqT3BIZi9NQnN2?=
 =?utf-8?B?M1prUFhKdy9wOThDQXhyc2tNaVExL2JKMHZ4dGhjUjU2eWVJZDVHeEZxUkJn?=
 =?utf-8?B?VEVMbi96SUJvTEFGUk84M29oUUsyT0tlZ3NtMk9oWFVYY1pnMmFZZDA1SlVS?=
 =?utf-8?B?RDlDQklsZzJwRnBMd25ub2JCS0ZoOVlnWXRCUnV0KzdFNEVMNGV3RHowNVZ3?=
 =?utf-8?B?aFZNTG41Qnp0TGl4WmVWTlJXSExEdlFkZThvaVpPaGRSUE5jMExSUTM4YW9Q?=
 =?utf-8?B?Z095cng0Wkp4aDR5by9RTm0rSEJVdDNtVDBFY1ozMkMyd25WNmMvYiswQ2ds?=
 =?utf-8?B?M1NXZDRjMXNkZTFMdWFUalI1MlZueVVkVi9tR2VXNHUvTFBGc2o0Qmw5VWU5?=
 =?utf-8?B?SXE3NzdOZnpWQ2w2b1dZVWZFOHFvSTFUSjBNdFludGZtYkFNamYzYnVnTHFI?=
 =?utf-8?B?NFZjK2ZEeWU3N2FiRGFUZWRZcWI2aURnbUxDSmdwMFhQVEljMDByNDZUb05P?=
 =?utf-8?B?UWVXZFZVKzdyNjhHVUtPU2dKYitTYTA5bzV2dW9QYUd5ejV2aW1Ua1ZIRDRk?=
 =?utf-8?B?bVQrQzdidXJQR01ROS9jNE5kd0tTcmp1bFIxamtGZ2QvaFVJaGFzR1lZS0Fk?=
 =?utf-8?B?a28vcUVkbXVxQ0IvS2RGYkt1RTZTNlZBTHVvR1JhdmgrZ3QxakxnbkJCOVpk?=
 =?utf-8?B?MmJMVUdtL2Z3MFhWd2xNMXM4SlBuNFNEbmMyOUwxUnVxcEtORTBBMkZJMUpp?=
 =?utf-8?B?RWZWNkhEQys0UWxCbjczbWs0VkowTDBLcFJwSGxjcHpGSnNJS0xMS3RRYWRh?=
 =?utf-8?B?YlNZeDczN2g3QnNaYXZ6TGwyUng1RXMzTmowOUJZakFETkYybWlrNGdoVTFp?=
 =?utf-8?B?cnFvUUMzZnNlUm8wRzRHVm5leC9YZC9wL2kxMURrWksrK3ZkeVl6TER0V3ha?=
 =?utf-8?B?bVhMQ21xY3IxdUhvaGRWNEs0aVZNRXc5aVk1SzJRaGtLNkVEWHNCMktSTWVl?=
 =?utf-8?B?Z0M4VUJiRHNmQXRxK1U2eGVYSmNraUE4d2xzYVdzTE8wK2htNUhpQ0ZtVmNL?=
 =?utf-8?B?OFJkNGNRUmJmRVBKUnVEY3ozdGphSFZkQWVlTzRjb21KdTkxajd1KzNrR1NO?=
 =?utf-8?Q?et8cV9plgGsxeXflN8J2TAVeHrLh/cIDRUszNLt1f0=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: f8d49f5a-a897-4f01-5654-08d9b90143ee
X-MS-Exchange-CrossTenant-AuthSource: BN0PR11MB5744.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Dec 2021 21:42:13.7167
 (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: UmcgFaPp9cshi7T6LLajsOBtpYfs36EJfgx8WVAOHjfmAyOfQHfNSMbdF8/LcgUUPoYcJQZ35x6EHuZw8almEFoEmaQhqAr6McUXY9AbBtI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR11MB1364
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Jarkko,

On 12/4/2021 3:08 PM, Jarkko Sakkinen wrote:
> On Wed, Dec 01, 2021 at 11:23:08AM -0800, Reinette Chatre wrote:
>> In the initial (SGX1) version of SGX, pages in an enclave need to be
>> created with permissions that support all usages of the pages, from the
>> time the enclave is initialized until it is unloaded. For example,
>> pages used by a JIT compiler or when code needs to otherwise be
>> relocated need to always have RWX permissions.
>>
>> SGX2 includes two functions that can be used to modify the enclave page
>> permissions of regular enclave pages within an initialized enclave.
>> ENCLS[EMODPR] is run from the OS and used to restrict enclave page
>> permissions while ENCLU[EMODPE] is run from within the enclave to
>> extend enclave page permissions.
>>
>> Enclave page permission changes need to be approached with care and
>> for this reason this initial support is to allow enclave page
>> permission changes _only_ if the new permissions are the same or
>> more restrictive that the permissions originally vetted at the time the
>> pages were added to the enclave. Support for extending enclave page
>> permissions beyond what was originally vetted is deferred.
> 
> This paragraph is out-of-scope for a commit message. You could have
> this in the cover letter but not here. I would just remove it.

I think this is essential information that is mentioned in the cover 
letter _and_ in this changelog. I will follow Dave's guidance and avoid 
"deferred" by just removing that last sentence.

> 
>> Whether enclave page permissions are restricted or extended it
>> is necessary to ensure that the page table entries and enclave page
>> permissions are in sync. Introduce a new ioctl, SGX_IOC_PAGE_MODP, to
> 
> SGX_IOC_PAGE_MODP does not match the naming convetion of these:
> 
> * SGX_IOC_ENCLAVE_CREATE
> * SGX_IOC_ENCLAVE_ADD_PAGES
> * SGX_IOC_ENCLAVE_INIT

ah - my understanding was that the SGX_IOC_ENCLAVE prefix related to 
operations related to the entire enclave and thus I introduced the 
prefix SGX_IOC_PAGE to relate to operations on pages within an enclave.

> 
> A better name would be SGX_IOC_ENCLAVE_MOD_PROTECTIONS. It doesn't
> do harm to be a more verbose.

Will do. I see later you propose SGX_IOC_ENCLAVE_MODIFY_TYPE - would you 
like them to be consistent wrt MOD/MODIFY?

Reinette