Message-ID: <8fc271bf-548f-969d-96db-ca78b21578f4@intel.com>
Date:   Mon, 13 Dec 2021 14:12:44 -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 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> <fff63bde-94a4-6c0c-3333-4cf392bee50b@intel.com>
 <407c12ed28b105a055c6ab6152c0458a31052963.camel@kernel.org>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <407c12ed28b105a055c6ab6152c0458a31052963.camel@kernel.org>
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?SVg4b3BsYW9iSU12UVdZc29helJYUklqbzZWWGhWSGtvd3F5YTN6RDRYNDI0?=
 =?utf-8?B?b2JNbktiMGdHNC9ieE1ZV0ppeG8rakRtYVBOUWhlaitpd0U3OGc4Q2xoRis3?=
 =?utf-8?B?VStZUzBodVRrbHFmUG84eVdHbFVtUWpKWGptTlNGTVRSdmVYYklHZUU2QlhI?=
 =?utf-8?B?eGY2QU1pc2tNcU9STlpYdnNmeVBlYUJMVVJvMFM0a3hMWnJmTXVLMmhIenV4?=
 =?utf-8?B?SXNIRkFtUUNYUlJYMjMxUjJjOWpZaFJoN1hKRXgvZ21xSFY4LzRWTFRMeHY4?=
 =?utf-8?B?ejB5QkZBOVo5Rlh6Z1Z4c2VzY1NzN3BGWGxBcFRTV05pYU9uWkZLOW5KSVIy?=
 =?utf-8?B?ditnZFZFNEtUd1Jab3Q2b0lLZ2p1aUxkWm1xR1JxaTBjNlFaaUlBQVNHcnFi?=
 =?utf-8?B?V2gvNTJRek5UZmYxaENxYnJ1NTcvMlZFNFF3c0o3TWRoc3pIVXdwOU5MVThx?=
 =?utf-8?B?NDgxL1NsWTNYa2diakJLNUkxZXQ3WmZKZ0xtOG5oaVRPODFuNjlybXMyN0Nn?=
 =?utf-8?B?Q3BUYnlNM3hMYXhNZ3dETGVjQk1rRWJMcXc0cTlnMnNzek0zejF0TGx1QmNn?=
 =?utf-8?B?NThwQyt5Nkpjb3F6VStGOEtPRUJsUk1FcmlKM0JRRHorWm51WEIvMEJsKy9L?=
 =?utf-8?B?VVZRWU1GVkpZOXdldGFyR0ZkSUhvSC91OWR1QldTbUYrQmVwVDNNQVpaVmlK?=
 =?utf-8?B?d1JZTTBxS2RkQWwzUTJQaFhnMlFpQTloMGwxVHlXdmp3blBiMlp0TlEyTVov?=
 =?utf-8?B?YjFMeDdKNmI0WXpWOExrcjVXV2ZNN0hUVUxLL1BBV2dKTm4rcUsxSUxqb1ZC?=
 =?utf-8?B?OUtVVFFhRmhaRzJka1dRZE05ZnI2RFA0MHBqSjQ3TWJVaVFQYU9rZEFHWXFN?=
 =?utf-8?B?V3NNU0RraWY3R1VPVytIOGxtKzA4dEltbW0zejI0Vy9lS2d4Y2M1cXBqMXFJ?=
 =?utf-8?B?NjBrb1BDTHZIcDFzWm5IbXJnYURValpmMjJQczl4TGhvTWp2d0RzWHQrOTJn?=
 =?utf-8?B?R0ZwU296VWYrd3EzdWwzeGplYytVZW5TZmtIVzFicHJrVTlOVVBsRzNaYkZO?=
 =?utf-8?B?NitmZXM5cUFiVStJOWgvNlFDdnA4bmRpaHVhV1hMSlF5UnQrRE9rekx6ek9i?=
 =?utf-8?B?QnJBelRHSUVudmpROVFZUmhCMHlqbk9rQmd6Q0FWODVISzhzV3JoYllRdlZU?=
 =?utf-8?B?RXdhZFRORE1ESFlDYkNXM3FOcWpOSFVIM0RES2FZeHJKWVhGS292am5UaGN1?=
 =?utf-8?B?UUI5cUNocFJvemhIMXJKU0VhUCtEUm16UlUxUkVnMlVRMnByY3lVemE2dkNR?=
 =?utf-8?B?cnFucHJVMVZzTXBUUzRSdlZ1SGRldThDdTVheE5xcEJteG45NndyY3ExMGpC?=
 =?utf-8?B?OVhUTWRTRlBYL3piWjZpZGtlRjJzaXROY09JdWFQUEFoRWxUR0RwTnBtSzBr?=
 =?utf-8?B?Rm40dndIMFhJRkwxNk9KR25remlkRlJNU01TT0lzVURqS3hlU1VkZzZJZWJ2?=
 =?utf-8?B?TnBDRUxhdXlpR0JVMGJrRDFkeTJZeEpFZzNhU0ZIQXl6a2ZNS3pyS3ZjUEhR?=
 =?utf-8?B?K2RGS0Z5NE5qbTg5VFRqbC9LYTdpNHZHM0ozSXVUMmtIaHAxV2pxbktTQW9D?=
 =?utf-8?B?RkVlaFY4cm9IN3BvQWorNThDTWdkUll4anBYOEtZTksrb2FmMnZrcVh0MExR?=
 =?utf-8?B?RHBjK2phaXR1R2s0L1FUbm80YTJNRFl3cUp4TEJFVkhYM2NzVmEzdkVMd1RM?=
 =?utf-8?B?ZXhmejJOZ09aVnYrek84YUpLL1lHTExldys0VVowRDhrWWQ1dEVuK0NYbmFi?=
 =?utf-8?B?N2xpNTBQR1dsTUp6K1Y0YzduR2NQOTNNYTdHZGxTMnFqU0paOGM4dnF4MFBa?=
 =?utf-8?B?TUNOR0d1Z1M1SFFWSDZ3WHo0WXlIY0owNDd2U2Z4NDcrRDVGQ0VqWG1RcTZ6?=
 =?utf-8?B?bUljeVJzalowcG9nUHRXUE03UzVaV1d0UFA4RTVtekkra2lJeWRZMWhSQzdN?=
 =?utf-8?B?aHVtQ0lDZDJXUHdVOGZZakZBQVdCK0IyMlNoQ2YzTjA0cXFDM2tnVVFwejBh?=
 =?utf-8?B?WUFSeXdhRnZ3RUpaelhCeUxDMkwrTE5Od0E0QkF2NnhuRHBKd29PU25qQkpQ?=
 =?utf-8?B?bC84Sm5pNUo0RXB0ZWxnSGR1LzU0aHRRZ1h3S2VBc2lpSk80YU1FSC9OcnV5?=
 =?utf-8?B?c0E5YndwK2NmVGdXb1l5KzhGWXJ0bm9GcThmQkpCMERTUisvaHIzOWpUdG9v?=
 =?utf-8?Q?24sWLGN0R7BnrT0plkxr6qeII3VM+nN4su3hkx1hQU=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 3f995afe-f9ad-43d7-14c2-08d9be85b2e2
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:12:49.2695
 (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: C93iX3C06aSph7UwRJc6NslXtm4HyzRh6teOykk9F5jAUVc+X/cIqDmwv5oNL3hdLM5h6YVrz4EjKBoxwSzQ/3yZJ0Jg781GkbVH/DuwLfs=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR11MB1908
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/10/2021 11:57 PM, Jarkko Sakkinen wrote:
> On Mon, 2021-12-06 at 13:42 -0800, Reinette Chatre wrote:
>> 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.
> 
> SGX_IOC_ENCLAVE_ADD_PAGES is also operation working on pages within an
> enclave.
> 
> Also, to be aligned with SGX_IOC_ENCLAVE_ADD_PAGES, the new operations
> should also take secinfo as input.

ok, will do.

> 
>>
>>>
>>> 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?
> 
> I would considering introducing just one new ioctl:
> 
>    SGX_IOC_ENCLAVE_MODIFY_PAGES
> 
> and choose either operations based on e.g. a flag
> (see flags field SGX_IOC_ENCLAVE_ADD_PAGES).
> 

There seems to be different opinion about the single ioctl() as 
per:https://lore.kernel.org/lkml/0fb14185-5cc3-a963-253d-2e119b4a52bb@intel.com/

I thus plan to proceed with the two ioctls, both taking secinfo as 
input. Would that be ok with you?

Reinette