File pacemaker#3293-0002-Fix-pacemaker-attrd-libcrmc... of Package pacemaker.34782

Overview Repositories Revisions Requests Users Attributes Meta

File pacemaker#3293-0002-Fix-pacemaker-attrd-libcrmcluster-avoid-use-after-fr.patch of Package pacemaker.34782

From 8d552c1b582a95f9879b15e2dd991a7f995e7eca Mon Sep 17 00:00:00 2001
From: Ken Gaillot <kgaillot@redhat.com>
Date: Thu, 14 Dec 2023 09:51:37 -0600
Subject: [PATCH] Fix: pacemaker-attrd,libcrmcluster: avoid use-after-free when
 remote node in cluster node cache

Previously, pacemaker-attrd removed any conflicting entry from the cluster node
cache before adding a node to the remote node cache. However, if the name used
was a pointer into the cluster node cache entry being freed, it would be reused
to create the remote node cache entry.

This avoids that and also moves the functionality into libcrmcluster for better
isolation of cache management. It also corrects mistakenly setting errno to a
negative value.
---
 daemons/attrd/attrd_commands.c | 26 ++------------------------
 lib/cluster/membership.c       | 30 ++++++++++++++++++++++++++++--
 2 files changed, 30 insertions(+), 26 deletions(-)

Index: pacemaker-2.0.5+20201202.ba59be712/daemons/attrd/attrd_commands.c
===================================================================
--- pacemaker-2.0.5+20201202.ba59be712.orig/daemons/attrd/attrd_commands.c
+++ pacemaker-2.0.5+20201202.ba59be712/daemons/attrd/attrd_commands.c
@@ -706,16 +706,6 @@ attrd_lookup_or_create_value(GHashTable
 
     crm_element_value_int(xml, PCMK__XA_ATTR_IS_REMOTE, &is_remote);
     if (is_remote) {
-        /* If we previously assumed this node was an unseen cluster node,
-         * remove its entry from the cluster peer cache.
-         */
-        crm_node_t *dup = crm_find_peer(0, host, NULL);
-
-        if (dup && (dup->uuid == NULL)) {
-            reap_crm_member(0, host);
-        }
-
-        /* Ensure this host is in the remote peer cache */
         CRM_ASSERT(crm_remote_peer_get(host) != NULL);
     }
 
Index: pacemaker-2.0.5+20201202.ba59be712/lib/cluster/membership.c
===================================================================
--- pacemaker-2.0.5+20201202.ba59be712.orig/lib/cluster/membership.c
+++ pacemaker-2.0.5+20201202.ba59be712/lib/cluster/membership.c
@@ -75,26 +75,50 @@ crm_remote_peer_cache_size(void)
  * \note When creating a new entry, this will leave the node state undetermined,
  *       so the caller should also call crm_update_peer_state() if the state is
  *       known.
+ * \note Because this can add and remove cache entries, callers should not
+ *       assume any previously obtained cache entry pointers remain valid.
  */
 crm_node_t *
 crm_remote_peer_get(const char *node_name)
 {
     crm_node_t *node;
+    char *node_name_copy = NULL;
 
     if (node_name == NULL) {
-        errno = -EINVAL;
+        errno = EINVAL;
         return NULL;
     }
 
+    /* It's theoretically possible that the node was added to the cluster peer
+     * cache before it was known to be a Pacemaker Remote node. Remove that
+     * entry unless it has a node ID, which means the name actually is
+     * associated with a cluster node. (@TODO return an error in that case?)
+     */
+    node = crm_find_peer(0, node_name, NULL);
+    if ((node != NULL) && (node->uuid == NULL)) {
+        /* node_name could be a pointer into the cache entry being removed, so
+         * reassign it to a copy before the original gets freed
+         */
+        node_name_copy = strdup(node_name);
+        if (node_name_copy == NULL) {
+            errno = ENOMEM;
+            return NULL;
+        }
+        node_name = node_name_copy;
+        reap_crm_member(0, node_name);
+    }
+
     /* Return existing cache entry if one exists */
     node = g_hash_table_lookup(crm_remote_peer_cache, node_name);
     if (node) {
+        free(node_name_copy);
         return node;
     }
 
     /* Allocate a new entry */
     node = calloc(1, sizeof(crm_node_t));
     if (node == NULL) {
+        free(node_name_copy);
         return NULL;
     }
 
@@ -103,7 +127,8 @@ crm_remote_peer_get(const char *node_nam
     node->uuid = strdup(node_name);
     if (node->uuid == NULL) {
         free(node);
-        errno = -ENOMEM;
+        errno = ENOMEM;
+        free(node_name_copy);
         return NULL;
     }
 
@@ -113,6 +138,7 @@ crm_remote_peer_get(const char *node_nam
 
     /* Update the entry's uname, ensuring peer status callbacks are called */
     crm_update_peer_uname(node, node_name);
+    free(node_name_copy);
     return node;
 }

Places

File pacemaker#3293-0002-Fix-pacemaker-attrd-libcrmcluster-avoid-use-after-fr.patch of Package pacemaker.34782

Places