Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <effafec4-affc-297b-4b0e-aeca1884fa4f@intel.com>
Date:   Wed, 19 Jan 2022 12:47:22 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.5.0
Subject: Re: [PATCH] x86/sgx: Add poison handling to reclaimer
Content-Language: en-US
To:     Dave Hansen <dave.hansen@intel.com>, <tony.luck@intel.com>,
        <dave.hansen@linux.intel.com>, <jarkko@kernel.org>,
        <tglx@linutronix.de>, <bp@alien8.de>, <luto@kernel.org>,
        <mingo@redhat.com>, <linux-sgx@vger.kernel.org>, <x86@kernel.org>
CC:     <linux-kernel@vger.kernel.org>
References: <ef74bd9548df61f77e802e7505affcfb5159c48c.1642545829.git.reinette.chatre@intel.com>
 <9cefa244-9830-c158-6112-b2c61a464632@intel.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <9cefa244-9830-c158-6112-b2c61a464632@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UWdBcU9oNXY1VTdGWUFQZFNRazdQMGpLaGtiRis3Z3hnVlA5a2hHbDBEdWdF?=
 =?utf-8?B?TytabFFra01FUmpuV0I0N3lCVkxjU1Fyb09DSHl1VC92cTlLUTRFa3FpYmVW?=
 =?utf-8?B?ZHJ5YllicGRIZWZLWmlraEw2KzZmNmJQMjB2a1lGZzNWeGw2bmdZMnRmZnAr?=
 =?utf-8?B?QTh3RU81b1BKK0h2S3RrQlVyNVo1dDdYaGpDdmtzVjh5dXhDRzYvN1h6YkJV?=
 =?utf-8?B?UXR5Yzl0WFZlM3U3Z3JTVUx1MmxURm9sVzVmUEVwYXpGK0xZT29LdUhBU1lo?=
 =?utf-8?B?ZENCc0JVWUtNa0V6cGhUM2l6MFVpSThJYWNMNFRuTnlKT0cxNWdSaXdlTmtw?=
 =?utf-8?B?UmVUdE53akduazlTcG5CRFpBYnVIZStBWDJDQ1pFZDErVm9OQXJhY3BkaXhQ?=
 =?utf-8?B?ckw3b1BIUWJBT0tZYUlsSnZBSlg1NDFtTmg2MW82anh5UW52UkhyWGk5dGtK?=
 =?utf-8?B?bnd0UVh1VnU5MWk5ZjNZUFI1KzZyYVkzMytIaFZWMEVsdzY0RFRqSERISDRL?=
 =?utf-8?B?U2xtZ241eXNHYUIrQ2RSTjQ5eDJyT2t6WXc2QWJRTWZFWGRkVFhOUmVSV2Y5?=
 =?utf-8?B?dnZkRk8xcEwvd2M0eWgza2dwTEhTMUYxSTV4Z2gzN1pVVDJJQUF1VFZJUzda?=
 =?utf-8?B?cEg4OUg0ZmY4Zy9GclpWZlNNcDVBSlhxMFJKT0RjK2Q5K0tOWlp0aGFzdTlk?=
 =?utf-8?B?cXJXS0k5VlRXVXBORWFsU3FkRkFSRjhSL2w1b0x0TEdkZUdlZUxjNHRmcVJE?=
 =?utf-8?B?ai8rUk5nbUpyblp4RU5YT1B1alEwUlljanRyK2UwMnFIbE16aVVyOGFuckdk?=
 =?utf-8?B?aDV6RGc0ZlFzMzVlRG9nZGo5RS9rNlFqbDZjYjczVmdEcFNHL0hqQ0wzL1JO?=
 =?utf-8?B?YlQwZDBIV1VSdktBOXJvTWdFd0VOdnFwaHVSTU5YY1FNaEpTdHhMWnhudks5?=
 =?utf-8?B?Z1RYcWFMVnM5VnBSaVNkOW9uOU53KzNiRWZ2M0RGbFVXNEFWS2ZzQUxHcWF1?=
 =?utf-8?B?NGNCcjZaMEJ3RDdOemd5eXEwVWprV1FRcWUvM3o5Ti85aEdNdkNNT1R1aVRW?=
 =?utf-8?B?dHJGemlsWWVBUzkrS09wVE9wcGxDNTRZZHRldmt3enJvRGl5Tk84WUl6WFhJ?=
 =?utf-8?B?OWZtTWFGOGROa2pEa3I1RXhUL3FFbU92MFFncW1ST2lpQitrc3doM2RoNWFK?=
 =?utf-8?B?WEFGUWFKOFpWdUJMNXA4Vm1zL01RaFhtM3FSUThEWnBqajJiOUZUWlFDS1lG?=
 =?utf-8?B?SFJBcjJGRUVoSFEzMDhRalN0Yit0RHBYYU5lY3U3K3J4bFdqRytMQjV0Tzli?=
 =?utf-8?B?MGNESTkxdXltYmR5NjJkSlI0ZjNoeUVIa3dINlU2TDR5RktiZ3JqaWJFcVdr?=
 =?utf-8?B?QmZUR3dvQmovU1dhb1g5WmxxYi9rUzZaa3dEL3pmcnpIL0xYTmlpTHNPS2Iz?=
 =?utf-8?B?K080aG8vOW43TFhXT3d2cHpCdmlTV3lnSnA0cllFeFlyVm80WTdTNzUxZGNB?=
 =?utf-8?B?T3BQUDkzSG5Tb1crSlRtNlY4MTJJcUNudXNSdkt2QURsL1ZFeEREbFFCZnVn?=
 =?utf-8?B?ZTZVUGIxWTdtSWdSWExMSi9lNEZjYnNSK3FWa0lHS2piOE83NXFyVzhyY2FW?=
 =?utf-8?B?alBuUEFrb1JOZitpK0hrRDFqWndnRisxVTNLbVRYdkFtaUhDdXJsTUhXMi9x?=
 =?utf-8?B?Y05Qc1FoeUIrOFl4eFVpSUR3ZE1RcGhEOEh5K0FZTSs2bmNTZTVONmZEOGFz?=
 =?utf-8?B?Wk5Ocit2TUpGNnhQbElQQyt6RzhzUUJaS25MN3lJY3hYYzBzSGw4ak9NcTlJ?=
 =?utf-8?B?dGlNZGM4WU5IQ2phSGMybzBqWmVlNzNCblBmdDRJY3dFWHVlcXoyUUtsbUlx?=
 =?utf-8?B?blpUTHpUc1NsL01zR1o3MmZBcmJoNDUyTUZ1UVh1SjJDWXh4TG5OTVBaZGg5?=
 =?utf-8?B?Nm1TTXk0UU5PLzhwd1greG1vbDhhZFBlVUlSVEJWK1RTSzBPclZwbzBYSzlZ?=
 =?utf-8?B?UGJIelUrSnJhTUY5N2JjZzVxcm5NYnlMVENGZ3NWdEhOOTg2Z2NvaTNyUWxX?=
 =?utf-8?B?bTVEa1FLQUdMeHpPSUpMUm5mYTA5Uk5qdGpmdnJGb0FxVzhBTUErT1gvb0gv?=
 =?utf-8?B?MzNTS3JPVnE1TWswbHVsUEVDUFVXYStYUlZabXlkVGl6aDIwempOY0hZRzVv?=
 =?utf-8?B?QWEwSkhYYjVmMWlkV0JnVFQvemUzdTJSdHNhc2FMeTNKRURGZHJNUC9BYlpa?=
 =?utf-8?B?eGZwcExKZ3hVYVl2TkV6bkhpcFRBPT0=?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 8fd61c68-dd1c-4c48-e2c0-08d9db8ce6bd
X-MS-Exchange-CrossTenant-AuthSource: BN0PR11MB5744.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Jan 2022 20:47:26.6181
 (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: ZuPpxbhA2IwLHqhlALv96b+eyi8tzRAZyqAMGJhliekScClB8prxhKi+Ux5rkO1kKkntbvS7jpoToZR+3hE8kSz8q269mPIK12JoIyDgUVU=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB5577
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Dave,

On 1/19/2022 11:51 AM, Dave Hansen wrote:
> On 1/18/22 3:05 PM, Reinette Chatre wrote:
>> The machine check recovery handling in SGX added the changes
>> listed below to the freeing of pages in sgx_free_epc_page().
>> The SGX reclaimer contains an open coded version of
>> sgx_free_epc_page() and thus did not obtain the changes in
>> support of poison handling.
> 
> I was trying to decide if this is an urgent fix or not.  A more crisp
> problem statement might have helped in the changelog.
> 
> But, from what I can tell, the most probable troublesome scenario here
> would be something like:
> 
>  1. Machine check (#MC) occurs (asynchronous, !MF_ACTION_REQUIRED)
>  2. arch_memory_failure() called is eventually
>  3. (SGX) page->poison set to 1
>  4. Page is reclaimed
>  5. Page added to normal free lists by sgx_reclaim_pages()
>     ^ This is the bug
>  6. Page is reallocated by some innocent enclave, a second (synchronous)
>     in-kernel #MC is induced, probably during EADD instruction.
>     ^ This is the fallout from the bug
> 
> #6 is unfortunate and can be avoided if this patch is applied.
> 
> Basically, this patch ensures that a bad enclave page is isolated
> quickly and causes a minimal amount of collateral damage.  Is this a
> valid summary?
> 
> 	The SGX reclaimer code lacks page poison handling in its free
> 	path.  This can lead to completely avoidable machine checks if a
> 	poisoned page is freed and reallocated instead of being
> 	isolated.

As I understand this code it does look like a valid summary to me. One
detail is that the poison page handling is currently done for SECS pages
when they are freed by the reclaimer (via sgx_reclaimer_write()).

Thank you very much for the detailed analysis. Should I resend with
an improved commit message that contains your scenario description
and summary?

Reinette