linux-kernel - Re: [PATCH v3 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]

Date:   Mon, 18 Oct 2021 17:17:30 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        dave.hansen@...ux.intel.com, x86@...nel.org, yang.zhong@...el.com,
        jarkko@...nel.org, bp@...e.de
Subject: Re: [PATCH v3 2/2] x86: sgx_vepc: implement SGX_IOC_VEPC_REMOVE ioctl

> +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) {
> +		int ret = sgx_vepc_remove_page(entry);
> +		if (ret) {
> +			if (ret == SGX_CHILD_PRESENT) {

There's a ton of documentation in the changelog and official docs, but a comment
here would also be helpful.

> +				failures++;
> +			} else {
> +				/*
> +				 * Unlike in sgx_vepc_free_page, userspace might
> +				 * call the ioctl while logical processors are
> +				 * running in the enclave, or cause faults due
> +				 * to concurrent access to pages under the same
> +				 * SECS.  So we cannot warn, we just report it.

Technically the kernel can WARN on #PF[*], as EREMOVE only hits #PF if there's a
legitimate #PF or if the target page is not an EPC page.  FWIW, the comments are
a little less compressed if the if statements aren't nested.

		if (ret == SGX_CHILD_PRESENT) {
			/*
			 * Track and return the number of SECS pages that cannot
			 * be removed because they have child EPC pages (in this
			 * vEPC or a different vEPC).
			 */
			failures++;
		} else if (ret) {
			/*
			 * Report errors due to #GP or SGX_ENCLAVE_ACT, but do
			 * not WARN as userspace can induce said failures by
			 * calling the ioctl concurrently on multiple vEPCs or
			 * while one or more CPUs is running the enclave.  Only
			 * a #PF on EREMOVE indicates a kernel/hardware issue.
			 */
			WARN_ON_ONCE(encls_faulted(ret) &&
				     ENCLS_TRAPNR(ret) == X86_TRAP_PF);
			return -EBUSY;
		}

[*] SGX1 hardware has an erratum where it signals #GP instead of #PF, but that's
    ok in this case because it's a false negative, not a false positive.

> +				 */
> +				return -EBUSY;
> +			}
> +		}
> +		cond_resched();
> +	}
> +
> +	/*
> +	 * Return the number of pages that failed to be removed, so
> +	 * userspace knows that there are still SECS pages lying
> +	 * around.

Nit, the comment doesn't need to span three lines.

	/*
	 * Return the number of pages that failed to be removed, so userspace
	 * knows that there are still SECS pages lying around.
	 */

> +	 */
> +	return failures;
> +}