Re: [Qemu-devel] [PULL 10/22] sev/i386: add command to initialize the memory encryption context

[Peter Maydell]

On 13 March 2018 at 12:56, Paolo Bonzini  wrote:
> From: Brijesh Singh 
>
> When memory encryption is enabled, KVM_SEV_INIT command is used to
> initialize the platform. The command loads the SEV related persistent
> data from non-volatile storage and initializes the platform context.
> This command should be first issued before invoking any other guest
> commands provided by the SEV firmware.

Hi; Coverity points out a memory leak in this code
(CID 1390613):

> +void *
> +sev_guest_init(const char *id)
> +{
> +SEVState *s;
> +char *devname;
> +int ret, fw_error;
> +uint32_t ebx;
> +uint32_t host_cbitpos;
> +struct sev_user_data_status status = {};
> +
> +s = g_new0(SEVState, 1);

Here we allocate memory into 's'...

> +s->sev_info = lookup_sev_guest_info(id);
> +if (!s->sev_info) {
> +error_report("%s: '%s' is not a valid '%s' object",
> + __func__, id, TYPE_QSEV_GUEST_INFO);
> +goto err;

...and this error-exit path will not free it because
it does g_free(sev_state), not g_free(s).

> +}
> +
> +sev_state = s;
> +s->state = SEV_STATE_UNINIT;

[...]

> +err:
> +g_free(sev_state);
> +sev_state = NULL;
> +return NULL;
> +}

The simplest fix is probably to move the 'sev_state = s;
s->state = SEV_STATE_UNINIT;' assignments to earlier in the
function.

Cleaner might be to ensure that nothing in the rest of
the function depends on sev_state being initialized,
and then to work purely with 's' and only set sev_state
to s just before returning success.

thanks
-- PMM

[Qemu-devel] [PULL 10/22] sev/i386: add command to initialize the memory encryption context

[Paolo Bonzini]

From: Brijesh Singh 

When memory encryption is enabled, KVM_SEV_INIT command is used to
initialize the platform. The command loads the SEV related persistent
data from non-volatile storage and initializes the platform context.
This command should be first issued before invoking any other guest
commands provided by the SEV firmware.

Cc: Paolo Bonzini 
Cc: Richard Henderson 
Cc: Eduardo Habkost 
Signed-off-by: Brijesh Singh 
Signed-off-by: Paolo Bonzini 
---
 target/i386/Makefile.objs |   1 +
 target/i386/monitor.c |  12 ++-
 target/i386/sev-stub.c|  41 +
 target/i386/sev.c | 224 ++
 target/i386/sev_i386.h|  24 +
 target/i386/trace-events  |   3 +
 6 files changed, 303 insertions(+), 2 deletions(-)
 create mode 100644 target/i386/sev-stub.c

diff --git a/target/i386/Makefile.objs b/target/i386/Makefile.objs
index d4697d8..04678f5 100644
--- a/target/i386/Makefile.objs
+++ b/target/i386/Makefile.objs
@@ -7,6 +7,7 @@ obj-$(CONFIG_SOFTMMU) += machine.o arch_memory_mapping.o 
arch_dump.o monitor.o
 obj-$(CONFIG_KVM) += kvm.o hyperv.o
 obj-$(CONFIG_SEV) += sev.o
 obj-$(call lnot,$(CONFIG_KVM)) += kvm-stub.o
+obj-$(call lnot,$(CONFIG_SEV)) += sev-stub.o
 # HAX support
 ifdef CONFIG_WIN32
 obj-$(CONFIG_HAX) += hax-all.o hax-mem.o hax-windows.o
diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index 0d1556f..4eae0a6 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -31,6 +31,7 @@
 #include "sysemu/kvm.h"
 #include "hmp.h"
 #include "qapi/error.h"
+#include "sev_i386.h"
 #include "qapi/qapi-commands-misc.h"
 
 
@@ -666,6 +667,13 @@ void hmp_info_io_apic(Monitor *mon, const QDict *qdict)
 
 SevInfo *qmp_query_sev(Error **errp)
 {
-error_setg(errp, "SEV feature is not available");
-return NULL;
+SevInfo *info;
+
+info = sev_get_info();
+if (!info) {
+error_setg(errp, "SEV feature is not available");
+return NULL;
+}
+
+return info;
 }
diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
new file mode 100644
index 000..c86d8c1
--- /dev/null
+++ b/target/i386/sev-stub.c
@@ -0,0 +1,41 @@
+/*
+ * QEMU SEV stub
+ *
+ * Copyright Advanced Micro Devices 2018
+ *
+ * Authors:
+ *  Brijesh Singh 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "sev_i386.h"
+
+SevInfo *sev_get_info(void)
+{
+return NULL;
+}
+
+bool sev_enabled(void)
+{
+return false;
+}
+
+uint64_t sev_get_me_mask(void)
+{
+return ~0;
+}
+
+uint32_t sev_get_cbit_position(void)
+{
+return 0;
+}
+
+uint32_t sev_get_reduced_phys_bits(void)
+{
+return 0;
+}
diff --git a/target/i386/sev.c b/target/i386/sev.c
index ab42e4a..91b5190 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -11,6 +11,11 @@
  *
  */
 
+#include 
+#include 
+
+#include 
+
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qom/object_interfaces.h"
@@ -18,10 +23,88 @@
 #include "sysemu/kvm.h"
 #include "sev_i386.h"
 #include "sysemu/sysemu.h"
+#include "trace.h"
 
 #define DEFAULT_GUEST_POLICY0x1 /* disable debug */
 #define DEFAULT_SEV_DEVICE  "/dev/sev"
 
+static SEVState *sev_state;
+
+static const char *const sev_fw_errlist[] = {
+"",
+"Platform state is invalid",
+"Guest state is invalid",
+"Platform configuration is invalid",
+"Buffer too small",
+"Platform is already owned",
+"Certificate is invalid",
+"Policy is not allowed",
+"Guest is not active",
+"Invalid address",
+"Bad signature",
+"Bad measurement",
+"Asid is already owned",
+"Invalid ASID",
+"WBINVD is required",
+"DF_FLUSH is required",
+"Guest handle is invalid",
+"Invalid command",
+"Guest is active",
+"Hardware error",
+"Hardware unsafe",
+"Feature not supported",
+"Invalid parameter"
+};
+
+#define SEV_FW_MAX_ERROR  ARRAY_SIZE(sev_fw_errlist)
+
+static int
+sev_ioctl(int fd, int cmd, void *data, int *error)
+{
+int r;
+struct kvm_sev_cmd input;
+
+memset(&input, 0x0, sizeof(input));
+
+input.id = cmd;
+input.sev_fd = fd;
+input.data = (__u64)data;
+
+r = kvm_vm_ioctl(kvm_state, KVM_MEMORY_ENCRYPT_OP, &input);
+
+if (error) {
+*error = input.error;
+}
+
+return r;
+}
+
+static int
+sev_platform_ioctl(int fd, int cmd, void *data, int *error)
+{
+int r;
+struct sev_issue_cmd arg;
+
+arg.cmd = cmd;
+arg.data = (unsigned long)data;
+r = ioctl(fd, SEV_ISSUE_CMD, &arg);
+if (error) {
+*error = arg.error;
+}
+
+return r;
+}
+
+static const char *
+fw_error_to_str(int code)
+{
+if (code < 0 || code >= SEV_FW_MAX_ERROR) {
+return "unknown error";
+}
+
+return sev_fw_errlist[code];
+}
+
 static void
 qsev_guest_finalize(Object *obj)
 {
@@ -219,6 +302,147 @@ static const TypeInf