On 4/28/22 13:11, Reinette Chatre wrote:
>
> Since the PCMD page in the backing store is modified the page
> should be set as dirty when releasing the reference to
> ensure the modified data is retained.
>
> Fixes: 08999b2489b4 ("x86/sgx: Free backing memory after faulting the enclave page")
> Signed-off-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>
> ---
> arch/x86/kernel/cpu/sgx/encl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> index e5d2661800ac..e03f124ce772 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.c
> +++ b/arch/x86/kernel/cpu/sgx/encl.c
> @@ -98,7 +98,7 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page,
> kunmap_atomic(pcmd_page);
> kunmap_atomic((void *)(unsigned long)pginfo.contents);
>
> - sgx_encl_put_backing(&b, false);
> + sgx_encl_put_backing(&b, true);
I think you're on the right track here. The concept is right. The
memset() wrote fresh data into the b.pcmd page. But, if it were clean
swap cache, it can be discarded again and the memset() might be lost.
I *think* all that would do in the end is leave us with a PCMD page that
will never be truncated because the page has non-zero PCMD data that
will never be used, the result of the thrown-away memset().
But, I'd rather this be done more directly and closer to the actual
dirtying of the page. Perhaps:
+ set_page_dirty(b.pcmd);
memset(pcmd_page + b.pcmd_offset, 0, sizeof(struct sgx_pcmd));
I don't think the "b.contents" page needs the same treatment because its
contents are always discarded in this path while some of the PCMD page's
contents need to be preserved.
