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  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:   Thu, 26 Oct 2017 14:26:15 -0500
From:   Brijesh Singh <brijesh.singh@....com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     brijesh.singh@....com, kvm@...r.kernel.org,
        Paolo Bonzini <pbonzini@...hat.com>,
        Radim Krčmář <rkrcmar@...hat.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Gary Hook <gary.hook@....com>,
        Tom Lendacky <thomas.lendacky@....com>,
        linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [Part2 PATCH v6 13/38] crypto: ccp: Add Secure Encrypted
 Virtualization (SEV) command support



On 10/26/2017 12:44 PM, Borislav Petkov wrote:
> On Thu, Oct 26, 2017 at 11:56:57AM -0500, Brijesh Singh wrote:
>> The variable is used as ref counter.
> 
> ... and it can't be converted to a boolean because...?
> 

SHUTDOWN command unconditionally transitions a platform to uninitialized 
state. The command does not care how many processes are actively using 
the PSP. We don't want to shutdown the firmware while other process is 
still using it.

e.g consider three processes (A, B, C)

Process A:
----------
sev_platform_init()
sev_do_cmd(..)
...
...
sev_do_cmd(..)
...
sev_platform_shutdown()

Process B:
-----------
sev_platform_init()
sev_do_cmd(...)
sev_platform_shutdown()

Process C:
----------
sev_platform_init()
sev_do_cmd(...)
sev_do_cmd(...)
sev_do_cmd(...)
sev_platform_shutdown()

As per the SEV spec section 5.1.2 (platform state machine), several 
commands require that platform should be initialized before issuing the 
actual command. As you can see Process B may finish quickly and SHUTDOWN 
from process B will simply uninitialize the firmware and cause 
unexpected result to process A and C.


>> In your previous reply you comments on global semaphore (fw_init_mutex) and
>> in response I tried to highlight why we need the global semaphore. Did I
>> misunderstood your comment ?
> 
> Yes, what happens if you get preempted while holding the mutex? Will the other
> process be able to do anything?
> 

If other process tries to issue the sev_platform_init/shutdown() then 
they have to wait.

The sev_platform_init() and sev_platform_shutdown() uses the same global 
mutex. See the original code below.

+static int __sev_platform_init(struct sev_data_init *data, int *error)
+{
+	int rc = 0;
+
+	mutex_lock(&fw_init_mutex);
+
+	if (!fw_init_count) {
+		rc = sev_do_cmd(SEV_CMD_INIT, data, error);
+		if (rc)
+			goto unlock;
+	}
+
+	fw_init_count++;
+
+unlock:
+	mutex_unlock(&fw_init_mutex);
+	return rc;
+
+}
+
+int sev_platform_shutdown(int *error)
+{
+	int rc = 0;
+
+	mutex_lock(&fw_init_mutex);
+
+	if (!fw_init_count)
+		goto unlock;
+
+	if (fw_init_count == 1) {
+		rc = sev_do_cmd(SEV_CMD_SHUTDOWN, 0, error);
+		if (rc)
+			goto unlock;
+	}
+
+	fw_init_count--;
+
+unlock:
+	mutex_unlock(&fw_init_mutex);
+	return rc;
+}

Powered by blists - more mailing lists