Re: [PATCH V2] x86/sgx: Add poison handling to reclaimer

[Reinette Chatre]

Hi Jarkko,

On 1/20/2022 5:09 AM, Jarkko Sakkinen wrote:
> On Wed, 2022-01-19 at 14:23 -0800, Reinette Chatre wrote:
>> The SGX reclaimer code lacks page poison handling in its main
>> free path. This can lead to avoidable machine checks if a
>> poisoned page is freed and reallocated instead of being
>> isolated.
>>
>> A troublesome scenario is:
>>  1. Machine check (#MC) occurs (asynchronous, !MF_ACTION_REQUIRED)
>>  2. arch_memory_failure() is eventually called
>>  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 (poison pages should be isolated on the
>>     sgx_poison_page_list instead)
>>  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 by replacing the open coded
>> enclave page freeing code in the reclaimer with sgx_free_epc_page()
>> to obtain support for poison page handling that includes placing the
>> poisoned page on the correct list.
>>
>> Fixes: d6d261bded8a ("x86/sgx: Add new sgx_epc_page flag bit to mark
>> free pages")
>> Fixes: 992801ae9243 ("x86/sgx: Initial poison handling for dirty and
>> free pages")
> 
> Same comment as for the first version: remove the first fixes tag.
> 

For completeness I'll duplicate my response also:

The commit you refer to, commit d6d261bded8a ("x86/sgx: Add new
sgx_epc_page flag bit to mark free pages", introduced a new page flag bit
(SGX_EPC_PAGE_IS_FREE) that should be set when an EPC page is freed. The
commit also sets the bit in sgx_free_epc_page() when an EPC page is freed.
The commit should also have set that bit when the EPC page is freed in the
reclaimer, which contains an open coded version of sgx_free_epc_page(),
but it did not. This fix adds the snippet that was omitted from that
commit.

Reinette