diff --git a/SOURCES/0001-tabrmd-options-fix-memory-leak.patch b/SOURCES/0001-tabrmd-options-fix-memory-leak.patch new file mode 100644 index 0000000..e8c5ec1 --- /dev/null +++ b/SOURCES/0001-tabrmd-options-fix-memory-leak.patch @@ -0,0 +1,211 @@ +From ff90674fd801dd369231a20c47ebef0d08402e9e Mon Sep 17 00:00:00 2001 +From: William Roberts +Date: Tue, 12 Jan 2021 14:12:48 -0600 +Subject: [PATCH 1/6] tabrmd-options: fix memory leak + +The tabrmd_options_t structure is initialized with static char * +strings. These strings can be replaced by g_option_context_parse(). +However, g_option_context_parse() replaces the string with allocated +memory and thus needs a call to g_free. Either one would need to keep +track if glib allocated the string and conditionally free it, or just +set all the strings to glib allocated strings. This patch takes the +approach of always allocating the option strings. + +Fixes leaks like: +==2677142==ERROR: LeakSanitizer: detected memory leaks + +Direct leak of 9 byte(s) in 1 object(s) allocated from: + #8 0x7fbd1acd5da1 in g_option_context_parse (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x5eda1) + #9 0x4cc438 in parse_opts /home/wcrobert/workspace/tpm2-abrmd/src/tabrmd-options.c:103:10 + #10 0x4c7ffe in main /home/wcrobert/workspace/tpm2-abrmd/src/tabrmd.c:41:10 + #11 0x7fbd1a8770b2 in __libc_start_main /build/glibc-ZN95T4/glibc-2.31/csu/../csu/libc-start.c:308:16 + #12 0x42004d in _start (/home/wcrobert/workspace/tpm2-abrmd/src/tpm2-abrmd+0x42004d) + +Signed-off-by: William Roberts +--- + src/tabrmd-init.c | 2 ++ + src/tabrmd-options.c | 53 +++++++++++++++++++++++++++++++++++++++----- + src/tabrmd-options.h | 11 +++++---- + src/tabrmd.c | 4 +++- + 4 files changed, 59 insertions(+), 11 deletions(-) + +diff --git a/src/tabrmd-init.c b/src/tabrmd-init.c +index 2ad7539..58e0103 100644 +--- a/src/tabrmd-init.c ++++ b/src/tabrmd-init.c +@@ -99,6 +99,8 @@ gmain_data_cleanup (gmain_data_t *data) + if (data->loop != NULL) { + main_loop_quit (data->loop); + } ++ ++ tabrmd_options_free(&data->options); + } + /* + * This function initializes and configures all of the long-lived objects +diff --git a/src/tabrmd-options.c b/src/tabrmd-options.c +index 0dd7b87..22f249c 100644 +--- a/src/tabrmd-options.c ++++ b/src/tabrmd-options.c +@@ -16,6 +16,12 @@ + #define G_OPTION_FLAG_NONE 0 + #endif + ++#define SET_STR_IF_NULL(var, value) \ ++ do { \ ++ var = var == NULL ? g_strdup(value) : var; \ ++ g_assert(var); \ ++ } while(0) ++ + /* + * This is a GOptionArgFunc callback invoked from the GOption processor from + * the parse_opts function below. It will be called when the daemon is +@@ -36,6 +42,22 @@ show_version (const gchar *option_name, + g_print ("tpm2-abrmd version %s\n", VERSION); + exit (0); + } ++ ++/** ++ * Frees internal memory associated with a tabrmd_options_t struct. ++ * @param opts ++ * The options to free, note it doesn't free opts itself. ++ */ ++void ++tabrmd_options_free(tabrmd_options_t *opts) ++{ ++ g_assert(opts); ++ ++ g_clear_pointer(&opts->dbus_name, g_free); ++ g_clear_pointer(&opts->prng_seed_file, g_free); ++ g_clear_pointer(&opts->tcti_conf, g_free); ++} ++ + /** + * This function parses the parameter argument vector and populates the + * parameter 'options' structure with data needed to configure the tabrmd. +@@ -51,7 +73,7 @@ parse_opts (gint argc, + gchar *argv[], + tabrmd_options_t *options) + { +- gchar *logger_name = "stdout"; ++ gchar *logger_name = NULL; + GOptionContext *ctx; + GError *err = NULL; + gboolean session_bus = FALSE; +@@ -105,33 +127,52 @@ parse_opts (gint argc, + return FALSE; + } + g_option_context_free (ctx); ++ ++ /* ++ * Set unset STRING options to defaults, we do this so we can free allocated ++ * string options with gfree, having a mix of const and allocated ptr's ++ * causes leaks ++ */ ++ SET_STR_IF_NULL(options->dbus_name, TABRMD_DBUS_NAME_DEFAULT); ++ SET_STR_IF_NULL(options->prng_seed_file, TABRMD_ENTROPY_SRC_DEFAULT); ++ SET_STR_IF_NULL(options->tcti_conf, TABRMD_TCTI_CONF_DEFAULT); ++ SET_STR_IF_NULL(logger_name, "stdout"); ++ + /* select the bus type, default to G_BUS_TYPE_SESSION */ + options->bus = session_bus ? G_BUS_TYPE_SESSION : G_BUS_TYPE_SYSTEM; +- if (set_logger (logger_name) == -1) { ++ gint ret = set_logger (logger_name); ++ if (ret == -1) { + g_critical ("Unknown logger: %s, try --help\n", logger_name); +- return FALSE; ++ g_free(logger_name); ++ goto error; + } ++ g_free(logger_name); ++ + if (options->max_connections < 1 || + options->max_connections > TABRMD_CONNECTION_MAX) + { + g_critical ("maximum number of connections must be between 1 " + "and %d", TABRMD_CONNECTION_MAX); +- return FALSE; ++ goto error; + } + if (options->max_sessions < 1 || + options->max_sessions > TABRMD_SESSIONS_MAX_DEFAULT) + { + g_critical ("max-sessions must be between 1 and %d", + TABRMD_SESSIONS_MAX_DEFAULT); +- return FALSE; ++ goto error; + } + if (options->max_transients < 1 || + options->max_transients > TABRMD_TRANSIENT_MAX) + { + g_critical ("max-trans-obj parameter must be between 1 and %d", + TABRMD_TRANSIENT_MAX); +- return FALSE; ++ goto error; + } + g_warning ("tcti_conf after: \"%s\"", options->tcti_conf); + return TRUE; ++ ++error: ++ tabrmd_options_free(options); ++ return FALSE; + } +diff --git a/src/tabrmd-options.h b/src/tabrmd-options.h +index 4994920..d6bcfe9 100644 +--- a/src/tabrmd-options.h ++++ b/src/tabrmd-options.h +@@ -15,10 +15,10 @@ + .max_connections = TABRMD_CONNECTIONS_MAX_DEFAULT, \ + .max_transients = TABRMD_TRANSIENT_MAX_DEFAULT, \ + .max_sessions = TABRMD_SESSIONS_MAX_DEFAULT, \ +- .dbus_name = TABRMD_DBUS_NAME_DEFAULT, \ +- .prng_seed_file = TABRMD_ENTROPY_SRC_DEFAULT, \ ++ .dbus_name = NULL, \ ++ .prng_seed_file = NULL, \ + .allow_root = FALSE, \ +- .tcti_conf = TABRMD_TCTI_CONF_DEFAULT, \ ++ .tcti_conf = NULL, \ + } + + typedef struct tabrmd_options { +@@ -28,7 +28,7 @@ typedef struct tabrmd_options { + guint max_transients; + guint max_sessions; + gchar *dbus_name; +- const gchar *prng_seed_file; ++ gchar *prng_seed_file; + gboolean allow_root; + gchar *tcti_conf; + } tabrmd_options_t; +@@ -38,4 +38,7 @@ parse_opts (gint argc, + gchar *argv[], + tabrmd_options_t *options); + ++void ++tabrmd_options_free(tabrmd_options_t *opts); ++ + #endif /* TABRMD_OPTIONS_H */ +diff --git a/src/tabrmd.c b/src/tabrmd.c +index 7c93e90..e015de3 100644 +--- a/src/tabrmd.c ++++ b/src/tabrmd.c +@@ -43,7 +43,8 @@ main (int argc, char *argv[]) + } + if (geteuid() == 0 && ! gmain_data.options.allow_root) { + g_print ("Refusing to run as root. Pass --allow-root if you know what you are doing.\n"); +- return 1; ++ ret = 1; ++ goto out; + } + + g_mutex_init (&gmain_data.init_mutex); +@@ -63,6 +64,7 @@ main (int argc, char *argv[]) + if (ret == 0 && gmain_data.ipc_disconnected) { + ret = EX_IOERR; + } ++out: + gmain_data_cleanup (&gmain_data); + return ret; + } +-- +2.34.3 + diff --git a/SOURCES/0002-resource-manager-rm-ref-count-inc-of-handle_entry.patch b/SOURCES/0002-resource-manager-rm-ref-count-inc-of-handle_entry.patch new file mode 100644 index 0000000..4d473ca --- /dev/null +++ b/SOURCES/0002-resource-manager-rm-ref-count-inc-of-handle_entry.patch @@ -0,0 +1,49 @@ +From ec7116d0e4de535a90c1dc5edabe821f04a0f8e0 Mon Sep 17 00:00:00 2001 +From: William Roberts +Date: Wed, 13 Jan 2021 12:21:47 -0600 +Subject: [PATCH 2/6] resource-manager: rm ref count inc of handle_entry + +Per: + - https://developer.gnome.org/gobject/stable/gobject-memory.html + +g_object_new sets the ref count to 1, so their is no need to bump it +again, we already have ownership. + +Fixes leaks like: +Direct leak of 10480 byte(s) in 2 object(s) allocated from: + #0 0x7f1aa88aabc8 in malloc (/usr/lib/x86_64-linux-gnu/libasan.so.5+0x10dbc8) + #1 0x7f1aa848acd8 in g_malloc (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x57cd8) + #2 0x7f1aa84a32c5 in g_slice_alloc (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x702c5) + #3 0x7f1aa84a38ed in g_slice_alloc0 (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x708ed) + #4 0x7f1aa85970cf in g_type_create_instance (/usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0+0x3b0cf) + #5 0x7f1aa857634c (/usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0+0x1a34c) + #6 0x7f1aa8578377 in g_object_new_valist (/usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0+0x1c377) + #7 0x7f1aa85786cc in g_object_new (/usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0+0x1c6cc) + #8 0x561e13d667d3 in handle_map_entry_new src/handle-map-entry.c:138 + #9 0x561e13d540d3 in create_context_mapping_transient src/resource-manager.c:1160 + #10 0x561e13d547b1 in resource_manager_create_context_mapping src/resource-manager.c:1261 + #11 0x561e13d54ec8 in resource_manager_process_tpm2_command src/resource-manager.c:1359 + #12 0x561e13d55365 in resource_manager_thread src/resource-manager.c:1424 + #13 0x7f1aa8384608 in start_thread /build/glibc-ZN95T4/glibc-2.31/nptl/pthread_create.c:477 + #14 0x7f1aa82ab292 in __clone (/lib/x86_64-linux-gnu/libc.so.6+0x122292) + +Signed-off-by: William Roberts +--- + src/resource-manager.c | 1 - + 1 file changed, 1 deletion(-) + +diff --git a/src/resource-manager.c b/src/resource-manager.c +index 904f683..050436f 100644 +--- a/src/resource-manager.c ++++ b/src/resource-manager.c +@@ -1167,7 +1167,6 @@ create_context_mapping_transient (ResourceManager *resmgr, + handle_map_insert (handle_map, vhandle, handle_entry); + g_object_unref (handle_map); + tpm2_response_set_handle (response, vhandle); +- g_object_ref (handle_entry); + } + /* + * This function after a Tpm2Command is sent to the TPM and: +-- +2.34.3 + diff --git a/SOURCES/0003-tabrmd-init.c-fix-leaks-on-main-to-thread-tpm2-insta.patch b/SOURCES/0003-tabrmd-init.c-fix-leaks-on-main-to-thread-tpm2-insta.patch new file mode 100644 index 0000000..9ee33ae --- /dev/null +++ b/SOURCES/0003-tabrmd-init.c-fix-leaks-on-main-to-thread-tpm2-insta.patch @@ -0,0 +1,51 @@ +From 62ae28635ada2a74b526244e8ea69cef74c6c022 Mon Sep 17 00:00:00 2001 +From: William Roberts +Date: Wed, 13 Jan 2021 13:52:06 -0600 +Subject: [PATCH 3/6] tabrmd-init.c: fix leaks on main to thread tpm2 instance + +Theirs a case where the Tpm2 object coming in from main to the thread +fails setup and the cleanup function doesn't unref it. Move it to the +main cleanup routine and use g_clear_object to be *clear* on whom owns +the reference. + +Fixes leaks like: +Indirect leak of 4176 byte(s) in 1 object(s) allocated from: + #0 0x7f652e71cdc6 in calloc (/usr/lib/x86_64-linux-gnu/libasan.so.5+0x10ddc6) + #1 0x7f652e25ad30 in g_malloc0 (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x57d30) + #2 0x555ebb1a1c5f in sapi_context_init src/tpm2.c:162 + #3 0x555ebb1a2fa8 in tpm2_new src/tpm2.c:438 + #4 0x555ebb19d665 in init_thread_func src/tabrmd-init.c:178 + #5 0x555ebb19bede in init_thread_func_tpm2_init_fail test/tabrmd-init_unit.c:199 + #6 0x7f652e6074e0 (/usr/lib/x86_64-linux-gnu/libcmocka.so.0+0x54e0) + +Signed-off-by: William Roberts +--- + src/tabrmd-init.c | 5 ++++- + 1 file changed, 4 insertions(+), 1 deletion(-) + +diff --git a/src/tabrmd-init.c b/src/tabrmd-init.c +index 58e0103..866c852 100644 +--- a/src/tabrmd-init.c ++++ b/src/tabrmd-init.c +@@ -99,6 +99,9 @@ gmain_data_cleanup (gmain_data_t *data) + if (data->loop != NULL) { + main_loop_quit (data->loop); + } ++ if (data->tpm2) { ++ g_clear_object (&data->tpm2); ++ } + + tabrmd_options_free(&data->options); + } +@@ -208,7 +211,7 @@ init_thread_func (gpointer user_data) + g_clear_object (&session_list); + data->response_sink = response_sink_new (); + g_object_unref (command_attrs); +- g_object_unref (data->tpm2); ++ g_clear_object (&data->tpm2); + /* + * Wire up the TPM command processing pipeline. TPM command buffers + * flow from the CommandSource, to the Tab then finally back to the +-- +2.34.3 + diff --git a/SOURCES/0004-init_thread_func-fix-deadlock.patch b/SOURCES/0004-init_thread_func-fix-deadlock.patch new file mode 100644 index 0000000..61afc17 --- /dev/null +++ b/SOURCES/0004-init_thread_func-fix-deadlock.patch @@ -0,0 +1,28 @@ +From 545287019c1b9689c92900330be058b5ab9cf5d6 Mon Sep 17 00:00:00 2001 +From: William Roberts +Date: Wed, 13 Jan 2021 15:11:42 -0600 +Subject: [PATCH 4/6] init_thread_func: fix deadlock + +The caller locks the mutex and never releases on the error path, only +the success path. + +Signed-off-by: William Roberts +--- + src/tabrmd-init.c | 1 + + 1 file changed, 1 insertion(+) + +diff --git a/src/tabrmd-init.c b/src/tabrmd-init.c +index 866c852..ea71155 100644 +--- a/src/tabrmd-init.c ++++ b/src/tabrmd-init.c +@@ -249,6 +249,7 @@ init_thread_func (gpointer user_data) + return GINT_TO_POINTER (0); + + err_out: ++ g_mutex_unlock (&data->init_mutex); + g_debug ("%s: calling gmain_data_cleanup", __func__); + gmain_data_cleanup (data); + return GINT_TO_POINTER (ret); +-- +2.34.3 + diff --git a/SOURCES/0005-ResourceManager-Avoid-double-free-in-resource-manage.patch b/SOURCES/0005-ResourceManager-Avoid-double-free-in-resource-manage.patch new file mode 100644 index 0000000..f3fcfd7 --- /dev/null +++ b/SOURCES/0005-ResourceManager-Avoid-double-free-in-resource-manage.patch @@ -0,0 +1,30 @@ +From a97e07d5a5947f5749e4ea25d0f538eeee8997bb Mon Sep 17 00:00:00 2001 +From: Jerry Snitselaar +Date: Mon, 23 Nov 2020 11:45:31 -0700 +Subject: [PATCH 5/6] ResourceManager: Avoid double free in resource-manager.c + +Clean up potential double free found by coverity in +resource_manager_load_session_from_handle. If flush_session has been +called, don't call session_list_remove which is already called in +flush_session. + +Signed-off-by: Jerry Snitselaar +--- + src/resource-manager.c | 1 + + 1 file changed, 1 insertion(+) + +diff --git a/src/resource-manager.c b/src/resource-manager.c +index 050436f..556184b 100644 +--- a/src/resource-manager.c ++++ b/src/resource-manager.c +@@ -239,6 +239,7 @@ resource_manager_load_session_from_handle (ResourceManager *resmgr, + rc = tpm2_response_get_code (response); + if (rc != TSS2_RC_SUCCESS) { + flush_session (resmgr, session_entry); ++ goto out; + } + } + if (will_flush) { +-- +2.34.3 + diff --git a/SOURCES/0006-tcti-initialize-GError-to-NULL.patch b/SOURCES/0006-tcti-initialize-GError-to-NULL.patch new file mode 100644 index 0000000..947001c --- /dev/null +++ b/SOURCES/0006-tcti-initialize-GError-to-NULL.patch @@ -0,0 +1,40 @@ +From a645f8c656b47568072351f4bfa58960016fbbac Mon Sep 17 00:00:00 2001 +From: Nicolas Iooss +Date: Mon, 27 Sep 2021 16:46:42 +0200 +Subject: [PATCH 6/6] tcti: initialize GError to NULL + +When an error happens in `tcti_tabrmd_read`, Glib reports: + + (process:905338): GLib-WARNING **: 06:59:08.971: GError set over the + top of a previous GError or uninitialized memory. + This indicates a bug in someone's code. You must ensure an error is + NULL before it's set. + The overwriting error message was: Error receiving data: Connection + reset by peer + +This warning was reported on +https://github.com/tpm2-software/tpm2-pkcs11/issues/705 + +Fix the warning by initializing `error` correctly. + +Signed-off-by: Nicolas Iooss +--- + src/tcti-tabrmd.c | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +diff --git a/src/tcti-tabrmd.c b/src/tcti-tabrmd.c +index d96709e..d0ab74d 100644 +--- a/src/tcti-tabrmd.c ++++ b/src/tcti-tabrmd.c +@@ -187,7 +187,7 @@ tcti_tabrmd_read (TSS2_TCTI_TABRMD_CONTEXT *ctx, + size_t size, + int32_t timeout) + { +- GError *error; ++ GError *error = NULL; + ssize_t num_read; + int ret; + +-- +2.34.3 + diff --git a/SPECS/tpm2-abrmd.spec b/SPECS/tpm2-abrmd.spec index 793fe66..c723509 100644 --- a/SPECS/tpm2-abrmd.spec +++ b/SPECS/tpm2-abrmd.spec @@ -2,7 +2,7 @@ Name: tpm2-abrmd Version: 2.3.3 -Release: 2%{?dist} +Release: 3%{?dist} Summary: A system daemon implementing TPM2 Access Broker and Resource Manager License: BSD @@ -21,6 +21,13 @@ BuildRequires: pkgconfig(tss2-sys) # tpm2-abrmd depends on tpm2-tss-devel for tss2-mu/sys libs BuildRequires: tpm2-tss-devel >= 2.3.1-2%{?dist} +Patch0: 0001-tabrmd-options-fix-memory-leak.patch +Patch1: 0002-resource-manager-rm-ref-count-inc-of-handle_entry.patch +Patch2: 0003-tabrmd-init.c-fix-leaks-on-main-to-thread-tpm2-insta.patch +Patch3: 0004-init_thread_func-fix-deadlock.patch +Patch4: 0005-ResourceManager-Avoid-double-free-in-resource-manage.patch +Patch5: 0006-tcti-initialize-GError-to-NULL.patch + # tpm2-abrmd depends on the package that contains its SELinux policy module Requires: (%{name}-selinux >= 2.0.0-1%{?dist} if selinux-policy-%{selinuxtype}) @@ -90,6 +97,10 @@ required to build applications that use tpm2-abrmd. %systemd_postun tpm2-abrmd.service %changelog +* Thu Aug 11 2022 Štěpán Horáček - 2.3.3-3 +- Fix memory leaks and double free + resolves: rhbz#2041912 + * Mon Nov 23 2020 Jerry Snitselaar - 2.3.3-2 - Update tpm2-tss-devel BuildRequires resolves: rhbz#1855177