[libvirt] [v0.9.12-maint 0/8] Backport changes for CVE-2013-6458 to v0.9.12-maint
Hi, attached patches backport the fixes for CVE-2013-6458 to v0.9.12-maint. I decided to cherry-pick the introduction of VIR_STRDUP and virReportError as well to ease backporting of future fixes. I'd be happy about any review. Daniel P. Berrange (1): Introduce virReportError macro for general error reporting Eric Blake (2): string: make VIR_STRDUP easier to use string: test VIR_STRDUP Jiri Denemark (4): qemu: Do not access stale data in virDomainBlockStats qemu: Avoid using stale data in virDomainGetBlockInfo qemu: Fix job usage in qemuDomainBlockJobImpl qemu: Fix job usage in virDomainGetBlockIoTune Michal Privoznik (1): virstring: Introduce VIR_STRDUP and VIR_STRNDUP HACKING | 16 ++++++++ cfg.mk | 1 + docs/hacking.html.in | 24 ++++++++++- src/libvirt_private.syms | 2 + src/qemu/qemu_driver.c | 68 ++++++++++++++++--------------- src/util/virprocess.c | 3 -- src/util/virstring.c | 78 ++++++++++++++++++++++++++++++++++++ src/util/virstring.h | 70 ++++++++++++++++++++++++++++++++ src/util/virterror_internal.h | 3 ++ tests/virstringtest.c | 93 ++++++++++++++++++++++++++++++++++++++++++- 10 files changed, 320 insertions(+), 38 deletions(-) -- 1.8.5.2
From: Michal Privoznik <mprivozn@redhat.com> The code adaptation is not done right now, but in subsequent patches. Hence I am not implementing syntax-check rule as it would break compilation. Developers are strongly advised to use these new macros. They are similar to VIR_ALLOC() logic: VIR_STRDUP(dst, src) returns zero on success, -1 otherwise. In case you don't want to report OOM error, use the _QUIET variant of a macro. Conflicts: src/libvirt_private.syms src/util/virstring.h (cherry picked from commit c3abb5c45988a0d7583f059974513722d82e2c2b) --- HACKING | 11 +++++++ docs/hacking.html.in | 14 +++++++++ src/libvirt_private.syms | 2 ++ src/util/virstring.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virstring.h | 60 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 161 insertions(+) diff --git a/HACKING b/HACKING index 69ea96b..5b5c34b 100644 --- a/HACKING +++ b/HACKING @@ -535,6 +535,17 @@ sizeof(dest) returns something meaningful). Note that this is a macro, so arguments could be evaluated more than once. This is equivalent to virStrncpy(dest, src, strlen(src), sizeof(dest)). + VIR_STRDUP(char *dst, const char *src); + VIR_STRNDUP(char *dst, const char *src, size_t n); + +You should avoid using strdup or strndup directly as they do not report +out-of-memory error. Use VIR_STRDUP or VIR_STRNDUP macros instead. Note, that +these two behave similar to VIR_ALLOC: on success zero is returned, otherwise +the result is -1 and dst is guaranteed to be NULL. In very specific cases, +when you don't want to report the out-of-memory error, you can use +VIR_STRDUP_QUIET or VIR_STRNDUP_QUIET, but such usage is very rare and usually +considered a flaw. + Variable length string buffer ============================= diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 89f9980..198afe7 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -641,6 +641,20 @@ virStrncpy(dest, src, strlen(src), sizeof(dest)). </p> +<pre> + VIR_STRDUP(char *dst, const char *src); + VIR_STRNDUP(char *dst, const char *src, size_t n); +</pre> + <p> + You should avoid using strdup or strndup directly as they do not report + out-of-memory error. Use VIR_STRDUP or VIR_STRNDUP macros instead. Note, + that these two behave similar to VIR_ALLOC: on success zero is returned, + otherwise the result is -1 and dst is guaranteed to be NULL. In very + specific cases, when you don't want to report the out-of-memory error, you + can use VIR_STRDUP_QUIET or VIR_STRNDUP_QUIET, but such usage is very rare + and usually considered a flaw. + </p> + <h2><a name="strbuf">Variable length string buffer</a></h2> <p> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f35fd63..21b593a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1495,9 +1495,11 @@ virStrerror; # virstring.h +virStrdup; virStringSplit; virStringJoin; virStringFreeList; +virStrndup; # virtime.h diff --git a/src/util/virstring.c b/src/util/virstring.c index 92289eb..21142b4 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -177,3 +177,77 @@ size_t virStringListLength(char **strings) return i; } + +/** + * virStrdup: + * @dest: where to store duplicated string + * @src: the source string to duplicate + * @report: whether to report OOM error, if there is one + * @domcode: error domain code + * @filename: caller's filename + * @funcname: caller's funcname + * @linenr: caller's line number + * + * Wrapper over strdup, which reports OOM error if told so, + * in which case callers wants to pass @domcode, @filename, + * @funcname and @linenr which should represent location in + * caller's body where virStrdup is called from. Consider + * using VIR_STRDUP which sets these automatically. + * + * Returns: 0 on success, -1 otherwise. + */ +int +virStrdup(char **dest, + const char *src, + bool report, + int domcode, + const char *filename, + const char *funcname, + size_t linenr) +{ + if (!(*dest = strdup(src))) { + if (report) + virReportOOMErrorFull(domcode, filename, funcname, linenr); + return -1; + } + + return 0; +} + +/** + * virStrndup: + * @dest: where to store duplicated string + * @src: the source string to duplicate + * @n: how many bytes to copy + * @report: whether to report OOM error, if there is one + * @domcode: error domain code + * @filename: caller's filename + * @funcname: caller's funcname + * @linenr: caller's line number + * + * Wrapper over strndup, which reports OOM error if told so, + * in which case callers wants to pass @domcode, @filename, + * @funcname and @linenr which should represent location in + * caller's body where virStrndup is called from. Consider + * using VIR_STRNDUP which sets these automatically. + * + * Returns: 0 on success, -1 otherwise. + */ +int +virStrndup(char **dest, + const char *src, + size_t n, + bool report, + int domcode, + const char *filename, + const char *funcname, + size_t linenr) +{ + if (!(*dest = strndup(src, n))) { + if (report) + virReportOOMErrorFull(domcode, filename, funcname, linenr); + return -1; + } + + return 0; +} diff --git a/src/util/virstring.h b/src/util/virstring.h index d68ed2f..cd5ccda 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -37,4 +37,64 @@ void virStringFreeList(char **strings); size_t virStringListLength(char **strings); +/* Don't call these directly - use the macros below */ +int virStrdup(char **dest, const char *src, bool report, int domcode, + const char *filename, const char *funcname, size_t linenr) + ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + +int virStrndup(char **dest, const char *src, size_t n, bool report, int domcode, + const char *filename, const char *funcname, size_t linenr) + ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + +/** + * VIR_STRDUP: + * @dst: variable to hold result (char*, not char**) + * @src: string to duplicate + * + * Duplicate @src string and store it into @dst. + * + * Returns -1 on failure (with OOM error reported), 0 on success + */ +# define VIR_STRDUP(dst, src) virStrdup(&(dst), src, true, VIR_FROM_THIS, \ + __FILE__, __FUNCTION__, __LINE__) + +/** + * VIR_STRDUP_QUIET: + * @dst: variable to hold result (char*, not char**) + * @src: string to duplicate + * + * Duplicate @src string and store it into @dst. + * + * Returns -1 on failure, 0 on success + */ +# define VIR_STRDUP_QUIET(dst, src) virStrdup(&(dst), src, false, 0, NULL, NULL, 0) + +/** + * VIR_STRNDUP: + * @dst: variable to hold result (char*, not char**) + * @src: string to duplicate + * @n: the maximum number of bytes to copy + * + * Duplicate @src string and store it into @dst. If @src is longer than @n, + * only @n bytes are copied and terminating null byte '\0' is added. + * + * Returns -1 on failure (with OOM error reported), 0 on success + */ +# define VIR_STRNDUP(dst, src, n) virStrndup(&(dst), src, n, true, \ + VIR_FROM_THIS, __FILE__, \ + __FUNCTION__, __LINE__) + +/** + * VIR_STRNDUP_QUIET: + * @dst: variable to hold result (char*, not char**) + * @src: string to duplicate + * @n: the maximum number of bytes to copy + * + * Duplicate @src string and store it into @dst. If @src is longer than @n, + * only @n bytes are copied and terminating null byte '\0' is added. + * + * Returns -1 on failure, 0 on success + */ +# define VIR_STRNDUP_QUIET(dst, src, n) virStrndup(&(dst), src, n, false, \ + 0, NULL, NULL, 0) #endif /* __VIR_STRING_H__ */ -- 1.8.5.2
From: Eric Blake <eblake@redhat.com> While reviewing proposed VIR_STRDUP conversions, I've already noticed several places that do: if (str && VIR_STRDUP(dest, str) < 0) which can be simplified by allowing str to be NULL (something that strdup() doesn't allow). Meanwhile, code that wants to ensure a non-NULL dest regardless of the source can check for <= 0. Also, make it part of the VIR_STRDUP contract that macro arguments are evaluated exactly once. * src/util/virstring.h (VIR_STRDUP, VIR_STRDUP_QUIET, VIR_STRNDUP) (VIR_STRNDUP_QUIET): Improve contract. * src/util/virstring.c (virStrdup, virStrndup): Change return conventions. * docs/hacking.html.in: Document this. * HACKING: Regenerate. Signed-off-by: Eric Blake <eblake@redhat.com> Conflicts: HACKING docs/hacking.html.in (cherry picked from commit 6b74a9f5d98e066f8dfdf5d5ccda68230b516246) --- HACKING | 17 +++++++++++------ docs/hacking.html.in | 16 ++++++++++++---- src/util/virstring.c | 12 ++++++++---- src/util/virstring.h | 22 ++++++++++++++++------ 4 files changed, 47 insertions(+), 20 deletions(-) diff --git a/HACKING b/HACKING index 5b5c34b..758d327 100644 --- a/HACKING +++ b/HACKING @@ -234,6 +234,11 @@ But if negating a complex condition is too ugly, then at least add braces: Preprocessor ============ +Macros defined with an ALL_CAPS name should generally be assumed to be unsafe +with regards to arguments with side-effects (that is, MAX(a++, b--) might +increment a or decrement b too many or too few times). Exceptions to this rule +are explicitly documented for macros in viralloc.h and virstring.h. + For variadic macros, stick with C99 syntax: #define vshPrint(_ctl, ...) fprintf(stdout, __VA_ARGS__) @@ -539,12 +544,12 @@ virStrncpy(dest, src, strlen(src), sizeof(dest)). VIR_STRNDUP(char *dst, const char *src, size_t n); You should avoid using strdup or strndup directly as they do not report -out-of-memory error. Use VIR_STRDUP or VIR_STRNDUP macros instead. Note, that -these two behave similar to VIR_ALLOC: on success zero is returned, otherwise -the result is -1 and dst is guaranteed to be NULL. In very specific cases, -when you don't want to report the out-of-memory error, you can use -VIR_STRDUP_QUIET or VIR_STRNDUP_QUIET, but such usage is very rare and usually -considered a flaw. +out-of-memory error, and do not allow a NULL source. Use VIR_STRDUP or +VIR_STRNDUP macros instead, which return 0 for NULL source, 1 for successful +copy, and -1 for allocation failure with the error already reported. In very +specific cases, when you don't want to report the out-of-memory error, you can +use VIR_STRDUP_QUIET or VIR_STRNDUP_QUIET, but such usage is very rare and +usually considered a flaw. Variable length string buffer diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 198afe7..d553f42 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -303,7 +303,14 @@ } </pre> - <h2><a href="types">Preprocessor</a></h2> + <h2><a name="preprocessor">Preprocessor</a></h2> + + <p>Macros defined with an ALL_CAPS name should generally be + assumed to be unsafe with regards to arguments with side-effects + (that is, MAX(a++, b--) might increment a or decrement b too + many or too few times). Exceptions to this rule are explicitly + documented for macros in viralloc.h and virstring.h. + </p> <p> For variadic macros, stick with C99 syntax: @@ -647,9 +654,10 @@ </pre> <p> You should avoid using strdup or strndup directly as they do not report - out-of-memory error. Use VIR_STRDUP or VIR_STRNDUP macros instead. Note, - that these two behave similar to VIR_ALLOC: on success zero is returned, - otherwise the result is -1 and dst is guaranteed to be NULL. In very + out-of-memory error, and do not allow a NULL source. Use + VIR_STRDUP or VIR_STRNDUP macros instead, which return 0 for + NULL source, 1 for successful copy, and -1 for allocation + failure with the error already reported. In very specific cases, when you don't want to report the out-of-memory error, you can use VIR_STRDUP_QUIET or VIR_STRNDUP_QUIET, but such usage is very rare and usually considered a flaw. diff --git a/src/util/virstring.c b/src/util/virstring.c index 21142b4..915c771 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -194,7 +194,7 @@ size_t virStringListLength(char **strings) * caller's body where virStrdup is called from. Consider * using VIR_STRDUP which sets these automatically. * - * Returns: 0 on success, -1 otherwise. + * Returns: 0 for NULL src, 1 on successful copy, -1 otherwise. */ int virStrdup(char **dest, @@ -205,13 +205,15 @@ virStrdup(char **dest, const char *funcname, size_t linenr) { + if (!src) + return 0; if (!(*dest = strdup(src))) { if (report) virReportOOMErrorFull(domcode, filename, funcname, linenr); return -1; } - return 0; + return 1; } /** @@ -231,7 +233,7 @@ virStrdup(char **dest, * caller's body where virStrndup is called from. Consider * using VIR_STRNDUP which sets these automatically. * - * Returns: 0 on success, -1 otherwise. + * Returns: 0 for NULL src, 1 on successful copy, -1 otherwise. */ int virStrndup(char **dest, @@ -243,11 +245,13 @@ virStrndup(char **dest, const char *funcname, size_t linenr) { + if (!src) + return 0; if (!(*dest = strndup(src, n))) { if (report) virReportOOMErrorFull(domcode, filename, funcname, linenr); return -1; } - return 0; + return 1; } diff --git a/src/util/virstring.h b/src/util/virstring.h index cd5ccda..74164bf 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -40,11 +40,11 @@ size_t virStringListLength(char **strings); /* Don't call these directly - use the macros below */ int virStrdup(char **dest, const char *src, bool report, int domcode, const char *filename, const char *funcname, size_t linenr) - ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1); int virStrndup(char **dest, const char *src, size_t n, bool report, int domcode, const char *filename, const char *funcname, size_t linenr) - ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1); /** * VIR_STRDUP: @@ -53,7 +53,10 @@ int virStrndup(char **dest, const char *src, size_t n, bool report, int domcode, * * Duplicate @src string and store it into @dst. * - * Returns -1 on failure (with OOM error reported), 0 on success + * This macro is safe to use on arguments with side effects. + * + * Returns -1 on failure (with OOM error reported), 0 if @src was NULL, + * 1 if @src was copied */ # define VIR_STRDUP(dst, src) virStrdup(&(dst), src, true, VIR_FROM_THIS, \ __FILE__, __FUNCTION__, __LINE__) @@ -65,7 +68,9 @@ int virStrndup(char **dest, const char *src, size_t n, bool report, int domcode, * * Duplicate @src string and store it into @dst. * - * Returns -1 on failure, 0 on success + * This macro is safe to use on arguments with side effects. + * + * Returns -1 on failure, 0 if @src was NULL, 1 if @src was copied */ # define VIR_STRDUP_QUIET(dst, src) virStrdup(&(dst), src, false, 0, NULL, NULL, 0) @@ -78,7 +83,10 @@ int virStrndup(char **dest, const char *src, size_t n, bool report, int domcode, * Duplicate @src string and store it into @dst. If @src is longer than @n, * only @n bytes are copied and terminating null byte '\0' is added. * - * Returns -1 on failure (with OOM error reported), 0 on success + * This macro is safe to use on arguments with side effects. + * + * Returns -1 on failure (with OOM error reported), 0 if @src was NULL, + * 1 if @src was copied */ # define VIR_STRNDUP(dst, src, n) virStrndup(&(dst), src, n, true, \ VIR_FROM_THIS, __FILE__, \ @@ -93,7 +101,9 @@ int virStrndup(char **dest, const char *src, size_t n, bool report, int domcode, * Duplicate @src string and store it into @dst. If @src is longer than @n, * only @n bytes are copied and terminating null byte '\0' is added. * - * Returns -1 on failure, 0 on success + * This macro is safe to use on arguments with side effects. + * + * Returns -1 on failure, 0 if @src was NULL, 1 if @src was copied */ # define VIR_STRNDUP_QUIET(dst, src, n) virStrndup(&(dst), src, n, false, \ 0, NULL, NULL, 0) -- 1.8.5.2
From: Eric Blake <eblake@redhat.com> The surest way to avoid regressions is to test documented behavior :) * tests/virstringtest.c (testStrdup): New test case. Signed-off-by: Eric Blake <eblake@redhat.com> (cherry picked from commit 504b4a8dae06330ba1735a28c316c1c68a32c471) --- tests/virstringtest.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 92 insertions(+), 1 deletion(-) diff --git a/tests/virstringtest.c b/tests/virstringtest.c index 7e726c6..aca79cd 100644 --- a/tests/virstringtest.c +++ b/tests/virstringtest.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2012 Red Hat, Inc. + * Copyright (C) 2012-2013 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -108,6 +108,95 @@ cleanup: return ret; } +static bool fail; + +static const char * +testStrdupLookup1(size_t i) +{ + switch (i) { + case 0: + return "hello"; + case 1: + return NULL; + default: + fail = true; + return "oops"; + } +} + +static size_t +testStrdupLookup2(size_t i) +{ + if (i) + fail = true; + return 5; +} + +static int +testStrdup(const void *data ATTRIBUTE_UNUSED) +{ + char *array[] = { NULL, NULL }; + size_t i = 0; + size_t j = 0; + size_t k = 0; + int ret = -1; + int value; + + value = VIR_STRDUP(array[i++], testStrdupLookup1(j++)); + if (value != 1) { + fprintf(stderr, "unexpected strdup result %d, expected 1\n", value); + goto cleanup; + } + if (i != 1) { + fprintf(stderr, "unexpected side effects i=%zu, expected 1\n", i); + goto cleanup; + } + if (j != 1) { + fprintf(stderr, "unexpected side effects j=%zu, expected 1\n", j); + goto cleanup; + } + if (STRNEQ_NULLABLE(array[0], "hello") || array[1]) { + fprintf(stderr, "incorrect array contents '%s' '%s'\n", + NULLSTR(array[0]), NULLSTR(array[1])); + goto cleanup; + } + + value = VIR_STRNDUP(array[i++], testStrdupLookup1(j++), + testStrdupLookup2(k++)); + if (value != 0) { + fprintf(stderr, "unexpected strdup result %d, expected 0\n", value); + goto cleanup; + } + if (i != 2) { + fprintf(stderr, "unexpected side effects i=%zu, expected 2\n", i); + goto cleanup; + } + if (j != 2) { + fprintf(stderr, "unexpected side effects j=%zu, expected 2\n", j); + goto cleanup; + } + if (k != 1) { + fprintf(stderr, "unexpected side effects k=%zu, expected 1\n", k); + goto cleanup; + } + if (STRNEQ_NULLABLE(array[0], "hello") || array[1]) { + fprintf(stderr, "incorrect array contents '%s' '%s'\n", + NULLSTR(array[0]), NULLSTR(array[1])); + goto cleanup; + } + + if (fail) { + fprintf(stderr, "side effects failed\n"); + goto cleanup; + } + + ret = 0; +cleanup: + for (i = 0; i < ARRAY_CARDINALITY(array); i++) + VIR_FREE(array[i]); + return ret; +} + static int mymain(void) @@ -154,6 +243,8 @@ mymain(void) const char *tokens7[] = { "The", "quick", "brown", "fox", "", NULL }; TEST_SPLIT("The quick brown fox ", " ", 0, tokens7); + if (virtTestRun("strdup", 1, testStrdup, NULL) < 0) + ret = -1; return ret==0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 1.8.5.2
From: "Daniel P. Berrange" <berrange@redhat.com> Nearly every source file does something like #define VIR_FROM_THIS VIR_FROM_FOO #define virFooReportErorr(code, ...) \ virReportErrorHelper(VIR_FROM_THIS, code, __FILE__, \ __FUNCTION__, __LINE__, \ __VA_ARGS__) This creates needless duplication and inconsistent error reporting function names in each file. It is trivial to just have virterror_internal.h provide a virReportError macro that is equivalent * src/util/virterror_internal.h: Define virReportError(code, ...) Signed-off-by: Daniel P. Berrange <berrange@redhat.com> (cherry picked from commit 7e94acd4fc470ea46ab94f21f7fe718d512ca3b1) --- cfg.mk | 1 + src/util/virprocess.c | 3 --- src/util/virterror_internal.h | 3 +++ 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/cfg.mk b/cfg.mk index 67141a9..80fbd14 100644 --- a/cfg.mk +++ b/cfg.mk @@ -560,6 +560,7 @@ msg_gen_function += virNetworkReportError msg_gen_function += virNodeDeviceReportError msg_gen_function += virNWFilterReportError msg_gen_function += virRaiseError +msg_gen_function += virReportError msg_gen_function += virReportErrorHelper msg_gen_function += virReportSystemError msg_gen_function += virSecretReportError diff --git a/src/util/virprocess.c b/src/util/virprocess.c index f05f8f3..9f5e093 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -40,9 +40,6 @@ #include "util.h" #include "virstring.h" #include "ignore-value.h" -#define virReportError(code, ...) \ - virReportErrorHelper(VIR_FROM_NONE, code, __FILE__, \ - __FUNCTION__, __LINE__, __VA_ARGS__) #define VIR_FROM_THIS VIR_FROM_NONE diff --git a/src/util/virterror_internal.h b/src/util/virterror_internal.h index b8cb279..2e8aaf8 100644 --- a/src/util/virterror_internal.h +++ b/src/util/virterror_internal.h @@ -76,6 +76,9 @@ void virReportOOMErrorFull(int domcode, # define virReportOOMError() \ virReportOOMErrorFull(VIR_FROM_THIS, __FILE__, __FUNCTION__, __LINE__) +# define virReportError(code, ...) \ + virReportErrorHelper(VIR_FROM_THIS, code, __FILE__, \ + __FUNCTION__, __LINE__, __VA_ARGS__) int virSetError(virErrorPtr newerr); void virDispatchError(virConnectPtr conn); -- 1.8.5.2
From: Jiri Denemark <jdenemar@redhat.com> CVE-2013-6458 https://bugzilla.redhat.com/show_bug.cgi?id=1043069 When virDomainDetachDeviceFlags is called concurrently to virDomainBlockStats: libvirtd may crash because qemuDomainBlockStats finds a disk in vm->def before getting a job on a domain and uses the disk pointer after getting the job. However, the domain in unlocked while waiting on a job condition and thus data behind the disk pointer may disappear. This happens when thread 1 runs virDomainDetachDeviceFlags and enters monitor to actually remove the disk. Then another thread starts running virDomainBlockStats, finds the disk in vm->def, and while it's waiting on the job condition (owned by the first thread), the first thread finishes the disk removal. When the second thread gets the job, the memory pointed to be the disk pointer is already gone. That said, every API that is going to begin a job should do that before fetching data from vm->def. Conflicts: src/qemu/qemu_driver.c (cherry picked from commit db86da5ca2109e4006c286a09b6c75bfe10676ad) --- src/qemu/qemu_driver.c | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c0b4707..3f46c10 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7604,34 +7604,29 @@ qemuDomainBlockStats(virDomainPtr dom, goto cleanup; } - if (!virDomainObjIsActive(vm)) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("domain is not running")); + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) goto cleanup; + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto endjob; } if ((i = virDomainDiskIndexByName(vm->def, path, false)) < 0) { - qemuReportError(VIR_ERR_INVALID_ARG, - _("invalid path: %s"), path); - goto cleanup; + virReportError(VIR_ERR_INVALID_ARG, + _("invalid path: %s"), path); + goto endjob; } disk = vm->def->disks[i]; if (!disk->info.alias) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("missing disk device alias name for %s"), disk->dst); - goto cleanup; + virReportError(VIR_ERR_INTERNAL_ERROR, + _("missing disk device alias name for %s"), disk->dst); + goto endjob; } priv = vm->privateData; - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) - goto cleanup; - - if (!virDomainObjIsActive(vm)) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("domain is not running")); - goto endjob; - } qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorGetBlockStatsInfo(priv->mon, -- 1.8.5.2
From: Jiri Denemark <jdenemar@redhat.com> CVE-2013-6458 Generally, every API that is going to begin a job should do that before fetching data from vm->def. However, qemuDomainGetBlockInfo does not know whether it will have to start a job or not before checking vm->def. To avoid using disk alias that might have been freed while we were waiting for a job, we use its copy. In case the disk was removed in the meantime, we will fail with "cannot find statistics for device '...'" error message. Conflicts: src/qemu/qemu_driver.c (cherry picked from commit b799259583bd65c0b2f5042e6c3ff19637ade881) --- src/qemu/qemu_driver.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3f46c10..b17aa09 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -92,6 +92,7 @@ #include "virnodesuspend.h" #include "virtime.h" #include "virtypedparam.h" +#include "virstring.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -8409,10 +8410,12 @@ cleanup: } -static int qemuDomainGetBlockInfo(virDomainPtr dom, - const char *path, - virDomainBlockInfoPtr info, - unsigned int flags) { +static int +qemuDomainGetBlockInfo(virDomainPtr dom, + const char *path, + virDomainBlockInfoPtr info, + unsigned int flags) +{ struct qemud_driver *driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; @@ -8423,6 +8426,7 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom, struct stat sb; int i; int format; + char *alias = NULL; virCheckFlags(0, -1); @@ -8545,13 +8549,16 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom, virDomainObjIsActive(vm)) { qemuDomainObjPrivatePtr priv = vm->privateData; + if (VIR_STRDUP(alias, disk->info.alias) < 0) + goto cleanup; + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) goto cleanup; if (virDomainObjIsActive(vm)) { qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorGetBlockExtent(priv->mon, - disk->info.alias, + alias, &info->allocation); qemuDomainObjExitMonitor(driver, vm); } else { @@ -8565,6 +8572,7 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom, } cleanup: + VIR_FREE(alias); virStorageFileFreeMetadata(meta); VIR_FORCE_CLOSE(fd); if (vm) -- 1.8.5.2
From: Jiri Denemark <jdenemar@redhat.com> CVE-2013-6458 Every API that is going to begin a job should do that before fetching data from vm->def. Conflicts: src/qemu/qemu_driver.c (cherry picked from commit f93d2caa070f6197ab50d372d286018b0ba6bbd8) --- src/qemu/qemu_driver.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b17aa09..f810275 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11749,11 +11749,6 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char *path, const char *base, goto cleanup; } - device = qemuDiskPathToAlias(vm, path, &idx); - if (!device) - goto cleanup; - disk = vm->def->disks[idx]; - if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; @@ -11763,6 +11758,11 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char *path, const char *base, goto endjob; } + device = qemuDiskPathToAlias(vm, path, &idx); + if (!device) + goto endjob; + disk = vm->def->disks[idx]; + qemuDomainObjEnterMonitorWithDriver(driver, vm); /* XXX - libvirt should really be tracking the backing file chain * itself, and validating that base is on the chain, rather than -- 1.8.5.2
From: Jiri Denemark <jdenemar@redhat.com> CVE-2013-6458 Every API that is going to begin a job should do that before fetching data from vm->def. Conflicts: src/qemu/qemu_driver.c (cherry picked from commit 3b56425938e2f97208d5918263efa0d6439e4ecd) --- src/qemu/qemu_driver.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f810275..30b703a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12157,12 +12157,6 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, goto cleanup; } - device = qemuDiskPathToAlias(vm, disk, NULL); - - if (!device) { - goto cleanup; - } - if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; @@ -12170,6 +12164,11 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, &persistentDef) < 0) goto endjob; + device = qemuDiskPathToAlias(vm, disk, NULL); + if (!device) { + goto endjob; + } + if (flags & VIR_DOMAIN_AFFECT_LIVE) { priv = vm->privateData; qemuDomainObjEnterMonitorWithDriver(driver, vm); -- 1.8.5.2
On 01/11/2014 07:27 AM, Guido Günther wrote:
Hi, attached patches backport the fixes for CVE-2013-6458 to v0.9.12-maint. I decided to cherry-pick the introduction of VIR_STRDUP and virReportError as well to ease backporting of future fixes. I'd be happy about any review.
Looks correct to me. I'll let you push to 0.9.12-maint since you already did that work; I already pushed to all the branches 0.10.2 and later. When porting to 0.10.2, I chose to just inline the call to strdup() instead of backporting VIR_STRDUP, for fewer patches but more conflict resolution; but either approach seems acceptable. Is anyone still using v0.9.11-maint? The CVE extends back to 0.9.8, so we could argue that we should either fix the 0.9.11 branch, or add another commit to the branch that explicitly marks it as end-of-life because no one appears to be relying on it. Fedora 18 is now end-of-life, so from Fedora's perspective, I only care about 0.10.2 (RHEL and CentOS 6), 1.0.5 (F19), 1.1.3 (F20) and soon 1.2.1 (rawhide), although I didn't mind touching all the intermediate branches on my way down to 0.10.2. RHEL 5 is also vulnerable to CVE-2013-6458, but as we don't have an upstream v0.8.2-maint branch (thank goodness!), that's something for Red Hat to worry about. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/15/2014 01:43 PM, Eric Blake wrote:
On 01/11/2014 07:27 AM, Guido Günther wrote:
Hi, attached patches backport the fixes for CVE-2013-6458 to v0.9.12-maint. I decided to cherry-pick the introduction of VIR_STRDUP and virReportError as well to ease backporting of future fixes. I'd be happy about any review.
Looks correct to me. I'll let you push to 0.9.12-maint since you already did that work; I already pushed to all the branches 0.10.2 and later. When porting to 0.10.2, I chose to just inline the call to strdup() instead of backporting VIR_STRDUP, for fewer patches but more conflict resolution; but either approach seems acceptable.
Oh, and I also pushed the two patches for CVE-2014-1447 to all branches back to 0.10.2. Since that also exists in 0.9.8, you'll want to include those two patches in your push to 0.9.12. There's a conflict resolution needed in the first of the two patches, if you want to borrow from 0.10.2-maint. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Wed, Jan 15, 2014 at 01:47:48PM -0700, Eric Blake wrote:
On 01/15/2014 01:43 PM, Eric Blake wrote:
On 01/11/2014 07:27 AM, Guido Günther wrote:
Hi, attached patches backport the fixes for CVE-2013-6458 to v0.9.12-maint. I decided to cherry-pick the introduction of VIR_STRDUP and virReportError as well to ease backporting of future fixes. I'd be happy about any review.
Looks correct to me. I'll let you push to 0.9.12-maint since you already did that work; I already pushed to all the branches 0.10.2 and later. When porting to 0.10.2, I chose to just inline the call to strdup() instead of backporting VIR_STRDUP, for fewer patches but more conflict resolution; but either approach seems acceptable.
Oh, and I also pushed the two patches for CVE-2014-1447 to all branches back to 0.10.2. Since that also exists in 0.9.8, you'll want to include those two patches in your push to 0.9.12. There's a conflict resolution needed in the first of the two patches, if you want to borrow from 0.10.2-maint.
I've cherry-picked these too and will tag a 0.9.12.3 during the next days. Thanks a lot! -- Guido
On Wed, Jan 15, 2014 at 01:43:54PM -0700, Eric Blake wrote:
On 01/11/2014 07:27 AM, Guido Günther wrote:
Hi, attached patches backport the fixes for CVE-2013-6458 to v0.9.12-maint. I decided to cherry-pick the introduction of VIR_STRDUP and virReportError as well to ease backporting of future fixes. I'd be happy about any review.
Looks correct to me. I'll let you push to 0.9.12-maint since you already did that work; I already pushed to all the branches 0.10.2 and later. When porting to 0.10.2, I chose to just inline the call to strdup() instead of backporting VIR_STRDUP, for fewer patches but more conflict resolution; but either approach seems acceptable.
Thanks for the review!
Is anyone still using v0.9.11-maint? The CVE extends back to 0.9.8, so we could argue that we should either fix the 0.9.11 branch, or add another commit to the branch that explicitly marks it as end-of-life because no one appears to be relying on it. Fedora 18 is now end-of-life, so from Fedora's perspective, I only care about 0.10.2 (RHEL and CentOS 6), 1.0.5 (F19), 1.1.3 (F20) and soon 1.2.1 (rawhide), although I didn't mind touching all the intermediate branches on my way down to 0.10.2. RHEL 5 is also vulnerable to CVE-2013-6458, but as we don't have an upstream v0.8.2-maint branch (thank goodness!), that's something for Red Hat to worry about.
I'd say let's close 0.9.11. We have 0.8.3 in Debian oldstable but I'm not going to open a maint branch for this but deal with it in the package itself. Cheers, -- Guido
On 01/15/2014 01:43 PM, Eric Blake wrote:
Is anyone still using v0.9.11-maint? The CVE extends back to 0.9.8, so we could argue that we should either fix the 0.9.11 branch, or add another commit to the branch that explicitly marks it as end-of-life because no one appears to be relying on it. Fedora 18 is now end-of-life, so from Fedora's perspective, I only care about 0.10.2 (RHEL and CentOS 6), 1.0.5 (F19), 1.1.3 (F20) and soon 1.2.1 (rawhide), although I didn't mind touching all the intermediate branches on my way down to 0.10.2. RHEL 5 is also vulnerable to CVE-2013-6458, but as we don't have an upstream v0.8.2-maint branch (thank goodness!), that's something for Red Hat to worry about.
I've gone ahead and marked v0.8.3-maint and v0.9.11-maint as closed (I'm not posting the actual patch here, but it was done by 'git rm -f \*' followed by recreating .gitignore and a placeholder README that mentions the death of the branch). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Wed, Jan 22, 2014 at 12:13:48PM -0700, Eric Blake wrote:
On 01/15/2014 01:43 PM, Eric Blake wrote:
Is anyone still using v0.9.11-maint? The CVE extends back to 0.9.8, so we could argue that we should either fix the 0.9.11 branch, or add another commit to the branch that explicitly marks it as end-of-life because no one appears to be relying on it. Fedora 18 is now end-of-life, so from Fedora's perspective, I only care about 0.10.2 (RHEL and CentOS 6), 1.0.5 (F19), 1.1.3 (F20) and soon 1.2.1 (rawhide), although I didn't mind touching all the intermediate branches on my way down to 0.10.2. RHEL 5 is also vulnerable to CVE-2013-6458, but as we don't have an upstream v0.8.2-maint branch (thank goodness!), that's something for Red Hat to worry about.
I've gone ahead and marked v0.8.3-maint and v0.9.11-maint as closed (I'm not posting the actual patch here, but it was done by 'git rm -f \*' followed by recreating .gitignore and a placeholder README that mentions the death of the branch).
FYI for openstack I examined the current libvirt versions in some major distros: https://wiki.openstack.org/wiki/LibvirtDistroSupportMatrix Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 01/23/2014 12:26 PM, Daniel P. Berrange wrote:
On Wed, Jan 22, 2014 at 12:13:48PM -0700, Eric Blake wrote:
On 01/15/2014 01:43 PM, Eric Blake wrote:
Is anyone still using v0.9.11-maint? The CVE extends back to 0.9.8, so we could argue that we should either fix the 0.9.11 branch, or add another commit to the branch that explicitly marks it as end-of-life because no one appears to be relying on it. Fedora 18 is now end-of-life, so from Fedora's perspective, I only care about 0.10.2 (RHEL and CentOS 6), 1.0.5 (F19), 1.1.3 (F20) and soon 1.2.1 (rawhide), although I didn't mind touching all the intermediate branches on my way down to 0.10.2. RHEL 5 is also vulnerable to CVE-2013-6458, but as we don't have an upstream v0.8.2-maint branch (thank goodness!), that's something for Red Hat to worry about. I've gone ahead and marked v0.8.3-maint and v0.9.11-maint as closed (I'm not posting the actual patch here, but it was done by 'git rm -f \*' followed by recreating .gitignore and a placeholder README that mentions the death of the branch). FYI for openstack I examined the current libvirt versions in some major distros:
After seeing that list, I thought an "end of life" column could be interesing, but then realized the only bit I was interested in was how long we will need to put of with the oldest version on the list. As far as I can tell, Ubuntu 12.04 LTS is scheduled for EOL in April 2017 (date from here: https://wiki.ubuntu.com/Releases ), so I guess *somebody* has to care about libvirt-0.9.8 until 2017 (of course we don't have a v0.9.8-maint branch anyway, so that's not likely going to happen within upstream infrastructure)
On Thu, Jan 23, 2014 at 10:26:14AM +0000, Daniel P. Berrange wrote:
On Wed, Jan 22, 2014 at 12:13:48PM -0700, Eric Blake wrote:
On 01/15/2014 01:43 PM, Eric Blake wrote:
Is anyone still using v0.9.11-maint? The CVE extends back to 0.9.8, so we could argue that we should either fix the 0.9.11 branch, or add another commit to the branch that explicitly marks it as end-of-life because no one appears to be relying on it. Fedora 18 is now end-of-life, so from Fedora's perspective, I only care about 0.10.2 (RHEL and CentOS 6), 1.0.5 (F19), 1.1.3 (F20) and soon 1.2.1 (rawhide), although I didn't mind touching all the intermediate branches on my way down to 0.10.2. RHEL 5 is also vulnerable to CVE-2013-6458, but as we don't have an upstream v0.8.2-maint branch (thank goodness!), that's something for Red Hat to worry about.
I've gone ahead and marked v0.8.3-maint and v0.9.11-maint as closed (I'm not posting the actual patch here, but it was done by 'git rm -f \*' followed by recreating .gitignore and a placeholder README that mentions the death of the branch).
FYI for openstack I examined the current libvirt versions in some major distros:
Great. I took the libverty to add wheezy-backports as well since we ship updated versions from there and it's nowadays integrated into the main Debian archive. Cheers, -- Guido
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Guido Günther -
Laine Stump