[PATCH 0/6] Add Security Guest doc and check for capabilities cache validation
This series introduces the concept of a 'Secure Guest' feature which covers on s390 IBM Secure Execution and on x86 AMD Secure Encrypted Virtualization. Besides adding documentation for IBM Secure Execution it also adds checks during validation of the qemu capabilities cache. These checks per architecture can be performed for IBM Secure Execution on s390 and AMD Secure Encrypted Virtualization on AMD x86 CPUs (both checks implemented in this series). For s390 the verification consists of: - checking if /sys/firmware/uv is available: meaning the HW facility is available and the host OS supports it; - checking if the kernel cmdline contains 'prot_virt=1': meaning the host OS wants to use the feature. For AMD Secure Encrypted Virtualization the verification consists of: - checking if /sys/module/kvm_amd/parameters/sev contains the value '1': meaning SEV is enabled in the host kernel; - checking if the kernel cmdline contains 'mem_encrypt=on': meaning SME memory encryption feature on the host is enabled Whenever the availability of the feature does not match the secure guest flag in the cache then libvirt will re-build it in order to pick up the new set of capabilities available. Additionally, this series adds the same aforementioned checks to the virt-host-validate tool to facilitate the manual verification process for users. Boris Fiuczynski (2): tools: secure guest check on s390 in virt-host-validate tools: secure guest check for AMD in virt-host-validate Paulo de Rezende Pinatti (3): util: introduce a parser for kernel cmdline arguments qemu: check if s390 secure guest support is enabled qemu: check if AMD secure guest support is enabled Viktor Mihajlovski (1): docs: Describe protected virtualization guest setup docs/kbase.html.in | 3 + docs/kbase/launch_security_sev.rst | 9 +- docs/kbase/s390_protected_virt.rst | 189 +++++++++++++++++++++++++++++ src/libvirt_private.syms | 2 + src/qemu/qemu_capabilities.c | 83 +++++++++++++ src/util/virutil.c | 173 ++++++++++++++++++++++++++ src/util/virutil.h | 18 +++ tests/utiltest.c | 130 ++++++++++++++++++++ tools/virt-host-validate-common.c | 90 +++++++++++++- tools/virt-host-validate-common.h | 5 + tools/virt-host-validate-qemu.c | 4 + 11 files changed, 701 insertions(+), 5 deletions(-) create mode 100644 docs/kbase/s390_protected_virt.rst -- 2.25.1
From: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com> Introduce two utility functions to parse a kernel command line string according to the kernel code parsing rules in order to enable the caller to perform operations such as verifying whether certain argument=value combinations are present or retrieving an argument's value. Signed-off-by: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/libvirt_private.syms | 2 + src/util/virutil.c | 173 +++++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 18 ++++ tests/utiltest.c | 130 +++++++++++++++++++++++++++++ 4 files changed, 323 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 935ef7303b..25b22a3e3f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3428,6 +3428,8 @@ virHostGetDRMRenderNode; virHostHasIOMMU; virIndexToDiskName; virIsDevMapperDevice; +virKernelCmdlineGetValue; +virKernelCmdlineMatchParam; virMemoryLimitIsSet; virMemoryLimitTruncate; virMemoryMaxValue; diff --git a/src/util/virutil.c b/src/util/virutil.c index fb46501142..264aae8d01 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1725,6 +1725,179 @@ virHostGetDRMRenderNode(void) return ret; } + +#define VIR_IS_CMDLINE_SPACE(value) \ + (g_ascii_isspace(value) || (unsigned char) value == 160) + + +static bool virKernelArgsAreEqual(const char *arg1, + const char *arg2, + size_t n) +{ + size_t i; + + for (i = 0; i < n; i++) { + if ((arg1[i] == '-' && arg2[i] == '_') || + (arg1[i] == '_' && arg2[i] == '-') || (arg1[i] == arg2[i])) { + if (arg1[i] == '\0') + return true; + continue; + } + return false; + } + return true; +} + + +/* + * Parse the kernel cmdline and store the value of @arg in @val + * which can be NULL if @arg has no value or if it's not found. + * In addition, store in @next the address right after + * @arg=@value for possible further processing. + * + * @arg: kernel command line argument + * @cmdline: kernel command line string to be checked for @arg + * @val: pointer to hold retrieved value of @arg + * @next: pointer to hold address right after @arg=@val, will + * point to the string's end (NULL) in case @arg is not found + * + * Returns 0 if @arg is present in @cmdline, 1 otherwise + */ +int virKernelCmdlineGetValue(const char *arg, + const char *cmdline, + char **val, + const char **next) +{ + size_t i = 0, param_i; + size_t arg_len = strlen(arg); + bool is_quoted, match; + + *next = cmdline; + *val = NULL; + + while (cmdline[i] != '\0') { + /* remove leading spaces */ + while (VIR_IS_CMDLINE_SPACE(cmdline[i])) + i++; + if (cmdline[i] == '"') { + i++; + is_quoted = true; + } else { + is_quoted = false; + } + for (param_i = i; cmdline[param_i]; param_i++) { + if ((VIR_IS_CMDLINE_SPACE(cmdline[param_i]) && !is_quoted) || + cmdline[param_i] == '=') + break; + if (cmdline[param_i] == '"') + is_quoted = !is_quoted; + } + if (param_i-i == arg_len && virKernelArgsAreEqual(cmdline+i, arg, arg_len)) + match = true; + else + match = false; + /* arg has no value */ + if (cmdline[param_i] != '=') { + if (match) { + *next = cmdline+param_i; + return 0; + } + i = param_i; + continue; + } + param_i++; + if (cmdline[param_i] == '\0') + break; + + if (cmdline[param_i] == '"') { + param_i++; + is_quoted = !is_quoted; + } + i = param_i; + + for (; cmdline[param_i]; param_i++) { + if (VIR_IS_CMDLINE_SPACE(cmdline[param_i]) && !is_quoted) + break; + if (cmdline[param_i] == '"') + is_quoted = !is_quoted; + } + if (match) { + *next = cmdline+param_i; + if (cmdline[param_i-1] == '"') + *val = g_strndup(cmdline+i, param_i-i-1); + else + *val = g_strndup(cmdline+i, param_i-i); + return 0; + } + i = param_i; + } + *next = cmdline+i; + return 1; +} + + +#define VIR_CMDLINE_STR_CMP(kernel_val, caller_val, flags) \ + (((flags & VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ) && \ + STREQ(kernel_val, caller_val)) || ((flags & VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX) && \ + STRPREFIX(kernel_val, caller_val))) + + +/* + * Try to match the provided kernel cmdline string with the provided @arg + * and the list @values of possible values according to the matching strategy + * defined in @flags. Possible options include: + * - VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX: do a substring comparison of values + * (uses size of value provided as input) + * - VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ: do a strict string comparison + * - VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY: first positive match satifies search + * (in case of multiple argument occurrences) + * - VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST: use the result of last argument occurence + * (in case of multiple argument occurrences) + * + * + * @cmdline: kernel command line string to be checked for @arg + * @arg: kernel command line argument + * @values: array of possible values to match @arg + * @len_values: size of array, it can be 0 meaning a match will be positive if the + * argument has no value. + * @flags: flag mask defining the strategy for matching and comparing + * + * Returns true if a match is found, false otherwise + */ +bool virKernelCmdlineMatchParam(const char *cmdline, + const char *arg, + const char **values, + size_t len_values, + virKernelCmdlineFlags flags) +{ + bool match = false; + size_t i; + const char *next = cmdline; + g_autofree char *kval = NULL; + + while (next[0] != '\0') { + VIR_FREE(kval); + if (virKernelCmdlineGetValue(arg, next, &kval, &next) != 0) + break; + if (!kval) { + match = (len_values == 0) ? true : false; + } else { + match = false; + for (i = 0; i < len_values; i++) { + if (VIR_CMDLINE_STR_CMP(kval, values[i], flags)) { + match = true; + break; + } + } + } + if (match && (flags & VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY)) + break; + } + + return match; +} + + /* * Get a password from the console input stream. * The caller must free the returned password. diff --git a/src/util/virutil.h b/src/util/virutil.h index 49b4bf440f..1edb415fbe 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -147,6 +147,24 @@ bool virHostHasIOMMU(void); char *virHostGetDRMRenderNode(void) G_GNUC_NO_INLINE; +typedef enum { + VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX = 1, + VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ = 2, + VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY = 4, + VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST = 8, +} virKernelCmdlineFlags; + +int virKernelCmdlineGetValue(const char *arg, + const char *cmdline, + char **val, + const char **next); + +bool virKernelCmdlineMatchParam(const char *cmdline, + const char *arg, + const char **values, + size_t len_values, + virKernelCmdlineFlags flags); + /** * VIR_ASSIGN_IS_OVERFLOW: * @rvalue: value that is checked (evaluated twice) diff --git a/tests/utiltest.c b/tests/utiltest.c index 5ae04585cb..1edc6a728e 100644 --- a/tests/utiltest.c +++ b/tests/utiltest.c @@ -254,6 +254,134 @@ testOverflowCheckMacro(const void *data G_GNUC_UNUSED) } +struct testKernelCmdlineGetValueData +{ + const char *cmdline; + const char *arg; + int rc; + const char *val; + size_t next; +}; + +static struct testKernelCmdlineGetValueData kEntries[] = { + { "arg1 arg2 arg3=val1", "arg4", 1, NULL, 19 }, + { "arg1=val1 arg2 arg3=val3 arg4", "arg2", 0, NULL, 14 }, + { "arg1=val1 arg2 arg3=val3 arg4", "arg3", 0, "val3", 24 }, + { "arg1=val1 arg2 arg-3=val3 arg4", "arg_3", 0, "val3", 25 }, + { "arg1=val1 arg2 arg_3=val3 arg4", "arg-3", 0, "val3", 25 }, + { "arg1=val1 arg2 arg_3=val3 arg4", "arg_3", 0, "val3", 25 }, + { "arg1=val1 arg2 arg-3=val3 arg4", "arg-3", 0, "val3", 25 }, + { "arg1=val1 arg2=\"value with spaces\" arg3=val3", "arg2", 0, "value with spaces", 34 }, + { "arg1=val1 arg2=\"value with spaces\" arg3=val3", "arg3", 0, "val3", 44 }, + { "arg1=val1 \"arg2=value with spaces\" arg3=val3", "arg2", 0, "value with spaces", 34 }, + { "arg1=val1 \"arg2=value with spaces\" arg3=val3", "arg3", 0, "val3", 44 }, + { "arg1=val1 arg2=\"val\"ue arg3", "arg2", 0, "val\"ue", 22 }, + { "arg1=val1 arg2=\"val\"ue arg3\" escaped=val2\"", "arg3\" escaped", 0, "val2", 42 }, + { "arg1=val1 arg2longer=someval arg2=val2 arg3", "arg2", 0, "val2", 38 }, +}; + +static int +testKernelCmdlineGetValue(const void *data G_GNUC_UNUSED) +{ + int rc; + char *val = NULL; + const char *next; + size_t i; + + for (i = 0; i < G_N_ELEMENTS(kEntries); ++i) { + VIR_FREE(val); + + rc = virKernelCmdlineGetValue(kEntries[i].arg, kEntries[i].cmdline, + &val, &next); + + if (rc != kEntries[i].rc || STRNEQ_NULLABLE(val, kEntries[i].val) || + (next - kEntries[i].cmdline) != kEntries[i].next) { + VIR_TEST_DEBUG("\nKernel cmdline [%s]", kEntries[i].cmdline); + VIR_TEST_DEBUG("Kernel argument [%s]", kEntries[i].arg); + VIR_TEST_DEBUG("Expect rc [%d]", kEntries[i].rc); + VIR_TEST_DEBUG("Actual rc [%d]", rc); + VIR_TEST_DEBUG("Expect value [%s]", kEntries[i].val); + VIR_TEST_DEBUG("Actual value [%s]", val); + VIR_TEST_DEBUG("Expect next index [%lu]", kEntries[i].next); + VIR_TEST_DEBUG("Actual next index [%lu]", + (size_t)(next - kEntries[i].cmdline)); + + VIR_FREE(val); + + return -1; + } + } + + VIR_FREE(val); + + return 0; +} + + +struct testKernelCmdlineMatchData +{ + const char *cmdline; + const char *arg; + const char *values[2]; + virKernelCmdlineFlags flags; + bool result; +}; + +static struct testKernelCmdlineMatchData kMatchEntries[] = { + {"arg1 myarg=no arg2=val2 myarg=yes arg4=val4 myarg=no arg5", "myarg", {"1", "y"}, VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY | VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ, false }, + {"arg1 myarg=no arg2=val2 myarg=yes arg4=val4 myarg=no arg5", "myarg", {"on", "yes"}, VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY | VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ, true }, + {"arg1 myarg=no arg2=val2 myarg=yes arg4=val4 myarg=no arg5", "myarg", {"1", "y"}, VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY | VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX, true }, + {"arg1 myarg=no arg2=val2 myarg=yes arg4=val4 myarg=no arg5", "myarg", {"a", "b"}, VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY | VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX, false }, + {"arg1 myarg=no arg2=val2 myarg=yes arg4=val4 myarg=no arg5", "myarg", {"on", "yes"}, VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST | VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ, false}, + {"arg1 myarg=no arg2=val2 myarg=yes arg4=val4 myarg=no arg5", "myarg", {"1", "y"}, VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST | VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX, false}, + {"arg1 myarg=no arg2=val2 arg4=val4 myarg=yes arg5", "myarg", {"on", "yes"}, VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST | VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ, true }, + {"arg1 myarg=no arg2=val2 arg4=val4 myarg=yes arg5", "myarg", {"1", "y"}, VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST | VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX, true }, + {"arg1 myarg=no arg2=val2 arg4=val4 myarg arg5", "myarg", {NULL, NULL}, VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST, true }, + {"arg1 myarg arg2=val2 arg4=val4 myarg=yes arg5", "myarg", {NULL, NULL}, VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY, true }, + {"arg1 myarg arg2=val2 arg4=val4 myarg=yes arg5", "myarg", {NULL, NULL}, VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST, false }, +}; + + +static int +testKernelCmdlineMatchParam(const void *data G_GNUC_UNUSED) +{ + bool result; + size_t i, lenValues; + + for (i = 0; i < G_N_ELEMENTS(kMatchEntries); ++i) { + if (kMatchEntries[i].values[0] == NULL) + lenValues = 0; + else + lenValues = G_N_ELEMENTS(kMatchEntries[i].values); + + result = virKernelCmdlineMatchParam(kMatchEntries[i].cmdline, + kMatchEntries[i].arg, + kMatchEntries[i].values, + lenValues, + kMatchEntries[i].flags); + + if (result != kMatchEntries[i].result) { + VIR_TEST_DEBUG("\nKernel cmdline [%s]", kMatchEntries[i].cmdline); + VIR_TEST_DEBUG("Kernel argument [%s]", kMatchEntries[i].arg); + VIR_TEST_DEBUG("Kernel values [%s] [%s]", kMatchEntries[i].values[0], + kMatchEntries[i].values[1]); + if (kMatchEntries[i].flags & VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX) + VIR_TEST_DEBUG("Flag [VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX]"); + if (kMatchEntries[i].flags & VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ) + VIR_TEST_DEBUG("Flag [VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ]"); + if (kMatchEntries[i].flags & VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY) + VIR_TEST_DEBUG("Flag [VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY]"); + if (kMatchEntries[i].flags & VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST) + VIR_TEST_DEBUG("Flag [VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST]"); + VIR_TEST_DEBUG("Expect result [%d]", kMatchEntries[i].result); + VIR_TEST_DEBUG("Actual result [%d]", result); + + return -1; + } + } + + return 0; +} static int @@ -277,6 +405,8 @@ mymain(void) DO_TEST(ParseVersionString); DO_TEST(RoundValueToPowerOfTwo); DO_TEST(OverflowCheckMacro); + DO_TEST(KernelCmdlineGetValue); + DO_TEST(KernelCmdlineMatchParam); return result == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.25.1
On Mon, May 11, 2020 at 06:41:56PM +0200, Boris Fiuczynski wrote:
From: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com>
Introduce two utility functions to parse a kernel command line string according to the kernel code parsing rules in order to enable the caller to perform operations such as verifying whether certain argument=value combinations are present or retrieving an argument's value.
Signed-off-by: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/libvirt_private.syms | 2 + src/util/virutil.c | 173 +++++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 18 ++++ tests/utiltest.c | 130 +++++++++++++++++++++++++++++ 4 files changed, 323 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 935ef7303b..25b22a3e3f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3428,6 +3428,8 @@ virHostGetDRMRenderNode; virHostHasIOMMU; virIndexToDiskName; virIsDevMapperDevice; +virKernelCmdlineGetValue; +virKernelCmdlineMatchParam; virMemoryLimitIsSet; virMemoryLimitTruncate; virMemoryMaxValue; diff --git a/src/util/virutil.c b/src/util/virutil.c index fb46501142..264aae8d01 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1725,6 +1725,179 @@ virHostGetDRMRenderNode(void) return ret; }
+ +#define VIR_IS_CMDLINE_SPACE(value) \ + (g_ascii_isspace(value) || (unsigned char) value == 160)
Hmm, we're not checking the non-breaking space anywhere else in the code, so I think we'd be fine not checking it here either, so plain g_ascii_isspace() would suffice in the code
+ + +static bool virKernelArgsAreEqual(const char *arg1, + const char *arg2, + size_t n) +{ + size_t i; + + for (i = 0; i < n; i++) { + if ((arg1[i] == '-' && arg2[i] == '_') || + (arg1[i] == '_' && arg2[i] == '-') || (arg1[i] == arg2[i])) {
Because '-' and '_' are equal in the parameter syntax, rather than introducing another string equality function just because of this, we should normalize the inputs by replacing occurrences of one with the other and then simply do STREQ at the appropriate spot in the code
+ if (arg1[i] == '\0') + return true; + continue; + } + return false; + } + return true; +} + + +/* + * Parse the kernel cmdline and store the value of @arg in @val + * which can be NULL if @arg has no value or if it's not found. + * In addition, store in @next the address right after + * @arg=@value for possible further processing. + * + * @arg: kernel command line argument + * @cmdline: kernel command line string to be checked for @arg + * @val: pointer to hold retrieved value of @arg + * @next: pointer to hold address right after @arg=@val, will + * point to the string's end (NULL) in case @arg is not found + * + * Returns 0 if @arg is present in @cmdline, 1 otherwise + */ +int virKernelCmdlineGetValue(const char *arg, + const char *cmdline, + char **val, + const char **next) +{ + size_t i = 0, param_i;
in this specific case I think that naming the iteration variable param_i is more confusing than actually settling down with something like "offset" especially when you're doing arithmetics with it.
+ size_t arg_len = strlen(arg); + bool is_quoted, match;
1 declaration/definition per line please
+ + *next = cmdline; + *val = NULL; + + while (cmdline[i] != '\0') { + /* remove leading spaces */ + while (VIR_IS_CMDLINE_SPACE(cmdline[i])) + i++;
For ^this, we already have a primitive virSkipSpaces()
+ if (cmdline[i] == '"') { + i++; + is_quoted = true; + } else { + is_quoted = false; + } + for (param_i = i; cmdline[param_i]; param_i++) { + if ((VIR_IS_CMDLINE_SPACE(cmdline[param_i]) && !is_quoted) || + cmdline[param_i] == '=') + break; + if (cmdline[param_i] == '"') + is_quoted = !is_quoted; + } + if (param_i-i == arg_len && virKernelArgsAreEqual(cmdline+i, arg, arg_len))
We don't use Yoda conditions, so ^this should be arg_len == param_i - i
+ match = true; + else + match = false; + /* arg has no value */ + if (cmdline[param_i] != '=') { + if (match) { + *next = cmdline+param_i;
please use a space in between the operands and the operator
+ return 0; + } + i = param_i; + continue; + } + param_i++; + if (cmdline[param_i] == '\0') + break; + + if (cmdline[param_i] == '"') { + param_i++; + is_quoted = !is_quoted; + } + i = param_i; + + for (; cmdline[param_i]; param_i++) { + if (VIR_IS_CMDLINE_SPACE(cmdline[param_i]) && !is_quoted) + break; + if (cmdline[param_i] == '"') + is_quoted = !is_quoted; + } + if (match) { + *next = cmdline+param_i; + if (cmdline[param_i-1] == '"') + *val = g_strndup(cmdline+i, param_i-i-1); + else + *val = g_strndup(cmdline+i, param_i-i); + return 0; + } + i = param_i; + } + *next = cmdline+i; + return 1;
Anyhow, this function is IMO doing too much on its own - parsing tokens, matching the arguments, and extracting parameters and their values. All of that should be split into helpers to enhance readability. Unfortunately you can't use our existing tokenizer virStringSplit because of values containing spaces, so you'll have to write your own that takes that into account, it should return a token (parameter or parameter=value), then parse the returned token into key-value pair, match the key with the input key and return whatever is needed to be returned.
+} + + +#define VIR_CMDLINE_STR_CMP(kernel_val, caller_val, flags) \ + (((flags & VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ) && \ + STREQ(kernel_val, caller_val)) || ((flags & VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX) && \ + STRPREFIX(kernel_val, caller_val))) + + +/* + * Try to match the provided kernel cmdline string with the provided @arg + * and the list @values of possible values according to the matching strategy + * defined in @flags. Possible options include: + * - VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX: do a substring comparison of values + * (uses size of value provided as input) + * - VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ: do a strict string comparison + * - VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY: first positive match satifies search
Why are ^these constants needed? Especially the last two. Kernel is parsing the parameters sequentially, so the last one wins. We should match that behaviour when determining the value of a setting and ignore any previous occurrences of a parameter. Regards, Erik
On 5/15/20 4:19 PM, Erik Skultety wrote:
On Mon, May 11, 2020 at 06:41:56PM +0200, Boris Fiuczynski wrote:
From: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com>
Introduce two utility functions to parse a kernel command line string according to the kernel code parsing rules in order to enable the caller to perform operations such as verifying whether certain argument=value combinations are present or retrieving an argument's value.
Signed-off-by: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/libvirt_private.syms | 2 + src/util/virutil.c | 173 +++++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 18 ++++ tests/utiltest.c | 130 +++++++++++++++++++++++++++++ 4 files changed, 323 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 935ef7303b..25b22a3e3f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3428,6 +3428,8 @@ virHostGetDRMRenderNode; virHostHasIOMMU; virIndexToDiskName; virIsDevMapperDevice; +virKernelCmdlineGetValue; +virKernelCmdlineMatchParam; virMemoryLimitIsSet; virMemoryLimitTruncate; virMemoryMaxValue; diff --git a/src/util/virutil.c b/src/util/virutil.c index fb46501142..264aae8d01 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1725,6 +1725,179 @@ virHostGetDRMRenderNode(void) return ret; }
+ +#define VIR_IS_CMDLINE_SPACE(value) \ + (g_ascii_isspace(value) || (unsigned char) value == 160)
Hmm, we're not checking the non-breaking space anywhere else in the code, so I think we'd be fine not checking it here either, so plain g_ascii_isspace() would suffice in the code
Paulo wanted to make sure the function would always produce the same result as the kernel code itself and the kernel code recognizes character 160 as space. In case you'd like to check that, see lib/cmdline.c -> function next_arg -> isspace -> include/linux/ctype.h -> __ismask & _S -> lib/ctype.c has a table with the entries for _S, including the character 160 which stands for "non breaking space" character. Anyway since it seems an unlikely case I removed it as requested.
+ + +static bool virKernelArgsAreEqual(const char *arg1, + const char *arg2, + size_t n) +{ + size_t i; + + for (i = 0; i < n; i++) { + if ((arg1[i] == '-' && arg2[i] == '_') || + (arg1[i] == '_' && arg2[i] == '-') || (arg1[i] == arg2[i])) {
Because '-' and '_' are equal in the parameter syntax, rather than introducing another string equality function just because of this, we should normalize the inputs by replacing occurrences of one with the other and then simply do STREQ at the appropriate spot in the code
While reworking the parsing it has been changed.
+ if (arg1[i] == '\0') + return true; + continue; + } + return false; + } + return true; +} + + +/* + * Parse the kernel cmdline and store the value of @arg in @val + * which can be NULL if @arg has no value or if it's not found. + * In addition, store in @next the address right after + * @arg=@value for possible further processing. + * + * @arg: kernel command line argument + * @cmdline: kernel command line string to be checked for @arg + * @val: pointer to hold retrieved value of @arg + * @next: pointer to hold address right after @arg=@val, will + * point to the string's end (NULL) in case @arg is not found + * + * Returns 0 if @arg is present in @cmdline, 1 otherwise + */ +int virKernelCmdlineGetValue(const char *arg, + const char *cmdline, + char **val, + const char **next) +{ + size_t i = 0, param_i;
in this specific case I think that naming the iteration variable param_i is more confusing than actually settling down with something like "offset" especially when you're doing arithmetics with it.
Changed it.
+ size_t arg_len = strlen(arg); + bool is_quoted, match;
1 declaration/definition per line please
Changed it.
+ + *next = cmdline; + *val = NULL; + + while (cmdline[i] != '\0') { + /* remove leading spaces */ + while (VIR_IS_CMDLINE_SPACE(cmdline[i])) + i++;
For ^this, we already have a primitive virSkipSpaces()
Changed it.
+ if (cmdline[i] == '"') { + i++; + is_quoted = true; + } else { + is_quoted = false; + } + for (param_i = i; cmdline[param_i]; param_i++) { + if ((VIR_IS_CMDLINE_SPACE(cmdline[param_i]) && !is_quoted) || + cmdline[param_i] == '=') + break; + if (cmdline[param_i] == '"') + is_quoted = !is_quoted; + } + if (param_i-i == arg_len && virKernelArgsAreEqual(cmdline+i, arg, arg_len))
We don't use Yoda conditions, so ^this should be arg_len == param_i - i
Corrected
+ match = true; + else + match = false; + /* arg has no value */ + if (cmdline[param_i] != '=') { + if (match) { + *next = cmdline+param_i;
please use a space in between the operands and the operator
Corrected
+ return 0; + } + i = param_i; + continue; + } + param_i++; + if (cmdline[param_i] == '\0') + break; + + if (cmdline[param_i] == '"') { + param_i++; + is_quoted = !is_quoted; + } + i = param_i; + + for (; cmdline[param_i]; param_i++) { + if (VIR_IS_CMDLINE_SPACE(cmdline[param_i]) && !is_quoted) + break; + if (cmdline[param_i] == '"') + is_quoted = !is_quoted; + } + if (match) { + *next = cmdline+param_i; + if (cmdline[param_i-1] == '"') + *val = g_strndup(cmdline+i, param_i-i-1); + else + *val = g_strndup(cmdline+i, param_i-i); + return 0; + } + i = param_i; + } + *next = cmdline+i; + return 1;
Anyhow, this function is IMO doing too much on its own - parsing tokens, matching the arguments, and extracting parameters and their values. All of that should be split into helpers to enhance readability. Unfortunately you can't use our existing tokenizer virStringSplit because of values containing spaces, so you'll have to write your own that takes that into account, it should return a token (parameter or parameter=value), then parse the returned token into key-value pair, match the key with the input key and return whatever is needed to be returned.
v2 will have a reworked approach which is more like you outlined above.
+} + + +#define VIR_CMDLINE_STR_CMP(kernel_val, caller_val, flags) \ + (((flags & VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ) && \ + STREQ(kernel_val, caller_val)) || ((flags & VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX) && \ + STRPREFIX(kernel_val, caller_val))) + + +/* + * Try to match the provided kernel cmdline string with the provided @arg + * and the list @values of possible values according to the matching strategy + * defined in @flags. Possible options include: + * - VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX: do a substring comparison of values + * (uses size of value provided as input) + * - VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ: do a strict string comparison + * - VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY: first positive match satifies search
Why are ^these constants needed? Especially the last two. Kernel is parsing the parameters sequentially, so the last one wins. We should match that behaviour when determining the value of a setting and ignore any previous occurrences of a parameter.
Regards, Erik
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 15/05/20 16:19, Erik Skultety wrote:
On Mon, May 11, 2020 at 06:41:56PM +0200, Boris Fiuczynski wrote:
From: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com>
Introduce two utility functions to parse a kernel command line string according to the kernel code parsing rules in order to enable the caller to perform operations such as verifying whether certain argument=value combinations are present or retrieving an argument's value.
Signed-off-by: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/libvirt_private.syms | 2 + src/util/virutil.c | 173 +++++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 18 ++++ tests/utiltest.c | 130 +++++++++++++++++++++++++++++ 4 files changed, 323 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 935ef7303b..25b22a3e3f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3428,6 +3428,8 @@ virHostGetDRMRenderNode; virHostHasIOMMU; virIndexToDiskName; virIsDevMapperDevice; +virKernelCmdlineGetValue; +virKernelCmdlineMatchParam; virMemoryLimitIsSet; virMemoryLimitTruncate; virMemoryMaxValue; diff --git a/src/util/virutil.c b/src/util/virutil.c index fb46501142..264aae8d01 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1725,6 +1725,179 @@ virHostGetDRMRenderNode(void) return ret; }
+ +#define VIR_IS_CMDLINE_SPACE(value) \ + (g_ascii_isspace(value) || (unsigned char) value == 160)
Hmm, we're not checking the non-breaking space anywhere else in the code, so I think we'd be fine not checking it here either, so plain g_ascii_isspace() would suffice in the code
+ + +static bool virKernelArgsAreEqual(const char *arg1, + const char *arg2, + size_t n) +{ + size_t i; + + for (i = 0; i < n; i++) { + if ((arg1[i] == '-' && arg2[i] == '_') || + (arg1[i] == '_' && arg2[i] == '-') || (arg1[i] == arg2[i])) {
Because '-' and '_' are equal in the parameter syntax, rather than introducing another string equality function just because of this, we should normalize the inputs by replacing occurrences of one with the other and then simply do STREQ at the appropriate spot in the code
+ if (arg1[i] == '\0') + return true; + continue; + } + return false; + } + return true; +} + + +/* + * Parse the kernel cmdline and store the value of @arg in @val + * which can be NULL if @arg has no value or if it's not found. + * In addition, store in @next the address right after + * @arg=@value for possible further processing. + * + * @arg: kernel command line argument + * @cmdline: kernel command line string to be checked for @arg + * @val: pointer to hold retrieved value of @arg + * @next: pointer to hold address right after @arg=@val, will + * point to the string's end (NULL) in case @arg is not found + * + * Returns 0 if @arg is present in @cmdline, 1 otherwise + */ +int virKernelCmdlineGetValue(const char *arg, + const char *cmdline, + char **val, + const char **next) +{ + size_t i = 0, param_i;
in this specific case I think that naming the iteration variable param_i is more confusing than actually settling down with something like "offset" especially when you're doing arithmetics with it.
+ size_t arg_len = strlen(arg); + bool is_quoted, match;
1 declaration/definition per line please
+ + *next = cmdline; + *val = NULL; + + while (cmdline[i] != '\0') { + /* remove leading spaces */ + while (VIR_IS_CMDLINE_SPACE(cmdline[i])) + i++;
For ^this, we already have a primitive virSkipSpaces()
+ if (cmdline[i] == '"') { + i++; + is_quoted = true; + } else { + is_quoted = false; + } + for (param_i = i; cmdline[param_i]; param_i++) { + if ((VIR_IS_CMDLINE_SPACE(cmdline[param_i]) && !is_quoted) || + cmdline[param_i] == '=') + break; + if (cmdline[param_i] == '"') + is_quoted = !is_quoted; + } + if (param_i-i == arg_len && virKernelArgsAreEqual(cmdline+i, arg, arg_len))
We don't use Yoda conditions, so ^this should be arg_len == param_i - i
+ match = true; + else + match = false; + /* arg has no value */ + if (cmdline[param_i] != '=') { + if (match) { + *next = cmdline+param_i;
please use a space in between the operands and the operator
+ return 0; + } + i = param_i; + continue; + } + param_i++; + if (cmdline[param_i] == '\0') + break; + + if (cmdline[param_i] == '"') { + param_i++; + is_quoted = !is_quoted; + } + i = param_i; + + for (; cmdline[param_i]; param_i++) { + if (VIR_IS_CMDLINE_SPACE(cmdline[param_i]) && !is_quoted) + break; + if (cmdline[param_i] == '"') + is_quoted = !is_quoted; + } + if (match) { + *next = cmdline+param_i; + if (cmdline[param_i-1] == '"') + *val = g_strndup(cmdline+i, param_i-i-1); + else + *val = g_strndup(cmdline+i, param_i-i); + return 0; + } + i = param_i; + } + *next = cmdline+i; + return 1;
Anyhow, this function is IMO doing too much on its own - parsing tokens, matching the arguments, and extracting parameters and their values. All of that should be split into helpers to enhance readability. Unfortunately you can't use our existing tokenizer virStringSplit because of values containing spaces, so you'll have to write your own that takes that into account, it should return a token (parameter or parameter=value), then parse the returned token into key-value pair, match the key with the input key and return whatever is needed to be returned.
+} + + +#define VIR_CMDLINE_STR_CMP(kernel_val, caller_val, flags) \ + (((flags & VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ) && \ + STREQ(kernel_val, caller_val)) || ((flags & VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX) && \ + STRPREFIX(kernel_val, caller_val))) + + +/* + * Try to match the provided kernel cmdline string with the provided @arg + * and the list @values of possible values according to the matching strategy + * defined in @flags. Possible options include: + * - VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX: do a substring comparison of values + * (uses size of value provided as input) + * - VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ: do a strict string comparison + * - VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY: first positive match satifies search
Why are ^these constants needed? Especially the last two. Kernel is parsing the parameters sequentially, so the last one wins. We should match that behaviour when determining the value of a setting and ignore any previous occurrences of a parameter.
It's not always the case that the last one wins. In the case of the prot_virt parameter for example as soon as a positive value (prot_virt=1 or prot_virt=y) shows up in the kernel cmdline the feature will be enabled, even if the last entry is negative. In order to handle the two possible behaviors we introduced the *_SEARCH flags SEARCH_STICKY (prot_virt behavior) and SEARCH_LAST (usual behavior, last one wins). As to the *_CMP flags, prot_virt again accepts multiple values and does a prefix-based check (see arch/s390/kernel/uv.c -> prot_virt_setup -> kstrtobool) so we introduced CMP_PREFIX to handle that, while CMP_EQ is for the usual case when the whole value is considered.
Regards, Erik
-- Best regards, Paulo de Rezende Pinatti
From: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com> This patch introduces a common function to verify if the availability of the so-called Secure Guest feature on the host has changed in order to invalidate the qemu capabilities cache. It can be used as an entry point for verification on different architectures. For s390 the verification consists of: - checking if /sys/firmware/uv is available: meaning the HW facility is available and the host OS supports it; - checking if the kernel cmdline contains 'prot_virt=1': meaning the host OS wants to use the feature. Whenever the availability of the feature does not match the secure guest flag in the cache then libvirt will re-build it in order to pick up the new set of capabilities available. Signed-off-by: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com> Tested-by: Viktor Mihajlovski <mihajlov@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/qemu/qemu_capabilities.c | 56 ++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 2c6e36685e..2874bb1e7c 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -23,6 +23,7 @@ #include "qemu_capabilities.h" #include "viralloc.h" +#include "virarch.h" #include "vircrypto.h" #include "virlog.h" #include "virerror.h" @@ -654,6 +655,7 @@ struct _virQEMUCaps { virObject parent; bool kvmSupportsNesting; + bool kvmSupportsSecureGuest; char *binary; time_t ctime; @@ -1864,6 +1866,7 @@ virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr qemuCaps) ret->invalidation = qemuCaps->invalidation; ret->kvmSupportsNesting = qemuCaps->kvmSupportsNesting; + ret->kvmSupportsSecureGuest = qemuCaps->kvmSupportsSecureGuest; ret->ctime = qemuCaps->ctime; @@ -4302,6 +4305,9 @@ virQEMUCapsLoadCache(virArch hostArch, if (virXPathBoolean("boolean(./kvmSupportsNesting)", ctxt) > 0) qemuCaps->kvmSupportsNesting = true; + if (virXPathBoolean("boolean(./kvmSupportsSecureGuest)", ctxt) > 0) + qemuCaps->kvmSupportsSecureGuest = true; + ret = 0; cleanup: VIR_FREE(str); @@ -4535,6 +4541,9 @@ virQEMUCapsFormatCache(virQEMUCapsPtr qemuCaps) if (qemuCaps->kvmSupportsNesting) virBufferAddLit(&buf, "<kvmSupportsNesting/>\n"); + if (qemuCaps->kvmSupportsSecureGuest) + virBufferAddLit(&buf, "<kvmSupportsSecureGuest/>\n"); + virBufferAdjustIndent(&buf, -2); virBufferAddLit(&buf, "</qemuCaps>\n"); @@ -4572,6 +4581,44 @@ virQEMUCapsSaveFile(void *data, } +/* + * Check whether IBM Secure Execution (S390) is enabled + */ +static bool +virQEMUCapsKVMSupportsSecureGuestS390(void) +{ + + g_autofree char *cmdline = NULL; + static const char *kValues[] = {"y", "Y", "1"}; + + if (!virFileIsDir("/sys/firmware/uv")) + return false; + if (virFileReadValueString(&cmdline, "/proc/cmdline") < 0) + return false; + if (virKernelCmdlineMatchParam(cmdline, "prot_virt", kValues, + G_N_ELEMENTS(kValues), + VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY | + VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX)) + return true; + return false; +} + + +/* + * Check whether the secure guest functionality is enabled. + * See the specific architecture function for details on the verifications made. + */ +static bool +virQEMUCapsKVMSupportsSecureGuest(void) +{ + virArch arch = virArchFromHost(); + + if (ARCH_IS_S390(arch)) + return virQEMUCapsKVMSupportsSecureGuestS390(); + return false; +} + + /* Check the kernel module parameters 'nested' file to determine if enabled * * Intel: 'kvm_intel' uses 'Y' @@ -4750,6 +4797,13 @@ virQEMUCapsIsValid(void *data, qemuCaps->binary, qemuCaps->kvmSupportsNesting); return false; } + + if (virQEMUCapsKVMSupportsSecureGuest() != qemuCaps->kvmSupportsSecureGuest) { + VIR_DEBUG("Outdated capabilities for '%s': kvm kernel secure guest " + "value changed from %d", + qemuCaps->binary, qemuCaps->kvmSupportsSecureGuest); + return false; + } } return true; @@ -5236,6 +5290,8 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch, qemuCaps->kernelVersion = g_strdup(kernelVersion); qemuCaps->kvmSupportsNesting = virQEMUCapsKVMSupportsNesting(); + + qemuCaps->kvmSupportsSecureGuest = virQEMUCapsKVMSupportsSecureGuest(); } return qemuCaps; -- 2.25.1
From: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com> Implement secure guest check for AMD SEV (Secure Encrypted Virtualization) in order to invalidate the qemu capabilities cache in case the availability of the feature changed. For AMD SEV the verification consists of: - checking if /sys/module/kvm_amd/parameters/sev contains the value '1': meaning SEV is enabled in the host kernel; - checking if the kernel cmdline contains 'mem_encrypt=on': meaning SME memory encryption feature on the host is enabled Signed-off-by: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- docs/kbase/launch_security_sev.rst | 2 +- src/qemu/qemu_capabilities.c | 27 +++++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/docs/kbase/launch_security_sev.rst b/docs/kbase/launch_security_sev.rst index 65f258587d..fa602c7432 100644 --- a/docs/kbase/launch_security_sev.rst +++ b/docs/kbase/launch_security_sev.rst @@ -109,7 +109,7 @@ following: </features> </domainCapabilities> -Note that if libvirt was already installed and libvirtd running before +Note that if libvirt (<6.4.0) was already installed and libvirtd running before enabling SEV in the kernel followed by the host reboot you need to force libvirtd to re-probe both the host and QEMU capabilities. First stop libvirtd: diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 2874bb1e7c..6cf926d52d 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4604,6 +4604,31 @@ virQEMUCapsKVMSupportsSecureGuestS390(void) } +/* + * Check whether AMD Secure Encrypted Virtualization (x86) is enabled + */ +static bool +virQEMUCapsKVMSupportsSecureGuestAMD(void) +{ + g_autofree char *cmdline = NULL; + g_autofree char *modValue = NULL; + static const char *kValues[] = {"on"}; + + if (virFileReadValueString(&modValue, "/sys/module/kvm_amd/parameters/sev") < 0) + return false; + if (modValue[0] != '1') + return false; + if (virFileReadValueString(&cmdline, "/proc/cmdline") < 0) + return false; + if (virKernelCmdlineMatchParam(cmdline, "mem_encrypt", kValues, + G_N_ELEMENTS(kValues), + VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST | + VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ)) + return true; + return false; +} + + /* * Check whether the secure guest functionality is enabled. * See the specific architecture function for details on the verifications made. @@ -4615,6 +4640,8 @@ virQEMUCapsKVMSupportsSecureGuest(void) if (ARCH_IS_S390(arch)) return virQEMUCapsKVMSupportsSecureGuestS390(); + if (ARCH_IS_X86(arch)) + return virQEMUCapsKVMSupportsSecureGuestAMD(); return false; } -- 2.25.1
Thanks for the patch Paulo, Few comments. On 5/11/20 11:41 AM, Boris Fiuczynski wrote:
From: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com>
Implement secure guest check for AMD SEV (Secure Encrypted Virtualization) in order to invalidate the qemu capabilities cache in case the availability of the feature changed.
For AMD SEV the verification consists of: - checking if /sys/module/kvm_amd/parameters/sev contains the value '1': meaning SEV is enabled in the host kernel; - checking if the kernel cmdline contains 'mem_encrypt=on': meaning SME memory encryption feature on the host is enabled
In addition to the kernel module parameter, we probably also need to check whether QEMU supports the feature. e.g, what if user has newer kernel but older qemu. e.g kernel > 4.18 but Qemu < 2.12. To check the SEV feature support, we should verify the following conditions: 1) check kernel module parameter is set 2) check if /dev/sev device exist (aka firmware is detected) 3) Check if Qemu supports SEV feature (maybe we can detect the existence of the query-sev QMP command or detect Qemu version >= 2.12) thanks
Signed-off-by: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- docs/kbase/launch_security_sev.rst | 2 +- src/qemu/qemu_capabilities.c | 27 +++++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/docs/kbase/launch_security_sev.rst b/docs/kbase/launch_security_sev.rst index 65f258587d..fa602c7432 100644 --- a/docs/kbase/launch_security_sev.rst +++ b/docs/kbase/launch_security_sev.rst @@ -109,7 +109,7 @@ following: </features> </domainCapabilities>
-Note that if libvirt was already installed and libvirtd running before +Note that if libvirt (<6.4.0) was already installed and libvirtd running before enabling SEV in the kernel followed by the host reboot you need to force libvirtd to re-probe both the host and QEMU capabilities. First stop libvirtd: diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 2874bb1e7c..6cf926d52d 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4604,6 +4604,31 @@ virQEMUCapsKVMSupportsSecureGuestS390(void) }
+/* + * Check whether AMD Secure Encrypted Virtualization (x86) is enabled + */ +static bool +virQEMUCapsKVMSupportsSecureGuestAMD(void) +{ + g_autofree char *cmdline = NULL; + g_autofree char *modValue = NULL; + static const char *kValues[] = {"on"}; + + if (virFileReadValueString(&modValue, "/sys/module/kvm_amd/parameters/sev") < 0) + return false; + if (modValue[0] != '1') + return false; + if (virFileReadValueString(&cmdline, "/proc/cmdline") < 0) + return false; + if (virKernelCmdlineMatchParam(cmdline, "mem_encrypt", kValues, + G_N_ELEMENTS(kValues), + VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST | + VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ)) + return true; + return false; +} + + /* * Check whether the secure guest functionality is enabled. * See the specific architecture function for details on the verifications made. @@ -4615,6 +4640,8 @@ virQEMUCapsKVMSupportsSecureGuest(void)
if (ARCH_IS_S390(arch)) return virQEMUCapsKVMSupportsSecureGuestS390(); + if (ARCH_IS_X86(arch)) + return virQEMUCapsKVMSupportsSecureGuestAMD(); return false; }
On 5/11/20 9:40 PM, Brijesh Singh wrote:
Thanks for the patch Paulo, Few comments.
On 5/11/20 11:41 AM, Boris Fiuczynski wrote:
From: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com>
Implement secure guest check for AMD SEV (Secure Encrypted Virtualization) in order to invalidate the qemu capabilities cache in case the availability of the feature changed.
For AMD SEV the verification consists of: - checking if /sys/module/kvm_amd/parameters/sev contains the value '1': meaning SEV is enabled in the host kernel; - checking if the kernel cmdline contains 'mem_encrypt=on': meaning SME memory encryption feature on the host is enabled
In addition to the kernel module parameter, we probably also need to check whether QEMU supports the feature. e.g, what if user has newer kernel but older qemu. e.g kernel > 4.18 but Qemu < 2.12. To check the SEV feature support, we should verify the following conditions:
1) check kernel module parameter is set Paulo implemented the checks following the documentation in docs/kbase/launch_security_sev.rst. The check for the module parameter sev is included. Is the check for the kernel cmdline parameter "mem_encrypt" not necessary? Or would that be covered by your suggested check in 2)?
2) check if /dev/sev device exist (aka firmware is detected)
This seems reasonable. Shouldn't it have been documented in docs/kbase/launch_security_sev.rst?
3) Check if Qemu supports SEV feature (maybe we can detect the existence of the query-sev QMP command or detect Qemu version >= 2.12)
The idea is to check the host capabilities to detect if qemus capabilities need to be reprobed. Therefore this would be a result if checks 1+2 change but it would not be a cache invalidation trigger.
thanks
Signed-off-by: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- docs/kbase/launch_security_sev.rst | 2 +- src/qemu/qemu_capabilities.c | 27 +++++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/docs/kbase/launch_security_sev.rst b/docs/kbase/launch_security_sev.rst index 65f258587d..fa602c7432 100644 --- a/docs/kbase/launch_security_sev.rst +++ b/docs/kbase/launch_security_sev.rst @@ -109,7 +109,7 @@ following: </features> </domainCapabilities>
-Note that if libvirt was already installed and libvirtd running before +Note that if libvirt (<6.4.0) was already installed and libvirtd running before enabling SEV in the kernel followed by the host reboot you need to force libvirtd to re-probe both the host and QEMU capabilities. First stop libvirtd: diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 2874bb1e7c..6cf926d52d 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4604,6 +4604,31 @@ virQEMUCapsKVMSupportsSecureGuestS390(void) }
+/* + * Check whether AMD Secure Encrypted Virtualization (x86) is enabled + */ +static bool +virQEMUCapsKVMSupportsSecureGuestAMD(void) +{ + g_autofree char *cmdline = NULL; + g_autofree char *modValue = NULL; + static const char *kValues[] = {"on"}; + + if (virFileReadValueString(&modValue, "/sys/module/kvm_amd/parameters/sev") < 0) + return false; + if (modValue[0] != '1') + return false; + if (virFileReadValueString(&cmdline, "/proc/cmdline") < 0) + return false; + if (virKernelCmdlineMatchParam(cmdline, "mem_encrypt", kValues, + G_N_ELEMENTS(kValues), + VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST | + VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ)) + return true; + return false; +} + + /* * Check whether the secure guest functionality is enabled. * See the specific architecture function for details on the verifications made. @@ -4615,6 +4640,8 @@ virQEMUCapsKVMSupportsSecureGuest(void)
if (ARCH_IS_S390(arch)) return virQEMUCapsKVMSupportsSecureGuestS390(); + if (ARCH_IS_X86(arch)) + return virQEMUCapsKVMSupportsSecureGuestAMD(); return false; }
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Tue, May 12, 2020 at 09:08:38AM +0200, Boris Fiuczynski wrote:
On 5/11/20 9:40 PM, Brijesh Singh wrote:
Thanks for the patch Paulo, Few comments.
On 5/11/20 11:41 AM, Boris Fiuczynski wrote:
From: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com>
Implement secure guest check for AMD SEV (Secure Encrypted Virtualization) in order to invalidate the qemu capabilities cache in case the availability of the feature changed.
For AMD SEV the verification consists of: - checking if /sys/module/kvm_amd/parameters/sev contains the value '1': meaning SEV is enabled in the host kernel; - checking if the kernel cmdline contains 'mem_encrypt=on': meaning SME memory encryption feature on the host is enabled
In addition to the kernel module parameter, we probably also need to check whether QEMU supports the feature. e.g, what if user has newer kernel but older qemu. e.g kernel > 4.18 but Qemu < 2.12. To check the SEV feature support, we should verify the following conditions:
1) check kernel module parameter is set Paulo implemented the checks following the documentation in docs/kbase/launch_security_sev.rst. The check for the module parameter sev is included. Is the check for the kernel cmdline parameter "mem_encrypt" not necessary? Or would that be covered by your suggested check in 2)?
Brijesh correct me here please, but IIRC having mem_encrypt set is merely a good security recommendation but is orthogonal with kvm.amd_sev feature, IOW SEV should work without SME. If my memory serves well here, we don't need and also should not parse the kernel cmdline in this case.
2) check if /dev/sev device exist (aka firmware is detected)
This seems reasonable. Shouldn't it have been documented in docs/kbase/launch_security_sev.rst?
Sure, we can add a mention about this. Although, doesn't 1 imply 2? IOW can you have the kernel module parameter set to 1 and yet kernel doesn't expose the /dev/sev node?
3) Check if Qemu supports SEV feature (maybe we can detect the existence of the query-sev QMP command or detect Qemu version >= 2.12)
^This is already achieved by qemuMonitorJSONGetSEVCapabilities
The idea is to check the host capabilities to detect if qemus capabilities need to be reprobed. Therefore this would be a result if checks 1+2 change but it would not be a cache invalidation trigger.
I agree in the sense that the SEV support that is currently being reported in QEMU capabilities wasn't a good choice because it reports only platform data which are constant to the host and have nothing to do with QEMU. It would be okay to just say <sev supported="yes|no"/> in qemu capabilities and report the rest in host capabilities. I haven't added this info into host capabilities when I realized the mistake because I didn't want to duplicate the platform info on 2 places. For the IBM secure guest feature, it makes total sense to report support within host capabilities, I'm just not sure whether we should do the same for SEV and try really hard to "fix" the mess. Regards, -- Erik Skultety
On 5/12/20 5:32 PM, Erik Skultety wrote: I leave 1) and 2) to the AMD experts... :-)
3) Check if Qemu supports SEV feature (maybe we can detect the existence of the query-sev QMP command or detect Qemu version >= 2.12) ^This is already achieved by qemuMonitorJSONGetSEVCapabilities
The idea is to check the host capabilities to detect if qemus capabilities need to be reprobed. Therefore this would be a result if checks 1+2 change but it would not be a cache invalidation trigger. I agree in the sense that the SEV support that is currently being reported in QEMU capabilities wasn't a good choice because it reports only platform data which are constant to the host and have nothing to do with QEMU. It would be okay to just say <sev supported="yes|no"/> in qemu capabilities and report the rest in host capabilities. I haven't added this info into host capabilities when I realized the mistake because I didn't want to duplicate the platform info on 2 places. For the IBM secure guest feature, it makes total sense to report support within host capabilities, I'm just not sure whether we should do the same for SEV and try really hard to "fix" the mess.
I am not sure I understand the "mess" correctly. The idea is to prevent the cached qemu capabilities becoming stale which Daniel PB has called earlier a bug in the caching algorithm. This can occur if the kernel or firmware configuration change. To find out these situations we try to implement qemu capability cache invalidation triggers. The monitor method you mentioned is called during a (re-)probe and for IBM Secure Execution the host CPU model is also updated during a (re-)probe. Wouldn't that clean up the "mess"? -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 5/12/20 10:32 AM, Erik Skultety wrote:
On Tue, May 12, 2020 at 09:08:38AM +0200, Boris Fiuczynski wrote:
On 5/11/20 9:40 PM, Brijesh Singh wrote:
Thanks for the patch Paulo, Few comments.
On 5/11/20 11:41 AM, Boris Fiuczynski wrote:
From: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com>
Implement secure guest check for AMD SEV (Secure Encrypted Virtualization) in order to invalidate the qemu capabilities cache in case the availability of the feature changed.
For AMD SEV the verification consists of: - checking if /sys/module/kvm_amd/parameters/sev contains the value '1': meaning SEV is enabled in the host kernel; - checking if the kernel cmdline contains 'mem_encrypt=on': meaning SME memory encryption feature on the host is enabled
In addition to the kernel module parameter, we probably also need to check whether QEMU supports the feature. e.g, what if user has newer kernel but older qemu. e.g kernel > 4.18 but Qemu < 2.12. To check the SEV feature support, we should verify the following conditions:
1) check kernel module parameter is set Paulo implemented the checks following the documentation in docs/kbase/launch_security_sev.rst. The check for the module parameter sev is included. Is the check for the kernel cmdline parameter "mem_encrypt" not necessary? Or would that be covered by your suggested check in 2)? Brijesh correct me here please, but IIRC having mem_encrypt set is merely a good security recommendation but is orthogonal with kvm.amd_sev feature, IOW SEV should work without SME. If my memory serves well here, we don't need and also should not parse the kernel cmdline in this case.
Yes, that is correct. mem_encrypt=on is not required for the SEV to work. The mem_encrypt=on option is meant to enable the SME feature on the host machine. In addition to the guest, if customer wants to protect the Hypervsior memory from physical attacks then they can use SME or TSME. The SME can be enabled using mem_encrypt=on, whereas the TSME can be enabled in the BIOS and does not require any kernel changes. AMD is mostly recommending TSME to protect against the physical attacks.
2) check if /dev/sev device exist (aka firmware is detected) This seems reasonable. Shouldn't it have been documented in docs/kbase/launch_security_sev.rst? Sure, we can add a mention about this. Although, doesn't 1 imply 2? IOW can you have the kernel module parameter set to 1 and yet kernel doesn't expose the /dev/sev node?
Currently, 1 does not imply 2, KVM driver does not initialize the firmware during the feature probe (i.e does not access the /dev/sev). The firmware initialization is delayed until the first guest launch. So only sane way to know whether firmware is been detected is check the existence of the /dev/sev or issue a query-sev command . The query-sev command will send the platform_status request to the firmware, if the firmware is not ready then this command will fail.
3) Check if Qemu supports SEV feature (maybe we can detect the existence of the query-sev QMP command or detect Qemu version >= 2.12) ^This is already achieved by qemuMonitorJSONGetSEVCapabilities
The idea is to check the host capabilities to detect if qemus capabilities need to be reprobed. Therefore this would be a result if checks 1+2 change but it would not be a cache invalidation trigger. I agree in the sense that the SEV support that is currently being reported in QEMU capabilities wasn't a good choice because it reports only platform data which are constant to the host and have nothing to do with QEMU. It would be okay to just say <sev supported="yes|no"/> in qemu capabilities and report the rest in host capabilities. I haven't added this info into host capabilities when I realized the mistake because I didn't want to duplicate the platform info on 2 places. For the IBM secure guest feature, it makes total sense to report support within host capabilities, I'm just not sure whether we should do the same for SEV and try really hard to "fix" the mess.
Regards,
2) check if /dev/sev device exist (aka firmware is detected) This seems reasonable. Shouldn't it have been documented in docs/kbase/launch_security_sev.rst? Sure, we can add a mention about this. Although, doesn't 1 imply 2? IOW can you have the kernel module parameter set to 1 and yet kernel doesn't expose the /dev/sev node?
Currently, 1 does not imply 2, KVM driver does not initialize the firmware during the feature probe (i.e does not access the /dev/sev). The firmware initialization is delayed until the first guest launch. So only sane way to know whether firmware is been detected is check the existence of the /dev/sev or issue a query-sev command . The query-sev command will send the platform_status request to the firmware, if the firmware is not ready then this command will fail.
I see. Can query-sev fail or return that it's disabled, aka {"enabled": false,...} in the SevInfo QMP response, but at the same time succeed in returning the platform capabilities via query-sev-capabilities? I'm asking, because libvirt only issues the latter to fill in the QEMU capabilities structure. -- Erik Skultety
On 5/13/20 1:21 AM, Erik Skultety wrote:
2) check if /dev/sev device exist (aka firmware is detected) This seems reasonable. Shouldn't it have been documented in docs/kbase/launch_security_sev.rst? Sure, we can add a mention about this. Although, doesn't 1 imply 2? IOW can you have the kernel module parameter set to 1 and yet kernel doesn't expose the /dev/sev node?
Currently, 1 does not imply 2, KVM driver does not initialize the firmware during the feature probe (i.e does not access the /dev/sev). The firmware initialization is delayed until the first guest launch. So only sane way to know whether firmware is been detected is check the existence of the /dev/sev or issue a query-sev command . The query-sev command will send the platform_status request to the firmware, if the firmware is not ready then this command will fail. I see. Can query-sev fail or return that it's disabled, aka {"enabled": false,...} in the SevInfo QMP response, but at the same time succeed in returning the platform capabilities via query-sev-capabilities? I'm asking, because libvirt only issues the latter to fill in the QEMU capabilities structure.
Just looked at qemu code, If /dev/sev does not exist then query-sev-capabilities should fail, and if SEV is not enabled in the guest then query-sev should returns false. So, basically what libvirt is doing correct, it should be using query-sev-capabilities to populate QEMU capabilities. It was my bad, I should have mentioned the query-sev-capabilities and not the query-sev check. thanks
On Wed, May 13, 2020 at 11:26:52PM -0500, Brijesh Singh wrote:
On 5/13/20 1:21 AM, Erik Skultety wrote:
2) check if /dev/sev device exist (aka firmware is detected) This seems reasonable. Shouldn't it have been documented in docs/kbase/launch_security_sev.rst? Sure, we can add a mention about this. Although, doesn't 1 imply 2? IOW can you have the kernel module parameter set to 1 and yet kernel doesn't expose the /dev/sev node?
Currently, 1 does not imply 2, KVM driver does not initialize the firmware during the feature probe (i.e does not access the /dev/sev). The firmware initialization is delayed until the first guest launch. So only sane way to know whether firmware is been detected is check the existence of the /dev/sev or issue a query-sev command . The query-sev command will send the platform_status request to the firmware, if the firmware is not ready then this command will fail. I see. Can query-sev fail or return that it's disabled, aka {"enabled": false,...} in the SevInfo QMP response, but at the same time succeed in returning the platform capabilities via query-sev-capabilities? I'm asking, because libvirt only issues the latter to fill in the QEMU capabilities structure.
Just looked at qemu code, If /dev/sev does not exist then query-sev-capabilities should fail, and if SEV is not enabled in the guest then query-sev should returns false. So, basically what libvirt is doing correct, it should be using query-sev-capabilities to populate QEMU capabilities. It was my bad, I should have mentioned the query-sev-capabilities and not the query-sev check.
Perfect, now it makes much more sense to me after quite a period not closely following this feature, thanks Brijesh. Boris, I'll start with the review today, but for the IBM part, I'm not sure I can get my hands on the necessary s390 HW (I'm also not familiar with s390), so I can do code-only review here, but at least for the AMD bits I do have access to the necessary HW and apart from scanning the filesystem (or parsing the kernel cmdline) the outcome is the same, so I'll do some mirroring in the review process. Regards, -- Erik Skultety
On 5/12/20 7:52 PM, Brijesh Singh wrote:
On 5/12/20 10:32 AM, Erik Skultety wrote:
On Tue, May 12, 2020 at 09:08:38AM +0200, Boris Fiuczynski wrote:
On 5/11/20 9:40 PM, Brijesh Singh wrote:
Thanks for the patch Paulo, Few comments.
On 5/11/20 11:41 AM, Boris Fiuczynski wrote:
From: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com>
Implement secure guest check for AMD SEV (Secure Encrypted Virtualization) in order to invalidate the qemu capabilities cache in case the availability of the feature changed.
For AMD SEV the verification consists of: - checking if /sys/module/kvm_amd/parameters/sev contains the value '1': meaning SEV is enabled in the host kernel; - checking if the kernel cmdline contains 'mem_encrypt=on': meaning SME memory encryption feature on the host is enabled
In addition to the kernel module parameter, we probably also need to check whether QEMU supports the feature. e.g, what if user has newer kernel but older qemu. e.g kernel > 4.18 but Qemu < 2.12. To check the SEV feature support, we should verify the following conditions:
1) check kernel module parameter is set Paulo implemented the checks following the documentation in docs/kbase/launch_security_sev.rst. The check for the module parameter sev is included. Is the check for the kernel cmdline parameter "mem_encrypt" not necessary? Or would that be covered by your suggested check in 2)? Brijesh correct me here please, but IIRC having mem_encrypt set is merely a good security recommendation but is orthogonal with kvm.amd_sev feature, IOW SEV should work without SME. If my memory serves well here, we don't need and also should not parse the kernel cmdline in this case.
Yes, that is correct. mem_encrypt=on is not required for the SEV to work. The mem_encrypt=on option is meant to enable the SME feature on the host machine. In addition to the guest, if customer wants to protect the Hypervsior memory from physical attacks then they can use SME or TSME. The SME can be enabled using mem_encrypt=on, whereas the TSME can be enabled in the BIOS and does not require any kernel changes. AMD is mostly recommending TSME to protect against the physical attacks.
2) check if /dev/sev device exist (aka firmware is detected) This seems reasonable. Shouldn't it have been documented in docs/kbase/launch_security_sev.rst? Sure, we can add a mention about this. Although, doesn't 1 imply 2? IOW can you have the kernel module parameter set to 1 and yet kernel doesn't expose the /dev/sev node?
Currently, 1 does not imply 2, KVM driver does not initialize the firmware during the feature probe (i.e does not access the /dev/sev). The firmware initialization is delayed until the first guest launch. So only sane way to know whether firmware is been detected is check the existence of the /dev/sev or issue a query-sev command . The query-sev command will send the platform_status request to the firmware, if the firmware is not ready then this command will fail.
Just to summarize the checks you want implemented for AMD SEV support: 1) check kernel module parameter is set (as already implemented) 2) check if /dev/sev device exist Please note that these checks will also go into virt-host-validate in patch 5.
3) Check if Qemu supports SEV feature (maybe we can detect the existence of the query-sev QMP command or detect Qemu version >= 2.12) ^This is already achieved by qemuMonitorJSONGetSEVCapabilities
The idea is to check the host capabilities to detect if qemus capabilities need to be reprobed. Therefore this would be a result if checks 1+2 change but it would not be a cache invalidation trigger. I agree in the sense that the SEV support that is currently being reported in QEMU capabilities wasn't a good choice because it reports only platform data which are constant to the host and have nothing to do with QEMU. It would be okay to just say <sev supported="yes|no"/> in qemu capabilities and report the rest in host capabilities. I haven't added this info into host capabilities when I realized the mistake because I didn't want to duplicate the platform info on 2 places. For the IBM secure guest feature, it makes total sense to report support within host capabilities, I'm just not sure whether we should do the same for SEV and try really hard to "fix" the mess.
Regards,
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 5/13/20 5:40 AM, Boris Fiuczynski wrote:
On 5/12/20 7:52 PM, Brijesh Singh wrote:
On 5/12/20 10:32 AM, Erik Skultety wrote:
On Tue, May 12, 2020 at 09:08:38AM +0200, Boris Fiuczynski wrote:
Thanks for the patch Paulo, Few comments.
On 5/11/20 11:41 AM, Boris Fiuczynski wrote:
From: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com>
Implement secure guest check for AMD SEV (Secure Encrypted Virtualization) in order to invalidate the qemu capabilities cache in case the availability of the feature changed.
For AMD SEV the verification consists of: - checking if /sys/module/kvm_amd/parameters/sev contains the value '1': meaning SEV is enabled in the host kernel; - checking if the kernel cmdline contains 'mem_encrypt=on': meaning SME memory encryption feature on the host is enabled
In addition to the kernel module parameter, we probably also need to check whether QEMU supports the feature. e.g, what if user has newer kernel but older qemu. e.g kernel > 4.18 but Qemu < 2.12. To check the SEV feature support, we should verify the following conditions:
1) check kernel module parameter is set Paulo implemented the checks following the documentation in docs/kbase/launch_security_sev.rst. The check for the module
On 5/11/20 9:40 PM, Brijesh Singh wrote: parameter sev is included. Is the check for the kernel cmdline parameter "mem_encrypt" not necessary? Or would that be covered by your suggested check in 2)? Brijesh correct me here please, but IIRC having mem_encrypt set is merely a good security recommendation but is orthogonal with kvm.amd_sev feature, IOW SEV should work without SME. If my memory serves well here, we don't need and also should not parse the kernel cmdline in this case.
Yes, that is correct. mem_encrypt=on is not required for the SEV to work. The mem_encrypt=on option is meant to enable the SME feature on the host machine. In addition to the guest, if customer wants to protect the Hypervsior memory from physical attacks then they can use SME or TSME. The SME can be enabled using mem_encrypt=on, whereas the TSME can be enabled in the BIOS and does not require any kernel changes. AMD is mostly recommending TSME to protect against the physical attacks.
2) check if /dev/sev device exist (aka firmware is detected) This seems reasonable. Shouldn't it have been documented in docs/kbase/launch_security_sev.rst? Sure, we can add a mention about this. Although, doesn't 1 imply 2? IOW can you have the kernel module parameter set to 1 and yet kernel doesn't expose the /dev/sev node?
Currently, 1 does not imply 2, KVM driver does not initialize the firmware during the feature probe (i.e does not access the /dev/sev). The firmware initialization is delayed until the first guest launch. So only sane way to know whether firmware is been detected is check the existence of the /dev/sev or issue a query-sev command . The query-sev command will send the platform_status request to the firmware, if the firmware is not ready then this command will fail.
Just to summarize the checks you want implemented for AMD SEV support: 1) check kernel module parameter is set (as already implemented) 2) check if /dev/sev device exist
Please note that these checks will also go into virt-host-validate in patch 5.
Sounds good to me, thanks.
3) Check if Qemu supports SEV feature (maybe we can detect the existence of the query-sev QMP command or detect Qemu version >= 2.12) ^This is already achieved by qemuMonitorJSONGetSEVCapabilities
The idea is to check the host capabilities to detect if qemus capabilities need to be reprobed. Therefore this would be a result if checks 1+2 change but it would not be a cache invalidation trigger. I agree in the sense that the SEV support that is currently being reported in QEMU capabilities wasn't a good choice because it reports only platform data which are constant to the host and have nothing to do with QEMU. It would be okay to just say <sev supported="yes|no"/> in qemu capabilities and report the rest in host capabilities. I haven't added this info into host capabilities when I realized the mistake because I didn't want to duplicate the platform info on 2 places. For the IBM secure guest feature, it makes total sense to report support within host capabilities, I'm just not sure whether we should do the same for SEV and try really hard to "fix" the mess.
Regards,
On Mon, May 11, 2020 at 06:41:58PM +0200, Boris Fiuczynski wrote:
From: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com>
Implement secure guest check for AMD SEV (Secure Encrypted Virtualization) in order to invalidate the qemu capabilities cache in case the availability of the feature changed.
For AMD SEV the verification consists of: - checking if /sys/module/kvm_amd/parameters/sev contains the value '1': meaning SEV is enabled in the host kernel; - checking if the kernel cmdline contains 'mem_encrypt=on': meaning SME memory encryption feature on the host is enabled
Signed-off-by: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- docs/kbase/launch_security_sev.rst | 2 +- src/qemu/qemu_capabilities.c | 27 +++++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/docs/kbase/launch_security_sev.rst b/docs/kbase/launch_security_sev.rst index 65f258587d..fa602c7432 100644 --- a/docs/kbase/launch_security_sev.rst +++ b/docs/kbase/launch_security_sev.rst @@ -109,7 +109,7 @@ following: </features> </domainCapabilities>
-Note that if libvirt was already installed and libvirtd running before +Note that if libvirt (<6.4.0) was already installed and libvirtd running before enabling SEV in the kernel followed by the host reboot you need to force libvirtd to re-probe both the host and QEMU capabilities. First stop libvirtd:
Docs updates tend to go to a separate patch.
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 2874bb1e7c..6cf926d52d 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4604,6 +4604,31 @@ virQEMUCapsKVMSupportsSecureGuestS390(void) }
+/* + * Check whether AMD Secure Encrypted Virtualization (x86) is enabled + */ +static bool +virQEMUCapsKVMSupportsSecureGuestAMD(void) +{ + g_autofree char *cmdline = NULL; + g_autofree char *modValue = NULL; + static const char *kValues[] = {"on"}; + + if (virFileReadValueString(&modValue, "/sys/module/kvm_amd/parameters/sev") < 0) + return false; + if (modValue[0] != '1') + return false; + if (virFileReadValueString(&cmdline, "/proc/cmdline") < 0) + return false; + if (virKernelCmdlineMatchParam(cmdline, "mem_encrypt", kValues, + G_N_ELEMENTS(kValues), + VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST | + VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ))
As discussed already, the last 2 checks will not be necessary.
+ return true; + return false; +} + + /* * Check whether the secure guest functionality is enabled. * See the specific architecture function for details on the verifications made. @@ -4615,6 +4640,8 @@ virQEMUCapsKVMSupportsSecureGuest(void)
if (ARCH_IS_S390(arch)) return virQEMUCapsKVMSupportsSecureGuestS390(); + if (ARCH_IS_X86(arch)) + return virQEMUCapsKVMSupportsSecureGuestAMD();
For s390 the check will most likely suffice, but for x86, there isn't just SEV, there's also MKTME from Intel. However, given there's no support for that in libvirt, we can figure out later how to propagate host caps in here so that CPU vendor can be extracted. I just wanted to make it clear, that we'll need to differentiate the x86 callbacks according to the CPU vendor at some point. -- Erik Skultety
On 5/18/20 2:14 PM, Erik Skultety wrote:
On Mon, May 11, 2020 at 06:41:58PM +0200, Boris Fiuczynski wrote:
From: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com>
Implement secure guest check for AMD SEV (Secure Encrypted Virtualization) in order to invalidate the qemu capabilities cache in case the availability of the feature changed.
For AMD SEV the verification consists of: - checking if /sys/module/kvm_amd/parameters/sev contains the value '1': meaning SEV is enabled in the host kernel; - checking if the kernel cmdline contains 'mem_encrypt=on': meaning SME memory encryption feature on the host is enabled
Signed-off-by: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- docs/kbase/launch_security_sev.rst | 2 +- src/qemu/qemu_capabilities.c | 27 +++++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/docs/kbase/launch_security_sev.rst b/docs/kbase/launch_security_sev.rst index 65f258587d..fa602c7432 100644 --- a/docs/kbase/launch_security_sev.rst +++ b/docs/kbase/launch_security_sev.rst @@ -109,7 +109,7 @@ following: </features> </domainCapabilities>
-Note that if libvirt was already installed and libvirtd running before +Note that if libvirt (<6.4.0) was already installed and libvirtd running before enabling SEV in the kernel followed by the host reboot you need to force libvirtd to re-probe both the host and QEMU capabilities. First stop libvirtd:
Docs updates tend to go to a separate patch.
And I also have been told when I had the docs updates separate to put doc updates into the patches that cause them.
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 2874bb1e7c..6cf926d52d 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4604,6 +4604,31 @@ virQEMUCapsKVMSupportsSecureGuestS390(void) }
+/* + * Check whether AMD Secure Encrypted Virtualization (x86) is enabled + */ +static bool +virQEMUCapsKVMSupportsSecureGuestAMD(void) +{ + g_autofree char *cmdline = NULL; + g_autofree char *modValue = NULL; + static const char *kValues[] = {"on"}; + + if (virFileReadValueString(&modValue, "/sys/module/kvm_amd/parameters/sev") < 0) + return false; + if (modValue[0] != '1') + return false; + if (virFileReadValueString(&cmdline, "/proc/cmdline") < 0) + return false; + if (virKernelCmdlineMatchParam(cmdline, "mem_encrypt", kValues, + G_N_ELEMENTS(kValues), + VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST | + VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ))
As discussed already, the last 2 checks will not be necessary.
Yes, I have made some updates for a follow up version.
+ return true; + return false; +} + + /* * Check whether the secure guest functionality is enabled. * See the specific architecture function for details on the verifications made. @@ -4615,6 +4640,8 @@ virQEMUCapsKVMSupportsSecureGuest(void)
if (ARCH_IS_S390(arch)) return virQEMUCapsKVMSupportsSecureGuestS390(); + if (ARCH_IS_X86(arch)) + return virQEMUCapsKVMSupportsSecureGuestAMD();
For s390 the check will most likely suffice, but for x86, there isn't just SEV, there's also MKTME from Intel. However, given there's no support for that in libvirt, we can figure out later how to propagate host caps in here so that CPU vendor can be extracted. I just wanted to make it clear, that we'll need to differentiate the x86 callbacks according to the CPU vendor at some point.
I agree that once Intels MKTME will be supported by libvirt some massaging needs to be done for the AMD/Intel split. -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Add checking in virt-host-validate for secure guest support on s390 for IBM Secure Execution. Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Tested-by: Viktor Mihajlovski <mihajlov@linux.ibm.com> Reviewed-by: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> --- tools/virt-host-validate-common.c | 58 +++++++++++++++++++++++++++++-- tools/virt-host-validate-common.h | 4 +++ tools/virt-host-validate-qemu.c | 4 +++ 3 files changed, 64 insertions(+), 2 deletions(-) diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index fbefbada96..dd73bd0dea 100644 --- a/tools/virt-host-validate-common.c +++ b/tools/virt-host-validate-common.c @@ -40,7 +40,8 @@ VIR_ENUM_IMPL(virHostValidateCPUFlag, VIR_HOST_VALIDATE_CPU_FLAG_LAST, "vmx", "svm", - "sie"); + "sie", + "158"); static bool quiet; @@ -210,7 +211,8 @@ virBitmapPtr virHostValidateGetCPUFlags(void) * on the architecture, so check possible prefixes */ if (!STRPREFIX(line, "flags") && !STRPREFIX(line, "Features") && - !STRPREFIX(line, "features")) + !STRPREFIX(line, "features") && + !STRPREFIX(line, "facilities")) continue; /* fgets() includes the trailing newline in the output buffer, @@ -439,3 +441,55 @@ bool virHostKernelModuleIsLoaded(const char *module) return ret; } + + +int virHostValidateSecureGuests(const char *hvname, + virHostValidateLevel level) +{ + virBitmapPtr flags; + bool hasFac158 = false; + virArch arch = virArchFromHost(); + g_autofree char *cmdline = NULL; + static const char *kIBMValues[] = {"y", "Y", "1"}; + + flags = virHostValidateGetCPUFlags(); + + if (flags && virBitmapIsBitSet(flags, VIR_HOST_VALIDATE_CPU_FLAG_FACILITY_158)) + hasFac158 = true; + + virBitmapFree(flags); + + virHostMsgCheck(hvname, "%s", _("for secure guest support")); + if (ARCH_IS_S390(arch)) { + if (hasFac158) { + if (!virFileIsDir("/sys/firmware/uv")) { + virHostMsgFail(level, "IBM Secure Execution not supported by " + "the currently used kernel"); + return 0; + } + if (virFileReadValueString(&cmdline, "/proc/cmdline") < 0) + return -1; + if (virKernelCmdlineMatchParam(cmdline, "prot_virt", kIBMValues, + G_N_ELEMENTS(kIBMValues), + VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY | + VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX)) { + virHostMsgPass(); + return 1; + } else { + virHostMsgFail(level, + "IBM Secure Execution appears to be disabled " + "in kernel. Add prot_virt=1 to kernel cmdline " + "arguments"); + } + } else { + virHostMsgFail(level, "Hardware or firmware does not provide " + "support for IBM Secure Execution"); + } + } else { + virHostMsgFail(level, + "Unknown if this platform has Secure Guest support"); + return -1; + } + + return 0; +} diff --git a/tools/virt-host-validate-common.h b/tools/virt-host-validate-common.h index 8ae60a21de..44b5544a12 100644 --- a/tools/virt-host-validate-common.h +++ b/tools/virt-host-validate-common.h @@ -37,6 +37,7 @@ typedef enum { VIR_HOST_VALIDATE_CPU_FLAG_VMX = 0, VIR_HOST_VALIDATE_CPU_FLAG_SVM, VIR_HOST_VALIDATE_CPU_FLAG_SIE, + VIR_HOST_VALIDATE_CPU_FLAG_FACILITY_158, VIR_HOST_VALIDATE_CPU_FLAG_LAST, } virHostValidateCPUFlag; @@ -83,4 +84,7 @@ int virHostValidateCGroupControllers(const char *hvname, int virHostValidateIOMMU(const char *hvname, virHostValidateLevel level); +int virHostValidateSecureGuests(const char *hvname, + virHostValidateLevel level); + bool virHostKernelModuleIsLoaded(const char *module); diff --git a/tools/virt-host-validate-qemu.c b/tools/virt-host-validate-qemu.c index bd717a604e..ea7f172790 100644 --- a/tools/virt-host-validate-qemu.c +++ b/tools/virt-host-validate-qemu.c @@ -127,5 +127,9 @@ int virHostValidateQEMU(void) VIR_HOST_VALIDATE_WARN) < 0) ret = -1; + if (virHostValidateSecureGuests("QEMU", + VIR_HOST_VALIDATE_WARN) < 0) + ret = -1; + return ret; } -- 2.25.1
On Mon, May 11, 2020 at 06:41:59PM +0200, Boris Fiuczynski wrote:
Add checking in virt-host-validate for secure guest support on s390 for IBM Secure Execution.
Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Tested-by: Viktor Mihajlovski <mihajlov@linux.ibm.com> Reviewed-by: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> --- tools/virt-host-validate-common.c | 58 +++++++++++++++++++++++++++++-- tools/virt-host-validate-common.h | 4 +++ tools/virt-host-validate-qemu.c | 4 +++ 3 files changed, 64 insertions(+), 2 deletions(-)
diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index fbefbada96..dd73bd0dea 100644 --- a/tools/virt-host-validate-common.c +++ b/tools/virt-host-validate-common.c @@ -40,7 +40,8 @@ VIR_ENUM_IMPL(virHostValidateCPUFlag, VIR_HOST_VALIDATE_CPU_FLAG_LAST, "vmx", "svm", - "sie"); + "sie", + "158");
static bool quiet;
@@ -210,7 +211,8 @@ virBitmapPtr virHostValidateGetCPUFlags(void) * on the architecture, so check possible prefixes */ if (!STRPREFIX(line, "flags") && !STRPREFIX(line, "Features") && - !STRPREFIX(line, "features")) + !STRPREFIX(line, "features") && + !STRPREFIX(line, "facilities"))
I can't comment on the first 2 hunks as I don't have an appropriate s390 at hand, so I can only trust you it's the correct way.
continue;
/* fgets() includes the trailing newline in the output buffer, @@ -439,3 +441,55 @@ bool virHostKernelModuleIsLoaded(const char *module)
return ret; } + + +int virHostValidateSecureGuests(const char *hvname, + virHostValidateLevel level) +{ + virBitmapPtr flags; + bool hasFac158 = false; + virArch arch = virArchFromHost(); + g_autofree char *cmdline = NULL; + static const char *kIBMValues[] = {"y", "Y", "1"}; + + flags = virHostValidateGetCPUFlags(); + + if (flags && virBitmapIsBitSet(flags, VIR_HOST_VALIDATE_CPU_FLAG_FACILITY_158)) + hasFac158 = true; + + virBitmapFree(flags); + + virHostMsgCheck(hvname, "%s", _("for secure guest support")); + if (ARCH_IS_S390(arch)) {
We don't need ^this check here, facility 158 won't be available on x86.
+ if (hasFac158) {
btw ^such checks should be adopted in QEMU caps code as well like, but we don't have an appropriate helper at the moment.
+ if (!virFileIsDir("/sys/firmware/uv")) { + virHostMsgFail(level, "IBM Secure Execution not supported by " + "the currently used kernel"); + return 0; + } + if (virFileReadValueString(&cmdline, "/proc/cmdline") < 0) + return -1; + if (virKernelCmdlineMatchParam(cmdline, "prot_virt", kIBMValues, + G_N_ELEMENTS(kIBMValues), + VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY | + VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX)) { + virHostMsgPass(); + return 1; + } else { + virHostMsgFail(level, + "IBM Secure Execution appears to be disabled " + "in kernel. Add prot_virt=1 to kernel cmdline " + "arguments"); + } + } else { + virHostMsgFail(level, "Hardware or firmware does not provide " + "support for IBM Secure Execution"); + } + } else { + virHostMsgFail(level, + "Unknown if this platform has Secure Guest support"); + return -1; + } + + return 0; +} diff --git a/tools/virt-host-validate-common.h b/tools/virt-host-validate-common.h index 8ae60a21de..44b5544a12 100644 --- a/tools/virt-host-validate-common.h +++ b/tools/virt-host-validate-common.h @@ -37,6 +37,7 @@ typedef enum { VIR_HOST_VALIDATE_CPU_FLAG_VMX = 0, VIR_HOST_VALIDATE_CPU_FLAG_SVM, VIR_HOST_VALIDATE_CPU_FLAG_SIE, + VIR_HOST_VALIDATE_CPU_FLAG_FACILITY_158,
VIR_HOST_VALIDATE_CPU_FLAG_LAST, } virHostValidateCPUFlag; @@ -83,4 +84,7 @@ int virHostValidateCGroupControllers(const char *hvname, int virHostValidateIOMMU(const char *hvname, virHostValidateLevel level);
+int virHostValidateSecureGuests(const char *hvname, + virHostValidateLevel level); + bool virHostKernelModuleIsLoaded(const char *module); diff --git a/tools/virt-host-validate-qemu.c b/tools/virt-host-validate-qemu.c index bd717a604e..ea7f172790 100644 --- a/tools/virt-host-validate-qemu.c +++ b/tools/virt-host-validate-qemu.c @@ -127,5 +127,9 @@ int virHostValidateQEMU(void) VIR_HOST_VALIDATE_WARN) < 0) ret = -1;
+ if (virHostValidateSecureGuests("QEMU", + VIR_HOST_VALIDATE_WARN) < 0) + ret = -1; + return ret; } -- 2.25.1
-- Erik Skultety
On 5/18/20 2:59 PM, Erik Skultety wrote:
On Mon, May 11, 2020 at 06:41:59PM +0200, Boris Fiuczynski wrote:
Add checking in virt-host-validate for secure guest support on s390 for IBM Secure Execution.
Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Tested-by: Viktor Mihajlovski <mihajlov@linux.ibm.com> Reviewed-by: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> --- tools/virt-host-validate-common.c | 58 +++++++++++++++++++++++++++++-- tools/virt-host-validate-common.h | 4 +++ tools/virt-host-validate-qemu.c | 4 +++ 3 files changed, 64 insertions(+), 2 deletions(-)
diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index fbefbada96..dd73bd0dea 100644 --- a/tools/virt-host-validate-common.c +++ b/tools/virt-host-validate-common.c @@ -40,7 +40,8 @@ VIR_ENUM_IMPL(virHostValidateCPUFlag, VIR_HOST_VALIDATE_CPU_FLAG_LAST, "vmx", "svm", - "sie"); + "sie", + "158");
static bool quiet;
@@ -210,7 +211,8 @@ virBitmapPtr virHostValidateGetCPUFlags(void) * on the architecture, so check possible prefixes */ if (!STRPREFIX(line, "flags") && !STRPREFIX(line, "Features") && - !STRPREFIX(line, "features")) + !STRPREFIX(line, "features") && + !STRPREFIX(line, "facilities"))
I can't comment on the first 2 hunks as I don't have an appropriate s390 at hand, so I can only trust you it's the correct way.
That is fine. We have given this some testing on s390.
continue;
/* fgets() includes the trailing newline in the output buffer, @@ -439,3 +441,55 @@ bool virHostKernelModuleIsLoaded(const char *module)
return ret; } + + +int virHostValidateSecureGuests(const char *hvname, + virHostValidateLevel level) +{ + virBitmapPtr flags; + bool hasFac158 = false; + virArch arch = virArchFromHost(); + g_autofree char *cmdline = NULL; + static const char *kIBMValues[] = {"y", "Y", "1"}; + + flags = virHostValidateGetCPUFlags(); + + if (flags && virBitmapIsBitSet(flags, VIR_HOST_VALIDATE_CPU_FLAG_FACILITY_158)) + hasFac158 = true; + + virBitmapFree(flags); + + virHostMsgCheck(hvname, "%s", _("for secure guest support")); + if (ARCH_IS_S390(arch)) {
We don't need ^this check here, facility 158 won't be available on x86.
Since it is very true that the facility 158 won't be available on x86 you do not want to get the message "Hardware or firmware does not provide support for IBM Secure Execution" on x86. On the other hand you can be on s390 and not have the facility available and want to know see the message.
+ if (hasFac158) {
btw ^such checks should be adopted in QEMU caps code as well like, but we don't have an appropriate helper at the moment.
+ if (!virFileIsDir("/sys/firmware/uv")) { + virHostMsgFail(level, "IBM Secure Execution not supported by " + "the currently used kernel"); + return 0; + } + if (virFileReadValueString(&cmdline, "/proc/cmdline") < 0) + return -1; + if (virKernelCmdlineMatchParam(cmdline, "prot_virt", kIBMValues, + G_N_ELEMENTS(kIBMValues), + VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY | + VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX)) { + virHostMsgPass(); + return 1; + } else { + virHostMsgFail(level, + "IBM Secure Execution appears to be disabled " + "in kernel. Add prot_virt=1 to kernel cmdline " + "arguments"); + } + } else { + virHostMsgFail(level, "Hardware or firmware does not provide " + "support for IBM Secure Execution"); + } + } else { + virHostMsgFail(level, + "Unknown if this platform has Secure Guest support"); + return -1; + } + + return 0; +} diff --git a/tools/virt-host-validate-common.h b/tools/virt-host-validate-common.h index 8ae60a21de..44b5544a12 100644 --- a/tools/virt-host-validate-common.h +++ b/tools/virt-host-validate-common.h @@ -37,6 +37,7 @@ typedef enum { VIR_HOST_VALIDATE_CPU_FLAG_VMX = 0, VIR_HOST_VALIDATE_CPU_FLAG_SVM, VIR_HOST_VALIDATE_CPU_FLAG_SIE, + VIR_HOST_VALIDATE_CPU_FLAG_FACILITY_158,
VIR_HOST_VALIDATE_CPU_FLAG_LAST, } virHostValidateCPUFlag; @@ -83,4 +84,7 @@ int virHostValidateCGroupControllers(const char *hvname, int virHostValidateIOMMU(const char *hvname, virHostValidateLevel level);
+int virHostValidateSecureGuests(const char *hvname, + virHostValidateLevel level); + bool virHostKernelModuleIsLoaded(const char *module); diff --git a/tools/virt-host-validate-qemu.c b/tools/virt-host-validate-qemu.c index bd717a604e..ea7f172790 100644 --- a/tools/virt-host-validate-qemu.c +++ b/tools/virt-host-validate-qemu.c @@ -127,5 +127,9 @@ int virHostValidateQEMU(void) VIR_HOST_VALIDATE_WARN) < 0) ret = -1;
+ if (virHostValidateSecureGuests("QEMU", + VIR_HOST_VALIDATE_WARN) < 0) + ret = -1; + return ret; } -- 2.25.1
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
...
+ + virHostMsgCheck(hvname, "%s", _("for secure guest support")); + if (ARCH_IS_S390(arch)) {
We don't need ^this check here, facility 158 won't be available on x86.
Since it is very true that the facility 158 won't be available on x86 you do not want to get the message "Hardware or firmware does not provide support for IBM Secure Execution" on x86. On the other hand you can be on s390 and not have the facility available and want to know see the message.
Oops, I missed the last nested else block, please ignore what I said then. -- Erik Skultety
Add checking in virt-host-validate for secure guest support on x86 for AMD Secure Encrypted Virtualization. Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Reviewed-by: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> --- docs/kbase/launch_security_sev.rst | 7 ++++-- tools/virt-host-validate-common.c | 36 ++++++++++++++++++++++++++++-- tools/virt-host-validate-common.h | 1 + 3 files changed, 40 insertions(+), 4 deletions(-) diff --git a/docs/kbase/launch_security_sev.rst b/docs/kbase/launch_security_sev.rst index fa602c7432..45166b3886 100644 --- a/docs/kbase/launch_security_sev.rst +++ b/docs/kbase/launch_security_sev.rst @@ -30,8 +30,11 @@ Enabling SEV on the host ======================== Before VMs can make use of the SEV feature you need to make sure your -AMD CPU does support SEV. You can check whether SEV is among the CPU -flags with: +AMD CPU does support SEV. You can run ``libvirt-host-validate`` +(libvirt >= 6.4.0) to check if your host supports secure guests or you +can follow the manual checks below. + +You can manually check whether SEV is among the CPU flags with: :: diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index dd73bd0dea..67aa420d35 100644 --- a/tools/virt-host-validate-common.c +++ b/tools/virt-host-validate-common.c @@ -41,7 +41,8 @@ VIR_ENUM_IMPL(virHostValidateCPUFlag, "vmx", "svm", "sie", - "158"); + "158", + "sev"); static bool quiet; @@ -447,15 +448,19 @@ int virHostValidateSecureGuests(const char *hvname, virHostValidateLevel level) { virBitmapPtr flags; - bool hasFac158 = false; + bool hasFac158 = false, hasAMDSev = false; virArch arch = virArchFromHost(); g_autofree char *cmdline = NULL; static const char *kIBMValues[] = {"y", "Y", "1"}; + static const char *kAMDValues[] = {"on"}; + g_autofree char *mod_value = NULL; flags = virHostValidateGetCPUFlags(); if (flags && virBitmapIsBitSet(flags, VIR_HOST_VALIDATE_CPU_FLAG_FACILITY_158)) hasFac158 = true; + else if (flags && virBitmapIsBitSet(flags, VIR_HOST_VALIDATE_CPU_FLAG_SEV)) + hasAMDSev = true; virBitmapFree(flags); @@ -485,6 +490,33 @@ int virHostValidateSecureGuests(const char *hvname, virHostMsgFail(level, "Hardware or firmware does not provide " "support for IBM Secure Execution"); } + } else if (hasAMDSev) { + if (virFileReadValueString(&mod_value, "/sys/module/kvm_amd/parameters/sev") < 0) { + virHostMsgFail(level, "AMD Secure Encrypted Virtualization not " + "supported by the currently used kernel"); + return 0; + } + if (mod_value[0] != '1') { + virHostMsgFail(level, + "AMD Secure Encrypted Virtualization appears to be " + "disabled in kernel. Add mem_encrypt=on " + "kvm_amd.sev=1 to kernel cmdline arguments"); + return 0; + } + if (virFileReadValueString(&cmdline, "/proc/cmdline") < 0) + return -1; + if (virKernelCmdlineMatchParam(cmdline, "mem_encrypt", kAMDValues, + G_N_ELEMENTS(kAMDValues), + VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST | + VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ)) { + virHostMsgPass(); + return 1; + } else { + virHostMsgFail(level, + "AMD Secure Encrypted Virtualization appears to be " + "disabled in kernel. Add mem_encrypt=on " + "kvm_amd.sev=1 to kernel cmdline arguments"); + } } else { virHostMsgFail(level, "Unknown if this platform has Secure Guest support"); diff --git a/tools/virt-host-validate-common.h b/tools/virt-host-validate-common.h index 44b5544a12..3df5ea0c7e 100644 --- a/tools/virt-host-validate-common.h +++ b/tools/virt-host-validate-common.h @@ -38,6 +38,7 @@ typedef enum { VIR_HOST_VALIDATE_CPU_FLAG_SVM, VIR_HOST_VALIDATE_CPU_FLAG_SIE, VIR_HOST_VALIDATE_CPU_FLAG_FACILITY_158, + VIR_HOST_VALIDATE_CPU_FLAG_SEV, VIR_HOST_VALIDATE_CPU_FLAG_LAST, } virHostValidateCPUFlag; -- 2.25.1
On Mon, May 11, 2020 at 06:42:00PM +0200, Boris Fiuczynski wrote:
Add checking in virt-host-validate for secure guest support on x86 for AMD Secure Encrypted Virtualization.
Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Reviewed-by: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> --- docs/kbase/launch_security_sev.rst | 7 ++++-- tools/virt-host-validate-common.c | 36 ++++++++++++++++++++++++++++-- tools/virt-host-validate-common.h | 1 + 3 files changed, 40 insertions(+), 4 deletions(-)
diff --git a/docs/kbase/launch_security_sev.rst b/docs/kbase/launch_security_sev.rst index fa602c7432..45166b3886 100644 --- a/docs/kbase/launch_security_sev.rst +++ b/docs/kbase/launch_security_sev.rst @@ -30,8 +30,11 @@ Enabling SEV on the host ========================
Before VMs can make use of the SEV feature you need to make sure your -AMD CPU does support SEV. You can check whether SEV is among the CPU -flags with: +AMD CPU does support SEV. You can run ``libvirt-host-validate`` +(libvirt >= 6.4.0) to check if your host supports secure guests or you +can follow the manual checks below. + +You can manually check whether SEV is among the CPU flags with:
^this change should go along the (<6.4.0) in one of the earlier patches into a standalone patch. Otherwise looking good. -- Erik Skultety
On 5/18/20 3:01 PM, Erik Skultety wrote:
On Mon, May 11, 2020 at 06:42:00PM +0200, Boris Fiuczynski wrote:
Add checking in virt-host-validate for secure guest support on x86 for AMD Secure Encrypted Virtualization.
Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Reviewed-by: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> --- docs/kbase/launch_security_sev.rst | 7 ++++-- tools/virt-host-validate-common.c | 36 ++++++++++++++++++++++++++++-- tools/virt-host-validate-common.h | 1 + 3 files changed, 40 insertions(+), 4 deletions(-)
diff --git a/docs/kbase/launch_security_sev.rst b/docs/kbase/launch_security_sev.rst index fa602c7432..45166b3886 100644 --- a/docs/kbase/launch_security_sev.rst +++ b/docs/kbase/launch_security_sev.rst @@ -30,8 +30,11 @@ Enabling SEV on the host ========================
Before VMs can make use of the SEV feature you need to make sure your -AMD CPU does support SEV. You can check whether SEV is among the CPU -flags with: +AMD CPU does support SEV. You can run ``libvirt-host-validate`` +(libvirt >= 6.4.0) to check if your host supports secure guests or you +can follow the manual checks below. + +You can manually check whether SEV is among the CPU flags with:
^this change should go along the (<6.4.0) in one of the earlier patches into a standalone patch.
Actually the earlier patches fix the stale cap cache and this update is because of a new support in libvirt-host-validate. I am not sure that we should tie these to into one patch. I would prefer to keep the two doc changes separate and with the changes that caused the update.
Otherwise looking good.
Thanks but the changes need also to be adjusted as discussed on patch 3. I will do so in a followup version. -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
...
diff --git a/docs/kbase/launch_security_sev.rst b/docs/kbase/launch_security_sev.rst index fa602c7432..45166b3886 100644 --- a/docs/kbase/launch_security_sev.rst +++ b/docs/kbase/launch_security_sev.rst @@ -30,8 +30,11 @@ Enabling SEV on the host ======================== Before VMs can make use of the SEV feature you need to make sure your -AMD CPU does support SEV. You can check whether SEV is among the CPU -flags with: +AMD CPU does support SEV. You can run ``libvirt-host-validate`` +(libvirt >= 6.4.0) to check if your host supports secure guests or you +can follow the manual checks below. + +You can manually check whether SEV is among the CPU flags with:
^this change should go along the (<6.4.0) in one of the earlier patches into a standalone patch.
Actually the earlier patches fix the stale cap cache and this update is because of a new support in libvirt-host-validate. I am not sure that we should tie these to into one patch. I would prefer to keep the two doc changes separate and with the changes that caused the update.
I won't argue against that logic. However, both patch 3 and this one update the same knowledge article. What IMO matters here the most is that once all of the changes you're introducing are applied as a unit, the article needs to reflect both the changes. From that perspective, at least to me it makes total sense to group the docs changes from both 3/6 and this patch to a single update to the SEV article accordingly. -- Erik Skultety
On 5/19/20 8:25 AM, Erik Skultety wrote:
...
diff --git a/docs/kbase/launch_security_sev.rst b/docs/kbase/launch_security_sev.rst index fa602c7432..45166b3886 100644 --- a/docs/kbase/launch_security_sev.rst +++ b/docs/kbase/launch_security_sev.rst @@ -30,8 +30,11 @@ Enabling SEV on the host ======================== Before VMs can make use of the SEV feature you need to make sure your -AMD CPU does support SEV. You can check whether SEV is among the CPU -flags with: +AMD CPU does support SEV. You can run ``libvirt-host-validate`` +(libvirt >= 6.4.0) to check if your host supports secure guests or you +can follow the manual checks below. + +You can manually check whether SEV is among the CPU flags with:
^this change should go along the (<6.4.0) in one of the earlier patches into a standalone patch.
Actually the earlier patches fix the stale cap cache and this update is because of a new support in libvirt-host-validate. I am not sure that we should tie these to into one patch. I would prefer to keep the two doc changes separate and with the changes that caused the update.
I won't argue against that logic. However, both patch 3 and this one update the same knowledge article. What IMO matters here the most is that once all of the changes you're introducing are applied as a unit, the article needs to reflect both the changes. From that perspective, at least to me it makes total sense to group the docs changes from both 3/6 and this patch to a single update to the SEV article accordingly.
Fine, I will sort out the changes and create a single AMD doc update patch. -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
From: Viktor Mihajlovski <mihajlov@linux.ibm.com> Protected virtualization/IBM Secure Execution for Linux protects guest memory and state from the host. Add some basic information about technology and a brief guide on setting up secure guests with libvirt. Signed-off-by: Viktor Mihajlovski <mihajlov@linux.ibm.com> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Reviewed-by: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com> --- docs/kbase.html.in | 3 + docs/kbase/s390_protected_virt.rst | 189 +++++++++++++++++++++++++++++ 2 files changed, 192 insertions(+) create mode 100644 docs/kbase/s390_protected_virt.rst diff --git a/docs/kbase.html.in b/docs/kbase.html.in index c586e0f676..241a212fa9 100644 --- a/docs/kbase.html.in +++ b/docs/kbase.html.in @@ -14,6 +14,9 @@ <dt><a href="kbase/secureusage.html">Secure usage</a></dt> <dd>Secure usage of the libvirt APIs</dd> + <dt><a href="kbase/s390_protected_virt.html">Protected virtualization on s390</a></dt> + <dd>Running secure s390 guests with IBM Secure Execution</dd> + <dt><a href="kbase/launch_security_sev.html">Launch security</a></dt> <dd>Securely launching VMs with AMD SEV</dd> diff --git a/docs/kbase/s390_protected_virt.rst b/docs/kbase/s390_protected_virt.rst new file mode 100644 index 0000000000..3e354455d1 --- /dev/null +++ b/docs/kbase/s390_protected_virt.rst @@ -0,0 +1,189 @@ +================================ +Protected Virtualization on s390 +================================ + +.. contents:: + +Overview +======== + +Protected virtualization, also known as IBM Secure Execution is a +hardware-based privacy protection technology for s390x (IBM Z). +It allows to execute virtual machines such that the host system +has no access to a VM's state and memory contents. + +Unlike other similar technologies, the memory of a running guest +is not encrypted but protected by hardware access controls, which +may only be manipulated by trusted system firmware, called +ultravisor. + +For the cases where the host needs access to guest memory (e.g. for +paging), it can request pages to be exported to it. The exported page +will be encrypted with a unique key for the running guest by the +ultravisor. The ultravisor also computes an integrity value for +the page, and stores it in a special table, together with the page +index and a counter. This way it can verify the integrity of +the page content upon re-import into the guest. + +In other cases it may be necessary for a guest to grant the host access +to dedicated memory regions (e.g. for I/O). The guest can request +that the ultravisor removes the memory protection from individual +pages, so that they can be shared with the host. Likewise, the +guest can undo the sharing. + +A secure guest will initially start in a regular non-protected VM. +The start-up is controlled by a small bootstrap program loaded +into memory together with encrypted operating system components and +a control structure (the PV header). +The operating system components (e.g. Linux kernel, initial RAM +file system, kernel parameters) are encrypted and integrity +protected. The component encryption keys and integrity values are +stored in the PV header. +The PV header is wrapped with a public key belonging to a specific +system (in fact it can be wrapped with multiple such keys). The +matching private key is only accessible by trusted hardware and +firmware in that specific system. +Consequently, such a secure guest boot image can only be run on the +systems it has been prepared for. Its contents can't be decrypted +without access to the private key and it can't be modified as +it is integrity protected. + +Host Requirements +================= + +IBM Secure Execution for Linux has some hardware and firmware +requirements. The system hardware must be an IBM z15 (or newer), +or an IBM LinuxONE III (or newer). + +It is also necessary that the IBM Secure Execution feature is +enabled for that system. With libvirt >= 6.4.0 you can run +``libvirt-host--validate`` or otherwise check for facility '158', e.g. + +:: + + $ grep facilities /proc/cpuinfo | grep 158 + +The kernel must include the protected virtualization support +which can be verified by checking for the presence of directory +``/sys/firmware/uv``. It will only be present when both the +hardware and the kernel support are available. + +Finally, the host operating system must donate some memory to +the ultravisor needed to store memory security information. +This is achieved by specifying the following kernel command +line parameter to the host boot configuration + +:: + + prot_virt=1 + + +Guest Requirements +================== + +Guest Boot +---------- + +To start a guest in protected virtualization secure mode, the +boot image must have been prepared first with the program +``genprotimg`` using the correct public key for this host. +``genprotimg`` is part of the package ``s390-tools``, or +``s390-utils``, depending on the Linux distribution being used. +It can also be found at +`<https://github.com/ibm-s390-tools/s390-tools/tree/master/genprotimg>`_ + +The guests have to be configured to use the host CPU model, which +must contain the ``unpack`` facility indicating ultravisor guest support. + +With the following command it's possible to check whether the host +CPU model satisfies the requirement + +:: + + $ virsh domcapabilities | grep unpack + +which should return + +:: + + <feature policy='require' name='unpack'/> + +Note that on hosts with libvirt < 6.4.0 if the check fails despite +the host system actually supporting protected virtualization guests, +this can be caused by a stale libvirt capabilities cache. +To recover, run the following commands + +:: + + $ systemctl stop libvirtd + $ rm /var/cache/libvirt/qemu/capabilities/*.xml + $ systemctl start libvirtd + + +Guest I/O +--------- + +Protected virtualization guests support I/O using virtio devices. +As the virtio data structures of secure guests are not accessible +by the host, it is necessary to use shared memory ('bounce buffers'). + +To enable virtio devices to use shared buffers, it is necessary +to configure them with platform_iommu enabled. This can done by adding +``iommu='on'`` to the driver element of a virtio device definition in the +guest's XML, e.g. + +:: + + <interface type='network'> + <source network='default'/> + <model type='virtio'/> + <driver name='vhost' iommu='on'/> + </interface> + +It is mandatory to define all virtio bus devices in this way to +prevent the host from attempting to access protected memory. +Ballooning will not work and is fenced by QEMU. It should be +disabled by specifying + +:: + + <memballoon model='none'/> + +Finally, the guest Linux must be instructed to allocate I/O +buffers using memory shared between host and guest using SWIOTLB. +This is done by adding ``swiotlb=nnn`` to the guest's kernel command +line string, where ``nnn`` stands for the number of statically +allocated 2K entries. A commonly used value for swiotlb is 262144. + +Example guest definition +======================== + +Minimal domain XML for a protected virtualization guest, essentially +it's mostly about the ``iommu`` property + +:: + + <domain type='kvm'> + <name>protected</name> + <memory unit='KiB'>2048000</memory> + <currentMemory unit='KiB'>2048000</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='s390x'>hvm</type> + </os> + <cpu mode='host-model'/> + <devices> + <disk type='file' device='disk'> + <driver name='qemu' type='qcow2' cache='none' io='native' iommu='on'> + <source file='/var/lib/libvirt/images/protected.qcow2'/> + <target dev='vda' bus='virtio'/> + </disk> + <interface type='network'> + <driver iommu='on'/> + <source network='default'/> + <model type='virtio'/> + </interface> + <console type='pty'/> + <memballoon model='none'/> + </devices> + </domain> -- 2.25.1
participants (4)
-
Boris Fiuczynski -
Brijesh Singh -
Erik Skultety -
Paulo de Rezende Pinatti