linux-kernel - Re: [PATCH 2/2] x86: sgx_vepc: implement SGX_IOC_VEPC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <50287173-0afb-36f4-058e-0960fb4017a7@intel.com>
Date:   Mon, 13 Sep 2021 12:33:24 -0700
From:   Dave Hansen <dave.hansen@...el.com>
To:     Paolo Bonzini <pbonzini@...hat.com>, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org
Cc:     x86@...nel.org, linux-sgx@...r.kernel.org, jarkko@...nel.org,
        dave.hansen@...ux.intel.com, yang.zhong@...el.com
Subject: Re: [PATCH 2/2] x86: sgx_vepc: implement SGX_IOC_VEPC_REMOVE ioctl

On 9/13/21 6:11 AM, Paolo Bonzini wrote:
> +static long sgx_vepc_remove_all(struct sgx_vepc *vepc)
> +{
> +	struct sgx_epc_page *entry;
> +	unsigned long index;
> +	long failures = 0;
> +
> +	xa_for_each(&vepc->page_array, index, entry)
> +		if (sgx_vepc_remove_page(entry))
> +			failures++;
> +
> +	/*
> +	 * Return the number of pages that failed to be removed, so
> +	 * userspace knows that there are still SECS pages lying
> +	 * around.
> +	 */
> +	return failures;
> +}

I'm not sure the retry logic should be in userspace.  Also, is this
strictly limited to SECS pages?  It could also happen if there were
enclaves running that used the page.  Granted, userspace can probably
stop all the vcpus, but the *interface* doesn't prevent it being called
like that.

What else can userspace do but:

	ret = ioctl(fd, SGX_IOC_VEPC_REMOVE);
	if (ret)
		ret = ioctl(fd, SGX_IOC_VEPC_REMOVE);
	if (ret)
		printf("uh oh\n");

We already have existing code to gather up the pages that couldn't be
EREMOVE'd and selectively EREMOVE them again.  Why not reuse that code
here?  If there is 100GB of EPC, it's gotta be painful to run through
the ->page_array twice when once plus a small list iteration will do.

Which reminds me...  Do we need a cond_resched() in there or anything?
That loop could theoretically get really, really long.