[PATCH v2 0/7] 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 /dev/sev exists 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. Changes in v2: [Patch 1] Reworked kernel cmdline parser into a parameter based processing. [Patch 2] Added missing value "on" to kvalue list [Patch 3] Changed AMD SEV support check to module parameter is set and /dev/sev exists. Moved doc changes to a new standalone patch 6. [Patch 4] Added missing value "on" to kvalue list [Patch 5] Changed AMD SEV support check to align with patch 3. Moved doc changes to a new standalone patch 6. [Patch 6] Summarized AMD SEV doc changes from patches 3 and 5. Adjusted libvirt version number [Patch 7 (v1: Patch 6)] Adjusted libvirt version number link to v1: https://www.redhat.com/archives/libvir-list/2020-May/msg00416.html Boris Fiuczynski (3): tools: secure guest check on s390 in virt-host-validate tools: secure guest check for AMD in virt-host-validate docs: update AMD launch secure description 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 | 76 ++++++++++++ src/util/virutil.c | 169 ++++++++++++++++++++++++++ src/util/virutil.h | 17 +++ tests/utiltest.c | 141 +++++++++++++++++++++ tools/virt-host-validate-common.c | 83 ++++++++++++- tools/virt-host-validate-common.h | 5 + tools/virt-host-validate-qemu.c | 4 + 11 files changed, 693 insertions(+), 5 deletions(-) create mode 100644 docs/kbase/s390_protected_virt.rst -- 2.25.4

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> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/libvirt_private.syms | 2 + src/util/virutil.c | 169 +++++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 17 ++++ tests/utiltest.c | 141 ++++++++++++++++++++++++++++++++ 4 files changed, 329 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a6af44fe1c..a206a943c5 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3433,6 +3433,8 @@ virHostGetDRMRenderNode; virHostHasIOMMU; virIndexToDiskName; virIsDevMapperDevice; +virKernelCmdlineMatchParam; +virKernelCmdlineNextParam; virMemoryLimitIsSet; virMemoryLimitTruncate; virMemoryMaxValue; diff --git a/src/util/virutil.c b/src/util/virutil.c index fb46501142..749c9d7116 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1725,6 +1725,175 @@ virHostGetDRMRenderNode(void) return ret; } + +static const char *virKernelCmdlineSkipDbQuote(const char *cmdline, + bool *is_quoted) +{ + if (cmdline[0] == '"') { + *is_quoted = !(*is_quoted); + cmdline++; + } + return cmdline; +} + + +static size_t virKernelCmdlineSearchForward(const char *cmdline, + bool *is_quoted, + bool include_equal) +{ + size_t index; + + for (index = 0; cmdline[index]; index++) { + if ((!(*is_quoted) && g_ascii_isspace(cmdline[index])) || + (include_equal && cmdline[index] == '=')) + break; + virKernelCmdlineSkipDbQuote(cmdline + index, is_quoted); + } + return index; +} + + +static size_t virKernelCmdlineNextSpace(const char *cmdline, + bool *is_quoted) +{ + return virKernelCmdlineSearchForward(cmdline, is_quoted, false); +} + + +static size_t virKernelCmdlineNextSpaceOrEqual(const char *cmdline, + bool *is_quoted) +{ + return virKernelCmdlineSearchForward(cmdline, is_quoted, true); +} + + +static char* virKernelArgNormalize(const char *arg) +{ + return virStringReplace(arg, "_", "-"); +} + + +static char* virKernelCmdlineArgNormalize(const char *cmdline, size_t offset) +{ + g_autofree char *param = g_strndup((cmdline), offset); + + return virKernelArgNormalize(param); +} + + +/* + * Parse the kernel cmdline and store the next parameter in @param + * and the value of @param in @val which can be NULL if @param has + * no value. In addition returns the address right after @param=@value + * for possible further processing. + * + * @cmdline: kernel command line string to be checked for next parameter + * @param: pointer to hold retrieved parameter, will be NULL if none found + * @val: pointer to hold retrieved value of @param + * + * Returns a pointer to address right after @param=@val in the + * kernel command line, will point to the string's end (NULL) + * in case no next parameter is found + */ +const char *virKernelCmdlineNextParam(const char *cmdline, + char **param, + char **val) +{ + size_t offset; + bool is_quoted = false; + *param = NULL; + *val = NULL; + + virSkipSpaces(&cmdline); + cmdline = virKernelCmdlineSkipDbQuote(cmdline, &is_quoted); + offset = virKernelCmdlineNextSpaceOrEqual(cmdline, &is_quoted); + if (offset == 0) + return cmdline; + + *param = virKernelCmdlineArgNormalize(cmdline, offset); + cmdline = cmdline + offset; + /* param has no value */ + if (*cmdline != '=') + return cmdline; + + cmdline = virKernelCmdlineSkipDbQuote(++cmdline, &is_quoted); + offset = virKernelCmdlineNextSpace(cmdline, &is_quoted); + if (cmdline[offset-1] == '"') + *val = g_strndup(cmdline, offset-1); + else + *val = g_strndup(cmdline, offset); + + return cmdline + offset; +} + + +#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 of values + * - 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 *norm_arg = virKernelArgNormalize(arg); + g_autofree char *kparam = NULL; + g_autofree char *kval = NULL; + + while (next[0] != '\0') { + VIR_FREE(kparam); + VIR_FREE(kval); + next = virKernelCmdlineNextParam(next, &kparam, &kval); + if (!kparam) + break; + if (STRNEQ(kparam, norm_arg)) + continue; + 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..7499b78153 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -147,6 +147,23 @@ 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; + +const char *virKernelCmdlineNextParam(const char *cmdline, + char **param, + char **val); + +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..01fb8c89f5 100644 --- a/tests/utiltest.c +++ b/tests/utiltest.c @@ -254,6 +254,145 @@ testOverflowCheckMacro(const void *data G_GNUC_UNUSED) } +struct testKernelCmdlineNextParamData +{ + const char *cmdline; + const char *param; + const char *val; + const char *next; +}; + +static struct testKernelCmdlineNextParamData kEntries[] = { + { "arg1 arg2 arg3=val1", "arg1", NULL, " arg2 arg3=val1" }, + { "arg1=val1 arg2 arg3=val3 arg4", "arg1", "val1", " arg2 arg3=val3 arg4" }, + { "arg3=val3 ", "arg3", "val3", " " }, + { "arg3=val3", "arg3", "val3", "" }, + { "arg-3=val3 arg4", "arg-3", "val3", " arg4" }, + { "arg_3=val3 arg4", "arg-3", "val3", " arg4" }, + { " arg_3=val3 arg4", "arg-3", "val3", " arg4" }, + { " arg-3=val3 arg4", "arg-3", "val3", " arg4" }, + { "arg2=\"value with spaces\" arg3=val3", "arg2", "value with spaces", " arg3=val3" }, + { " arg2=\"value with spaces\" arg3=val3", "arg2", "value with spaces", " arg3=val3" }, + { " \"arg2=value with spaces\" arg3=val3", "arg2", "value with spaces", " arg3=val3" }, + { "arg2=\"val\"ue arg3", "arg2", "val\"ue", " arg3" }, + { " arg3\" escaped=val2\"", "arg3\" escaped", "val2", "" }, + { " arg2longer=someval arg2=val2 arg3 ", "arg2longer", "someval", " arg2=val2 arg3 " }, + { "=val1 arg2=val2", NULL, NULL, "=val1 arg2=val2" }, + { " ", NULL, NULL, "" }, + { "", NULL, NULL, "" }, +}; + +static int +testKernelCmdlineNextParam(const void *data G_GNUC_UNUSED) +{ + char *param = NULL; + char *val = NULL; + const char *next; + size_t i; + + for (i = 0; i < G_N_ELEMENTS(kEntries); ++i) { + VIR_FREE(param); + VIR_FREE(val); + + next = virKernelCmdlineNextParam(kEntries[i].cmdline, ¶m, &val); + + if (STRNEQ_NULLABLE(param, kEntries[i].param) || + STRNEQ_NULLABLE(val, kEntries[i].val) || + STRNEQ(next, kEntries[i].next)) { + VIR_TEST_DEBUG("\nKernel cmdline [%s]", kEntries[i].cmdline); + VIR_TEST_DEBUG("Expect param [%s]", kEntries[i].param); + VIR_TEST_DEBUG("Actual param [%s]", param); + VIR_TEST_DEBUG("Expect value [%s]", kEntries[i].val); + VIR_TEST_DEBUG("Actual value [%s]", val); + VIR_TEST_DEBUG("Expect next [%s]", kEntries[i].next); + VIR_TEST_DEBUG("Actual next [%s]", next); + + VIR_FREE(param); + VIR_FREE(val); + + return -1; + } + } + + VIR_FREE(param); + 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 }, + {"arg1 my-arg=no arg2=val2 arg4=val4 my_arg=yes arg5", "my-arg", {"on", "yes"}, VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST | VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ, true }, + {"arg1 my-arg=no arg2=val2 arg4=val4 my_arg=yes arg5 ", "my-arg", {"on", "yes"}, VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST | VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ, true }, + {"arg1 my-arg arg2=val2 arg4=val4 my_arg=yes arg5", "my_arg", {NULL, NULL}, VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY, true }, + {"arg1 my-arg arg2=val2 arg4=val4 my-arg=yes arg5", "my_arg", {NULL, NULL}, VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY, true }, + {"=val1 my-arg arg2=val2 arg4=val4 my-arg=yes arg5", "myarg", {NULL, NULL}, VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY, false }, + {"arg1 arg2=", "arg2", {"", ""}, VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST | VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ, true }, + {" ", "myarg", {NULL, NULL}, VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST, false }, + {"", "", {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 +416,8 @@ mymain(void) DO_TEST(ParseVersionString); DO_TEST(RoundValueToPowerOfTwo); DO_TEST(OverflowCheckMacro); + DO_TEST(KernelCmdlineNextParam); + DO_TEST(KernelCmdlineMatchParam); return result == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.25.4

Paulo de Rezende Pinatti <ppinatti@linux.ibm.com> [2020-05-29, 12:10PM +0200]:
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> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/libvirt_private.syms | 2 + src/util/virutil.c | 169 +++++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 17 ++++ tests/utiltest.c | 141 ++++++++++++++++++++++++++++++++ 4 files changed, 329 insertions(+)
Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com>

On Fri, May 29, 2020 at 12:10:03PM +0200, Paulo de Rezende Pinatti wrote:
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> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/libvirt_private.syms | 2 + src/util/virutil.c | 169 +++++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 17 ++++ tests/utiltest.c | 141 ++++++++++++++++++++++++++++++++ 4 files changed, 329 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a6af44fe1c..a206a943c5 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3433,6 +3433,8 @@ virHostGetDRMRenderNode; virHostHasIOMMU; virIndexToDiskName; virIsDevMapperDevice; +virKernelCmdlineMatchParam; +virKernelCmdlineNextParam; virMemoryLimitIsSet; virMemoryLimitTruncate; virMemoryMaxValue; diff --git a/src/util/virutil.c b/src/util/virutil.c index fb46501142..749c9d7116 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1725,6 +1725,175 @@ virHostGetDRMRenderNode(void) return ret; }
+ +static const char *virKernelCmdlineSkipDbQuote(const char *cmdline,
minor nitpick: we can call ^this simply ...SkipQuote, we don't need to be so specific about it being a double quotation mark, do we?
+ bool *is_quoted) +{ + if (cmdline[0] == '"') { + *is_quoted = !(*is_quoted); + cmdline++; + } + return cmdline; +} + + +static size_t virKernelCmdlineSearchForward(const char *cmdline, + bool *is_quoted, + bool include_equal)
Hmm, what if instead we tried to find and return the index of the '=' character but iterated all the way until the next applicable (i.e. taking quotation into account) space and saved that end of arg/arg=val parameter into **res. The caller of this function would then continue directly from *res with the next arg/arg=val parameter. We could then call this something like virKernelCmdlineFindEqual and return -1 if there is no '=' sign, indicating that it's a standalone parameter with no value.
+{ + size_t index; + + for (index = 0; cmdline[index]; index++) { + if ((!(*is_quoted) && g_ascii_isspace(cmdline[index])) || + (include_equal && cmdline[index] == '=')) + break; + virKernelCmdlineSkipDbQuote(cmdline + index, is_quoted); + } + return index; +} + + +static size_t virKernelCmdlineNextSpace(const char *cmdline, + bool *is_quoted) +{ + return virKernelCmdlineSearchForward(cmdline, is_quoted, false); +} + + +static size_t virKernelCmdlineNextSpaceOrEqual(const char *cmdline, + bool *is_quoted) +{ + return virKernelCmdlineSearchForward(cmdline, is_quoted, true); +}
If we implement what I suggested above for virKernelCmdlineSearchForward, we won't need 2 wrappers for the same thing differentiating only in whether we do or do not need to search for the '=' sign as well.
+ + +static char* virKernelArgNormalize(const char *arg) +{ + return virStringReplace(arg, "_", "-"); +} + + +static char* virKernelCmdlineArgNormalize(const char *cmdline, size_t offset) +{ + g_autofree char *param = g_strndup((cmdline), offset); + + return virKernelArgNormalize(param);
See below for comments why we don't need this wrapper.
+} + + +/* + * Parse the kernel cmdline and store the next parameter in @param + * and the value of @param in @val which can be NULL if @param has + * no value. In addition returns the address right after @param=@value + * for possible further processing. + * + * @cmdline: kernel command line string to be checked for next parameter + * @param: pointer to hold retrieved parameter, will be NULL if none found + * @val: pointer to hold retrieved value of @param + * + * Returns a pointer to address right after @param=@val in the + * kernel command line, will point to the string's end (NULL) + * in case no next parameter is found + */ +const char *virKernelCmdlineNextParam(const char *cmdline, + char **param, + char **val) +{ + size_t offset; + bool is_quoted = false; + *param = NULL; + *val = NULL; + + virSkipSpaces(&cmdline); + cmdline = virKernelCmdlineSkipDbQuote(cmdline, &is_quoted); + offset = virKernelCmdlineNextSpaceOrEqual(cmdline, &is_quoted);
if you get the index of the equal sign, but iterate all the way until the end of the arg/arg=val parameter, you can use index and do: *param = g_strndup(cmdline, equal_index);
+ if (offset == 0)
If we used something like char **res (taking it as a parameter to this function) to represent the rest of the unparsed cmdline, then it should be NULL in this case, so the check could remain with a slight adjustment (I haven't implemented it, but I do hope it should work :))
+ return cmdline; + + *param = virKernelCmdlineArgNormalize(cmdline, offset);
^This normalization should be done in virKernelCmdlineMatchParam instead. That way, you won't need a wrapper over virKernelArgNormalize.
+ cmdline = cmdline + offset; + /* param has no value */ + if (*cmdline != '=') + return cmdline;
^This check could then be dropped along with moving the cursor in the cmdline string.
+ + cmdline = virKernelCmdlineSkipDbQuote(++cmdline, &is_quoted); + offset = virKernelCmdlineNextSpace(cmdline, &is_quoted);
If we do the above, we should be able to ditch ^this second loop searching for the next space.
+ if (cmdline[offset-1] == '"') + *val = g_strndup(cmdline, offset-1); + else + *val = g_strndup(cmdline, offset);
^Here you'd have to do arithmetics using the *res variable instead.
+ + return cmdline + offset;
We'd simply return *res;
+} + + +#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
I know you used it in the following patches to match against the accepted values, but do we really need to match with a prefix, I'd be fine with simple stirng equality matching in all cases and not mix the matching strategy for both values and kernel arguments.
+ * (uses size of value provided as input) + * - VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ: do a strict string comparison of values
^This should be default used in all cases IMO unless the prefix matching provides us with a considerable performance gain.
+ * - VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY: first positive match satifies search
is "sticky searching" some terminus technicus? If not, we probably should name this FLAGS_MATCH_FIRST and FLAGS_MATCH_LAST respectively.
+ * (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 *norm_arg = virKernelArgNormalize(arg); + g_autofree char *kparam = NULL; + g_autofree char *kval = NULL;
^These last two variables can be moved into the while loop, you won't need the explicit VIR_FREEs then.
+ + while (next[0] != '\0') { + VIR_FREE(kparam); + VIR_FREE(kval); + next = virKernelCmdlineNextParam(next, &kparam, &kval);
Insert a blank line in between all these "ifs" for better readability (I know the coding guideline we have doesn't mention it).
+ if (!kparam) + break;
You'd do the normalization of the parsed arg value here.
+ if (STRNEQ(kparam, norm_arg)) + continue; + 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..7499b78153 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -147,6 +147,23 @@ 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; + +const char *virKernelCmdlineNextParam(const char *cmdline, + char **param, + char **val); + +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..01fb8c89f5 100644 --- a/tests/utiltest.c +++ b/tests/utiltest.c @@ -254,6 +254,145 @@ testOverflowCheckMacro(const void *data G_GNUC_UNUSED) }
+struct testKernelCmdlineNextParamData +{ + const char *cmdline; + const char *param; + const char *val; + const char *next; +}; + +static struct testKernelCmdlineNextParamData kEntries[] = { + { "arg1 arg2 arg3=val1", "arg1", NULL, " arg2 arg3=val1" }, + { "arg1=val1 arg2 arg3=val3 arg4", "arg1", "val1", " arg2 arg3=val3 arg4" }, + { "arg3=val3 ", "arg3", "val3", " " }, + { "arg3=val3", "arg3", "val3", "" }, + { "arg-3=val3 arg4", "arg-3", "val3", " arg4" }, + { "arg_3=val3 arg4", "arg-3", "val3", " arg4" }, + { " arg_3=val3 arg4", "arg-3", "val3", " arg4" }, + { " arg-3=val3 arg4", "arg-3", "val3", " arg4" }, + { "arg2=\"value with spaces\" arg3=val3", "arg2", "value with spaces", " arg3=val3" }, + { " arg2=\"value with spaces\" arg3=val3", "arg2", "value with spaces", " arg3=val3" }, + { " \"arg2=value with spaces\" arg3=val3", "arg2", "value with spaces", " arg3=val3" }, + { "arg2=\"val\"ue arg3", "arg2", "val\"ue", " arg3" }, + { " arg3\" escaped=val2\"", "arg3\" escaped", "val2", "" },
^Is this even valid for the kernel itself? Looking at [1], they clearly don't allow escaped \" in the arg/value. [1] https://github.com/torvalds/linux/blob/db54615e21419c3cb4d699a0b0aa16cc44d0e...
+ { " arg2longer=someval arg2=val2 arg3 ", "arg2longer", "someval", " arg2=val2 arg3 " }, + { "=val1 arg2=val2", NULL, NULL, "=val1 arg2=val2" }, + { " ", NULL, NULL, "" }, + { "", NULL, NULL, "" }, +}; + +static int +testKernelCmdlineNextParam(const void *data G_GNUC_UNUSED) +{ + char *param = NULL; + char *val = NULL; + const char *next; + size_t i; + + for (i = 0; i < G_N_ELEMENTS(kEntries); ++i) { + VIR_FREE(param); + VIR_FREE(val); + + next = virKernelCmdlineNextParam(kEntries[i].cmdline, ¶m, &val); + + if (STRNEQ_NULLABLE(param, kEntries[i].param) || + STRNEQ_NULLABLE(val, kEntries[i].val) || + STRNEQ(next, kEntries[i].next)) { + VIR_TEST_DEBUG("\nKernel cmdline [%s]", kEntries[i].cmdline); + VIR_TEST_DEBUG("Expect param [%s]", kEntries[i].param); + VIR_TEST_DEBUG("Actual param [%s]", param); + VIR_TEST_DEBUG("Expect value [%s]", kEntries[i].val); + VIR_TEST_DEBUG("Actual value [%s]", val); + VIR_TEST_DEBUG("Expect next [%s]", kEntries[i].next); + VIR_TEST_DEBUG("Actual next [%s]", next); + + VIR_FREE(param); + VIR_FREE(val); + + return -1; + } + } + + VIR_FREE(param); + VIR_FREE(val); + + return 0; +}
I appreciate the thorough unit testing :) Erik

On 08/06/20 16:35, Erik Skultety wrote:
On Fri, May 29, 2020 at 12:10:03PM +0200, Paulo de Rezende Pinatti wrote:
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> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/libvirt_private.syms | 2 + src/util/virutil.c | 169 +++++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 17 ++++ tests/utiltest.c | 141 ++++++++++++++++++++++++++++++++ 4 files changed, 329 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a6af44fe1c..a206a943c5 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3433,6 +3433,8 @@ virHostGetDRMRenderNode; virHostHasIOMMU; virIndexToDiskName; virIsDevMapperDevice; +virKernelCmdlineMatchParam; +virKernelCmdlineNextParam; virMemoryLimitIsSet; virMemoryLimitTruncate; virMemoryMaxValue; diff --git a/src/util/virutil.c b/src/util/virutil.c index fb46501142..749c9d7116 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1725,6 +1725,175 @@ virHostGetDRMRenderNode(void) return ret; }
+ +static const char *virKernelCmdlineSkipDbQuote(const char *cmdline,
minor nitpick: we can call ^this simply ...SkipQuote, we don't need to be so specific about it being a double quotation mark, do we?
Sure, changed to SkipQuote.
+ bool *is_quoted) +{ + if (cmdline[0] == '"') { + *is_quoted = !(*is_quoted); + cmdline++; + } + return cmdline; +} + + +static size_t virKernelCmdlineSearchForward(const char *cmdline, + bool *is_quoted, + bool include_equal)
Hmm, what if instead we tried to find and return the index of the '=' character but iterated all the way until the next applicable (i.e. taking quotation into account) space and saved that end of arg/arg=val parameter into **res. The caller of this function would then continue directly from *res with the next arg/arg=val parameter. We could then call this something like virKernelCmdlineFindEqual and return -1 if there is no '=' sign, indicating that it's a standalone parameter with no value.
Yes, we can do that. It would look like this: size_t index; size_t equal_index = 0; for (index = 0; cmdline[index]; index++) { if (!(is_quoted) && g_ascii_isspace(cmdline[index])) break; if (equal_index == 0 && cmdline[index] == '=') { equal_index = index; continue; } virKernelCmdlineSkipQuote(cmdline + index, &is_quoted); } *res = cmdline + index; return equal_index; I found it more convenient to use 0 rather than -1 so that we can also handle the case when the argument itself starts with the equal sign.
+{ + size_t index; + + for (index = 0; cmdline[index]; index++) { + if ((!(*is_quoted) && g_ascii_isspace(cmdline[index])) || + (include_equal && cmdline[index] == '=')) + break; + virKernelCmdlineSkipDbQuote(cmdline + index, is_quoted); + } + return index; +} + + +static size_t virKernelCmdlineNextSpace(const char *cmdline, + bool *is_quoted) +{ + return virKernelCmdlineSearchForward(cmdline, is_quoted, false); +} + + +static size_t virKernelCmdlineNextSpaceOrEqual(const char *cmdline, + bool *is_quoted) +{ + return virKernelCmdlineSearchForward(cmdline, is_quoted, true); +}
If we implement what I suggested above for virKernelCmdlineSearchForward, we won't need 2 wrappers for the same thing differentiating only in whether we do or do not need to search for the '=' sign as well.
Yes, wrappers are gone.
+ + +static char* virKernelArgNormalize(const char *arg) +{ + return virStringReplace(arg, "_", "-"); +} + + +static char* virKernelCmdlineArgNormalize(const char *cmdline, size_t offset) +{ + g_autofree char *param = g_strndup((cmdline), offset); + + return virKernelArgNormalize(param);
See below for comments why we don't need this wrapper.
This wrapper is also gone.
+} + + +/* + * Parse the kernel cmdline and store the next parameter in @param + * and the value of @param in @val which can be NULL if @param has + * no value. In addition returns the address right after @param=@value + * for possible further processing. + * + * @cmdline: kernel command line string to be checked for next parameter + * @param: pointer to hold retrieved parameter, will be NULL if none found + * @val: pointer to hold retrieved value of @param + * + * Returns a pointer to address right after @param=@val in the + * kernel command line, will point to the string's end (NULL) + * in case no next parameter is found + */ +const char *virKernelCmdlineNextParam(const char *cmdline, + char **param, + char **val) +{ + size_t offset; + bool is_quoted = false; + *param = NULL; + *val = NULL; + + virSkipSpaces(&cmdline); + cmdline = virKernelCmdlineSkipDbQuote(cmdline, &is_quoted); + offset = virKernelCmdlineNextSpaceOrEqual(cmdline, &is_quoted);
if you get the index of the equal sign, but iterate all the way until the end of the arg/arg=val parameter, you can use index and do:
*param = g_strndup(cmdline, equal_index);
Yep, done.
+ if (offset == 0)
If we used something like char **res (taking it as a parameter to this function) to represent the rest of the unparsed cmdline, then it should be NULL in this case, so the check could remain with a slight adjustment (I haven't implemented it, but I do hope it should work :))
See below how it looks like now.
+ return cmdline; + + *param = virKernelCmdlineArgNormalize(cmdline, offset);
^This normalization should be done in virKernelCmdlineMatchParam instead. That way, you won't need a wrapper over virKernelArgNormalize.
Yes, done.
+ cmdline = cmdline + offset; + /* param has no value */ + if (*cmdline != '=') + return cmdline;
^This check could then be dropped along with moving the cursor in the cmdline string.
Yes, done.
+ + cmdline = virKernelCmdlineSkipDbQuote(++cmdline, &is_quoted); + offset = virKernelCmdlineNextSpace(cmdline, &is_quoted);
If we do the above, we should be able to ditch ^this second loop searching for the next space.
Yes, done.
+ if (cmdline[offset-1] == '"') + *val = g_strndup(cmdline, offset-1); + else + *val = g_strndup(cmdline, offset);
^Here you'd have to do arithmetics using the *res variable instead.
+ + return cmdline + offset;
We'd simply return *res;
So the function would look like this now: virSkipSpaces(&cmdline); cmdline = virKernelCmdlineSkipQuote(cmdline, &is_quoted); equal_index = virKernelCmdlineFindEqual(cmdline, is_quoted, &next); if (next == cmdline) return next; /* param has no value */ if (equal_index == 0) { if (is_quoted && next[-1] == '"') *param = g_strndup(cmdline, next - cmdline - 1); else *param = g_strndup(cmdline, next - cmdline); return next; } *param = g_strndup(cmdline, equal_index); if (cmdline[equal_index + 1] == '"') { is_quoted = true; equal_index++; } if (is_quoted && next[-1] == '"') *val = g_strndup(cmdline + equal_index + 1, next - cmdline - equal_index - 2); else *val = g_strndup(cmdline + equal_index + 1, next - cmdline - equal_index - 1); return next; There's still a bit of handling required due to the kernel's quoting rules. I took the liberty of using 'next' in place of 'res' as I think it conveys better its purpose. Let me know if that looks good to you.
+} + + +#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
I know you used it in the following patches to match against the accepted values, but do we really need to match with a prefix, I'd be fine with simple stirng equality matching in all cases and not mix the matching strategy for both values and kernel arguments.
The prefix based comparison is to make sure libvirt matches exactly like the kernel code does for the parameter prot_virt (see arch/s390/kernel/uv.c -> prot_virt_setup -> kstrtobool) which performs prefix based matching. Using equality matching should work for most cases but there would be certain corner cases missed (i.e. prot_virt=yfoo will be recognized as 'activate' by the kernel but not by libvirt). If you think such cases are not worth the trouble I can remove the PREFIX flag, let me know what you prefer.
+ * (uses size of value provided as input) + * - VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ: do a strict string comparison of values
^This should be default used in all cases IMO unless the prefix matching provides us with a considerable performance gain.
Yes, that makes sense. I made it the default now.
+ * - VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY: first positive match satifies search
is "sticky searching" some terminus technicus? If not, we probably should name this FLAGS_MATCH_FIRST and FLAGS_MATCH_LAST respectively.
I guess the flag explanation was not clear enough, sticky actually means "stop as soon as you find a match, no matter the order", so FLAGS_SEARCH_FIRST doesn't really fit here. A better name could be FLAGS_SEARCH_ANY. Let me know if you agree.
+ * (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 *norm_arg = virKernelArgNormalize(arg); + g_autofree char *kparam = NULL; + g_autofree char *kval = NULL;
^These last two variables can be moved into the while loop, you won't need the explicit VIR_FREEs then.
Done.
+ + while (next[0] != '\0') { + VIR_FREE(kparam); + VIR_FREE(kval); + next = virKernelCmdlineNextParam(next, &kparam, &kval);
Insert a blank line in between all these "ifs" for better readability (I know the coding guideline we have doesn't mention it).
Done.
+ if (!kparam) + break;
You'd do the normalization of the parsed arg value here.
Done.
+ if (STRNEQ(kparam, norm_arg)) + continue; + 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..7499b78153 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -147,6 +147,23 @@ 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; + +const char *virKernelCmdlineNextParam(const char *cmdline, + char **param, + char **val); + +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..01fb8c89f5 100644 --- a/tests/utiltest.c +++ b/tests/utiltest.c @@ -254,6 +254,145 @@ testOverflowCheckMacro(const void *data G_GNUC_UNUSED) }
+struct testKernelCmdlineNextParamData +{ + const char *cmdline; + const char *param; + const char *val; + const char *next; +}; + +static struct testKernelCmdlineNextParamData kEntries[] = { + { "arg1 arg2 arg3=val1", "arg1", NULL, " arg2 arg3=val1" }, + { "arg1=val1 arg2 arg3=val3 arg4", "arg1", "val1", " arg2 arg3=val3 arg4" }, + { "arg3=val3 ", "arg3", "val3", " " }, + { "arg3=val3", "arg3", "val3", "" }, + { "arg-3=val3 arg4", "arg-3", "val3", " arg4" }, + { "arg_3=val3 arg4", "arg-3", "val3", " arg4" }, + { " arg_3=val3 arg4", "arg-3", "val3", " arg4" }, + { " arg-3=val3 arg4", "arg-3", "val3", " arg4" }, + { "arg2=\"value with spaces\" arg3=val3", "arg2", "value with spaces", " arg3=val3" }, + { " arg2=\"value with spaces\" arg3=val3", "arg2", "value with spaces", " arg3=val3" }, + { " \"arg2=value with spaces\" arg3=val3", "arg2", "value with spaces", " arg3=val3" }, + { "arg2=\"val\"ue arg3", "arg2", "val\"ue", " arg3" }, + { " arg3\" escaped=val2\"", "arg3\" escaped", "val2", "" },
^Is this even valid for the kernel itself? Looking at [1], they clearly don't allow escaped \" in the arg/value.
[1] https://github.com/torvalds/linux/blob/db54615e21419c3cb4d699a0b0aa16cc44d0e...
I guess the word "escaped" in this test is a bit misleading; it's actually escaping the blank space, not the quote itself. This is valid for the kernel. In order to assure our parsing results would match those of the kernel I executed the code in cmdline.c in a standalone file to generate the reference values for the test. I'll change "arg3\" escaped" to "arg3\" with spaces" to clearly state the intention here.
+ { " arg2longer=someval arg2=val2 arg3 ", "arg2longer", "someval", " arg2=val2 arg3 " }, + { "=val1 arg2=val2", NULL, NULL, "=val1 arg2=val2" }, + { " ", NULL, NULL, "" }, + { "", NULL, NULL, "" }, +}; + +static int +testKernelCmdlineNextParam(const void *data G_GNUC_UNUSED) +{ + char *param = NULL; + char *val = NULL; + const char *next; + size_t i; + + for (i = 0; i < G_N_ELEMENTS(kEntries); ++i) { + VIR_FREE(param); + VIR_FREE(val); + + next = virKernelCmdlineNextParam(kEntries[i].cmdline, ¶m, &val); + + if (STRNEQ_NULLABLE(param, kEntries[i].param) || + STRNEQ_NULLABLE(val, kEntries[i].val) || + STRNEQ(next, kEntries[i].next)) { + VIR_TEST_DEBUG("\nKernel cmdline [%s]", kEntries[i].cmdline); + VIR_TEST_DEBUG("Expect param [%s]", kEntries[i].param); + VIR_TEST_DEBUG("Actual param [%s]", param); + VIR_TEST_DEBUG("Expect value [%s]", kEntries[i].val); + VIR_TEST_DEBUG("Actual value [%s]", val); + VIR_TEST_DEBUG("Expect next [%s]", kEntries[i].next); + VIR_TEST_DEBUG("Actual next [%s]", next); + + VIR_FREE(param); + VIR_FREE(val); + + return -1; + } + } + + VIR_FREE(param); + VIR_FREE(val); + + return 0; +}
I appreciate the thorough unit testing :)
:)
Erik
-- Best regards, Paulo de Rezende Pinatti

On 10/06/20 15:55, Paulo de Rezende Pinatti wrote:
+ { " arg3\" escaped=val2\"", "arg3\" escaped", "val2", "" },
^Is this even valid for the kernel itself? Looking at [1], they clearly don't allow escaped \" in the arg/value.
[1] https://github.com/torvalds/linux/blob/db54615e21419c3cb4d699a0b0aa16cc44d0e...
I guess the word "escaped" in this test is a bit misleading; it's actually escaping the blank space, not the quote itself. This is valid for the kernel. In order to assure our parsing results would match those of the kernel I executed the code in cmdline.c in a standalone file to generate the reference values for the test.
I'll change "arg3\" escaped" to "arg3\" with spaces" to clearly state the intention here.
I forgot to mention that while double checking the reference values I had to add a missing trailing double quote to the expected result value in order to match what the kernel produces for this case. So this is actually "val2\"". -- Best regards, Paulo de Rezende Pinatti

On Wed, Jun 10, 2020 at 03:55:14PM +0200, Paulo de Rezende Pinatti wrote:
On 08/06/20 16:35, Erik Skultety wrote:
On Fri, May 29, 2020 at 12:10:03PM +0200, Paulo de Rezende Pinatti wrote:
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> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/libvirt_private.syms | 2 + src/util/virutil.c | 169 +++++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 17 ++++ tests/utiltest.c | 141 ++++++++++++++++++++++++++++++++ 4 files changed, 329 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a6af44fe1c..a206a943c5 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3433,6 +3433,8 @@ virHostGetDRMRenderNode; virHostHasIOMMU; virIndexToDiskName; virIsDevMapperDevice; +virKernelCmdlineMatchParam; +virKernelCmdlineNextParam; virMemoryLimitIsSet; virMemoryLimitTruncate; virMemoryMaxValue; diff --git a/src/util/virutil.c b/src/util/virutil.c index fb46501142..749c9d7116 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1725,6 +1725,175 @@ virHostGetDRMRenderNode(void) return ret; }
+ +static const char *virKernelCmdlineSkipDbQuote(const char *cmdline,
minor nitpick: we can call ^this simply ...SkipQuote, we don't need to be so specific about it being a double quotation mark, do we?
Sure, changed to SkipQuote.
+ bool *is_quoted) +{ + if (cmdline[0] == '"') { + *is_quoted = !(*is_quoted); + cmdline++; + } + return cmdline; +} + + +static size_t virKernelCmdlineSearchForward(const char *cmdline, + bool *is_quoted, + bool include_equal)
Hmm, what if instead we tried to find and return the index of the '=' character but iterated all the way until the next applicable (i.e. taking quotation into account) space and saved that end of arg/arg=val parameter into **res. The caller of this function would then continue directly from *res with the next arg/arg=val parameter. We could then call this something like virKernelCmdlineFindEqual and return -1 if there is no '=' sign, indicating that it's a standalone parameter with no value.
Yes, we can do that. It would look like this:
size_t index;
nitpick: ^this is a standard iteration variable, so naming it "i" might look more "expected"
size_t equal_index = 0;
for (index = 0; cmdline[index]; index++) { if (!(is_quoted) && g_ascii_isspace(cmdline[index])) break; if (equal_index == 0 && cmdline[index] == '=') { equal_index = index; continue; } virKernelCmdlineSkipQuote(cmdline + index, &is_quoted); } *res = cmdline + index; return equal_index;
I found it more convenient to use 0 rather than -1 so that we can also handle the case when the argument itself starts with the equal sign.
Yes, that's fine, but please provide a doc string for this function mentioning this. ...
So the function would look like this now:
virSkipSpaces(&cmdline); cmdline = virKernelCmdlineSkipQuote(cmdline, &is_quoted); equal_index = virKernelCmdlineFindEqual(cmdline, is_quoted, &next);
if (next == cmdline) return next;
/* param has no value */ if (equal_index == 0) { if (is_quoted && next[-1] == '"') *param = g_strndup(cmdline, next - cmdline - 1); else *param = g_strndup(cmdline, next - cmdline); return next; }
*param = g_strndup(cmdline, equal_index);
if (cmdline[equal_index + 1] == '"') { is_quoted = true; equal_index++; }
if (is_quoted && next[-1] == '"') *val = g_strndup(cmdline + equal_index + 1, next - cmdline - equal_index - 2); else *val = g_strndup(cmdline + equal_index + 1, next - cmdline - equal_index - 1); return next;
There's still a bit of handling required due to the kernel's quoting rules. I took the liberty of using 'next' in place of 'res' as I think it conveys better its purpose. Let me know if that looks good to you.
That is fine, looks good.
+} + + +#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
I know you used it in the following patches to match against the accepted values, but do we really need to match with a prefix, I'd be fine with simple stirng equality matching in all cases and not mix the matching strategy for both values and kernel arguments.
The prefix based comparison is to make sure libvirt matches exactly like the kernel code does for the parameter prot_virt (see arch/s390/kernel/uv.c -> prot_virt_setup -> kstrtobool) which performs prefix based matching. Using equality matching should work for most cases but there would be certain corner cases missed (i.e. prot_virt=yfoo will be recognized as 'activate' by the kernel but not by libvirt).
That..is...interesting...
If you think such cases are not worth the trouble I can remove the PREFIX flag, let me know what you prefer.
Well, given that kernel supposedly recognizes "yfoo" as "on" just because of the prefix, we obviously can't neglect the PREFIX matching can we? If we did and someone indeed used such a horrible example of a cmdline, virt-host-validate would lie about the feature not being enabled (even though it would be) because libvirt would only do an equality match, so it has to stay. <reprove>bad kernel, bad kernel</reprove> Just to re-assure myself, that's a generic behavior used for all option parsing in the kernel not just prot_virt, right? If so, then contrary to my previous response, we should always use and default to prefix matching, because given the situation equality matching would clearly not be a satisfactory behaviour. ...
+ * - VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY: first positive match satifies search
is "sticky searching" some terminus technicus? If not, we probably should name this FLAGS_MATCH_FIRST and FLAGS_MATCH_LAST respectively.
I guess the flag explanation was not clear enough, sticky actually means "stop as soon as you find a match, no matter the order", so
Well, isn't "stop as soon as you find a match" == match first occurrence?
FLAGS_SEARCH_FIRST doesn't really fit here. A better name could be FLAGS_SEARCH_ANY. Let me know if you agree.
...
+struct testKernelCmdlineNextParamData +{ + const char *cmdline; + const char *param; + const char *val; + const char *next; +}; + +static struct testKernelCmdlineNextParamData kEntries[] = { + { "arg1 arg2 arg3=val1", "arg1", NULL, " arg2 arg3=val1" }, + { "arg1=val1 arg2 arg3=val3 arg4", "arg1", "val1", " arg2 arg3=val3 arg4" }, + { "arg3=val3 ", "arg3", "val3", " " }, + { "arg3=val3", "arg3", "val3", "" }, + { "arg-3=val3 arg4", "arg-3", "val3", " arg4" }, + { "arg_3=val3 arg4", "arg-3", "val3", " arg4" }, + { " arg_3=val3 arg4", "arg-3", "val3", " arg4" }, + { " arg-3=val3 arg4", "arg-3", "val3", " arg4" }, + { "arg2=\"value with spaces\" arg3=val3", "arg2", "value with spaces", " arg3=val3" }, + { " arg2=\"value with spaces\" arg3=val3", "arg2", "value with spaces", " arg3=val3" }, + { " \"arg2=value with spaces\" arg3=val3", "arg2", "value with spaces", " arg3=val3" }, + { "arg2=\"val\"ue arg3", "arg2", "val\"ue", " arg3" }, + { " arg3\" escaped=val2\"", "arg3\" escaped", "val2", "" },
^Is this even valid for the kernel itself? Looking at [1], they clearly don't allow escaped \" in the arg/value.
[1] https://github.com/torvalds/linux/blob/db54615e21419c3cb4d699a0b0aa16cc44d0e...
I guess the word "escaped" in this test is a bit misleading; it's actually escaping the blank space, not the quote itself. This is valid for the kernel. In order to assure our parsing results would match those of the kernel I executed the code in cmdline.c in a standalone file to generate the reference values for the test.
I'll change "arg3\" escaped" to "arg3\" with spaces" to clearly state the intention here.
Sure, that will help, although I wasn't relating to the word "escaped" in my reply. Nevermind though, my bad, I just couldn't wrap my head around seeing "val\"ue" and somehow could not make sense out of it, too much QE to process I guess, it's fine. Regards, Erik

On 11/06/20 10:09, Erik Skultety wrote:
On Wed, Jun 10, 2020 at 03:55:14PM +0200, Paulo de Rezende Pinatti wrote:
On 08/06/20 16:35, Erik Skultety wrote:
On Fri, May 29, 2020 at 12:10:03PM +0200, Paulo de Rezende Pinatti wrote:
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> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/libvirt_private.syms | 2 + src/util/virutil.c | 169 +++++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 17 ++++ tests/utiltest.c | 141 ++++++++++++++++++++++++++++++++ 4 files changed, 329 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a6af44fe1c..a206a943c5 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3433,6 +3433,8 @@ virHostGetDRMRenderNode; virHostHasIOMMU; virIndexToDiskName; virIsDevMapperDevice; +virKernelCmdlineMatchParam; +virKernelCmdlineNextParam; virMemoryLimitIsSet; virMemoryLimitTruncate; virMemoryMaxValue; diff --git a/src/util/virutil.c b/src/util/virutil.c index fb46501142..749c9d7116 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1725,6 +1725,175 @@ virHostGetDRMRenderNode(void) return ret; }
+ +static const char *virKernelCmdlineSkipDbQuote(const char *cmdline,
minor nitpick: we can call ^this simply ...SkipQuote, we don't need to be so specific about it being a double quotation mark, do we?
Sure, changed to SkipQuote.
+ bool *is_quoted) +{ + if (cmdline[0] == '"') { + *is_quoted = !(*is_quoted); + cmdline++; + } + return cmdline; +} + + +static size_t virKernelCmdlineSearchForward(const char *cmdline, + bool *is_quoted, + bool include_equal)
Hmm, what if instead we tried to find and return the index of the '=' character but iterated all the way until the next applicable (i.e. taking quotation into account) space and saved that end of arg/arg=val parameter into **res. The caller of this function would then continue directly from *res with the next arg/arg=val parameter. We could then call this something like virKernelCmdlineFindEqual and return -1 if there is no '=' sign, indicating that it's a standalone parameter with no value.
Yes, we can do that. It would look like this:
size_t index;
nitpick: ^this is a standard iteration variable, so naming it "i" might look more "expected"
Changed.
size_t equal_index = 0;
for (index = 0; cmdline[index]; index++) { if (!(is_quoted) && g_ascii_isspace(cmdline[index])) break; if (equal_index == 0 && cmdline[index] == '=') { equal_index = index; continue; } virKernelCmdlineSkipQuote(cmdline + index, &is_quoted); } *res = cmdline + index; return equal_index;
I found it more convenient to use 0 rather than -1 so that we can also handle the case when the argument itself starts with the equal sign.
Yes, that's fine, but please provide a doc string for this function mentioning this.
Will do.
...
So the function would look like this now:
virSkipSpaces(&cmdline); cmdline = virKernelCmdlineSkipQuote(cmdline, &is_quoted); equal_index = virKernelCmdlineFindEqual(cmdline, is_quoted, &next);
if (next == cmdline) return next;
/* param has no value */ if (equal_index == 0) { if (is_quoted && next[-1] == '"') *param = g_strndup(cmdline, next - cmdline - 1); else *param = g_strndup(cmdline, next - cmdline); return next; }
*param = g_strndup(cmdline, equal_index);
if (cmdline[equal_index + 1] == '"') { is_quoted = true; equal_index++; }
if (is_quoted && next[-1] == '"') *val = g_strndup(cmdline + equal_index + 1, next - cmdline - equal_index - 2); else *val = g_strndup(cmdline + equal_index + 1, next - cmdline - equal_index - 1); return next;
There's still a bit of handling required due to the kernel's quoting rules. I took the liberty of using 'next' in place of 'res' as I think it conveys better its purpose. Let me know if that looks good to you.
That is fine, looks good.
+} + + +#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
I know you used it in the following patches to match against the accepted values, but do we really need to match with a prefix, I'd be fine with simple stirng equality matching in all cases and not mix the matching strategy for both values and kernel arguments.
The prefix based comparison is to make sure libvirt matches exactly like the kernel code does for the parameter prot_virt (see arch/s390/kernel/uv.c -> prot_virt_setup -> kstrtobool) which performs prefix based matching. Using equality matching should work for most cases but there would be certain corner cases missed (i.e. prot_virt=yfoo will be recognized as 'activate' by the kernel but not by libvirt).
That..is...interesting...
If you think such cases are not worth the trouble I can remove the PREFIX flag, let me know what you prefer.
Well, given that kernel supposedly recognizes "yfoo" as "on" just because of the prefix, we obviously can't neglect the PREFIX matching can we? If we did and someone indeed used such a horrible example of a cmdline, virt-host-validate would lie about the feature not being enabled (even though it would be) because libvirt would only do an equality match, so it has to stay.
<reprove>bad kernel, bad kernel</reprove>
Just to re-assure myself, that's a generic behavior used for all option parsing in the kernel not just prot_virt, right? If so, then contrary to my previous response, we should always use and default to prefix matching, because given the situation equality matching would clearly not be a satisfactory behaviour.
No, actually other parameters might use equality matching, this is the case with 'mem_encrypt' (see arch/x86/mm/mem_encrypt_identity.c -> sme_enable -> strncmp at line 575) which was being checked for SEV in the first version of this series. I think we are fine with the current approach as we allow the caller to choose which matching strategy to use by specifying the appropriate flag.
...
+ * - VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY: first positive match satifies search
is "sticky searching" some terminus technicus? If not, we probably should name this FLAGS_MATCH_FIRST and FLAGS_MATCH_LAST respectively.
I guess the flag explanation was not clear enough, sticky actually means "stop as soon as you find a match, no matter the order", so
Well, isn't "stop as soon as you find a match" == match first occurrence?
I suppose one can also see that way :) I will thus use SEARCH_FIRST as requested.
FLAGS_SEARCH_FIRST doesn't really fit here. A better name could be FLAGS_SEARCH_ANY. Let me know if you agree.
...
+struct testKernelCmdlineNextParamData +{ + const char *cmdline; + const char *param; + const char *val; + const char *next; +}; + +static struct testKernelCmdlineNextParamData kEntries[] = { + { "arg1 arg2 arg3=val1", "arg1", NULL, " arg2 arg3=val1" }, + { "arg1=val1 arg2 arg3=val3 arg4", "arg1", "val1", " arg2 arg3=val3 arg4" }, + { "arg3=val3 ", "arg3", "val3", " " }, + { "arg3=val3", "arg3", "val3", "" }, + { "arg-3=val3 arg4", "arg-3", "val3", " arg4" }, + { "arg_3=val3 arg4", "arg-3", "val3", " arg4" }, + { " arg_3=val3 arg4", "arg-3", "val3", " arg4" }, + { " arg-3=val3 arg4", "arg-3", "val3", " arg4" }, + { "arg2=\"value with spaces\" arg3=val3", "arg2", "value with spaces", " arg3=val3" }, + { " arg2=\"value with spaces\" arg3=val3", "arg2", "value with spaces", " arg3=val3" }, + { " \"arg2=value with spaces\" arg3=val3", "arg2", "value with spaces", " arg3=val3" }, + { "arg2=\"val\"ue arg3", "arg2", "val\"ue", " arg3" }, + { " arg3\" escaped=val2\"", "arg3\" escaped", "val2", "" },
^Is this even valid for the kernel itself? Looking at [1], they clearly don't allow escaped \" in the arg/value.
[1] https://github.com/torvalds/linux/blob/db54615e21419c3cb4d699a0b0aa16cc44d0e...
I guess the word "escaped" in this test is a bit misleading; it's actually escaping the blank space, not the quote itself. This is valid for the kernel. In order to assure our parsing results would match those of the kernel I executed the code in cmdline.c in a standalone file to generate the reference values for the test.
I'll change "arg3\" escaped" to "arg3\" with spaces" to clearly state the intention here.
Sure, that will help, although I wasn't relating to the word "escaped" in my reply. Nevermind though, my bad, I just couldn't wrap my head around seeing "val\"ue" and somehow could not make sense out of it, too much QE to process I guess, it's fine.
Regards, Erik
-- Best regards, Paulo de Rezende Pinatti

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> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Tested-by: Viktor Mihajlovski <mihajlov@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@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 f12769635a..cbc577353b 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" @@ -657,6 +658,7 @@ struct _virQEMUCaps { virObject parent; bool kvmSupportsNesting; + bool kvmSupportsSecureGuest; char *binary; time_t ctime; @@ -1901,6 +1903,7 @@ virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr qemuCaps) ret->invalidation = qemuCaps->invalidation; ret->kvmSupportsNesting = qemuCaps->kvmSupportsNesting; + ret->kvmSupportsSecureGuest = qemuCaps->kvmSupportsSecureGuest; ret->ctime = qemuCaps->ctime; @@ -4396,6 +4399,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); @@ -4633,6 +4639,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"); @@ -4670,6 +4679,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", "on", "ON", "oN", "On", "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' @@ -4857,6 +4904,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; @@ -5349,6 +5403,8 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch, qemuCaps->kernelVersion = g_strdup(kernelVersion); qemuCaps->kvmSupportsNesting = virQEMUCapsKVMSupportsNesting(); + + qemuCaps->kvmSupportsSecureGuest = virQEMUCapsKVMSupportsSecureGuest(); } return qemuCaps; -- 2.25.4

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 /dev/sev exists Signed-off-by: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> --- src/qemu/qemu_capabilities.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index cbc577353b..0d19d4adff 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4702,6 +4702,24 @@ virQEMUCapsKVMSupportsSecureGuestS390(void) } +/* + * Check whether AMD Secure Encrypted Virtualization (x86) is enabled + */ +static bool +virQEMUCapsKVMSupportsSecureGuestAMD(void) +{ + g_autofree char *modValue = NULL; + + if (virFileReadValueString(&modValue, "/sys/module/kvm_amd/parameters/sev") < 0) + return false; + if (modValue[0] != '1') + return false; + if (virFileExists(QEMU_DEV_SEV)) + return true; + return false; +} + + /* * Check whether the secure guest functionality is enabled. * See the specific architecture function for details on the verifications made. @@ -4713,6 +4731,8 @@ virQEMUCapsKVMSupportsSecureGuest(void) if (ARCH_IS_S390(arch)) return virQEMUCapsKVMSupportsSecureGuestS390(); + if (ARCH_IS_X86(arch)) + return virQEMUCapsKVMSupportsSecureGuestAMD(); return false; } -- 2.25.4

On Fri, May 29, 2020 at 12:10:05PM +0200, Paulo de Rezende Pinatti wrote:
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 /dev/sev exists
Signed-off-by: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>

From: Boris Fiuczynski <fiuczy@linux.ibm.com> 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..8ead68798f 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", "on", "ON", "oN", "On", "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.4

On Fri, May 29, 2020 at 12:10:06PM +0200, Paulo de Rezende Pinatti wrote:
From: Boris Fiuczynski <fiuczy@linux.ibm.com>
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..8ead68798f 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", "on", "ON", "oN", "On", "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; + }
Empty line here...
+ if (virFileReadValueString(&cmdline, "/proc/cmdline") < 0) + return -1;
and here..
+ if (virKernelCmdlineMatchParam(cmdline, "prot_virt", kIBMValues, + G_N_ELEMENTS(kIBMValues), + VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY | + VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX)) {
Depending on whether we have an agreement on not needing to match according to PREFIX, this would have to be reworked, for the rest: Reviewed-by: Erik Skultety <eskultet@redhat.com>

From: Boris Fiuczynski <fiuczy@linux.ibm.com> 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> --- tools/virt-host-validate-common.c | 29 +++++++++++++++++++++++++++-- tools/virt-host-validate-common.h | 1 + 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index 8ead68798f..de007f2c43 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,18 @@ 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", "on", "ON", "oN", "On", "1"}; + 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 +489,27 @@ 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 (virFileExists("/dev/sev")) { + virHostMsgPass(); + return 1; + } else { + virHostMsgFail(level, + "AMD Secure Encrypted Virtualization appears to be " + "disabled in firemare."); + } } 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.4

On Fri, May 29, 2020 at 12:10:07PM +0200, Paulo de Rezende Pinatti wrote:
From: Boris Fiuczynski <fiuczy@linux.ibm.com>
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> --- tools/virt-host-validate-common.c | 29 +++++++++++++++++++++++++++-- tools/virt-host-validate-common.h | 1 + 2 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index 8ead68798f..de007f2c43 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,18 @@ int virHostValidateSecureGuests(const char *hvname, virHostValidateLevel level) { virBitmapPtr flags; - bool hasFac158 = false; + bool hasFac158 = false, hasAMDSev = false;
^one variable definition per line
virArch arch = virArchFromHost(); g_autofree char *cmdline = NULL; static const char *kIBMValues[] = {"y", "Y", "on", "ON", "oN", "On", "1"}; + 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 +489,27 @@ 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; + }
empty line here...
+ 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; + }
and here... Reviewed-by: Erik Skultety <eskultet@redhat.com>

From: Boris Fiuczynski <fiuczy@linux.ibm.com> Update document with changes in qemu capability caching and the added secure guest support checking for AMD SEV in virt-host-validate. Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- docs/kbase/launch_security_sev.rst | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/docs/kbase/launch_security_sev.rst b/docs/kbase/launch_security_sev.rst index 65f258587d..19b978481a 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.5.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: :: @@ -109,7 +112,7 @@ following: </features> </domainCapabilities> -Note that if libvirt was already installed and libvirtd running before +Note that if libvirt (<6.5.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: -- 2.25.4

On Fri, May 29, 2020 at 12:10:08PM +0200, Paulo de Rezende Pinatti wrote:
From: Boris Fiuczynski <fiuczy@linux.ibm.com>
Update document with changes in qemu capability caching and the added secure guest support checking for AMD SEV in virt-host-validate.
Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>

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..f38d16d743 --- /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.5.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.5.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.4

On Fri, May 29, 2020 at 12:10:09PM +0200, Paulo de Rezende Pinatti wrote:
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> ---
Boris said this went through a good internal review content-wise already and I don't see anything out of the ordinary from the outside :), so: Reviewed-by: Erik Skultety <eskultet@redhat.com>

Ping for reviews. On 29/05/20 12:10, Paulo de Rezende Pinatti wrote:
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 /dev/sev exists
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.
Changes in v2:
[Patch 1] Reworked kernel cmdline parser into a parameter based processing. [Patch 2] Added missing value "on" to kvalue list [Patch 3] Changed AMD SEV support check to module parameter is set and /dev/sev exists. Moved doc changes to a new standalone patch 6. [Patch 4] Added missing value "on" to kvalue list [Patch 5] Changed AMD SEV support check to align with patch 3. Moved doc changes to a new standalone patch 6. [Patch 6] Summarized AMD SEV doc changes from patches 3 and 5. Adjusted libvirt version number [Patch 7 (v1: Patch 6)] Adjusted libvirt version number
link to v1: https://www.redhat.com/archives/libvir-list/2020-May/msg00416.html
Boris Fiuczynski (3): tools: secure guest check on s390 in virt-host-validate tools: secure guest check for AMD in virt-host-validate docs: update AMD launch secure description
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 | 76 ++++++++++++ src/util/virutil.c | 169 ++++++++++++++++++++++++++ src/util/virutil.h | 17 +++ tests/utiltest.c | 141 +++++++++++++++++++++ tools/virt-host-validate-common.c | 83 ++++++++++++- tools/virt-host-validate-common.h | 5 + tools/virt-host-validate-qemu.c | 4 + 11 files changed, 693 insertions(+), 5 deletions(-) create mode 100644 docs/kbase/s390_protected_virt.rst
-- Best regards, Paulo de Rezende Pinatti
participants (3)
-
Bjoern Walk
-
Erik Skultety
-
Paulo de Rezende Pinatti