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

Overview Repositories Revisions Requests Users Attributes Meta

File pacemaker#3293-0002-Fix-pacemaker-attrd-libcrmcluster-avoid-use-after-fr.patch of Package pacemaker (Revision 3ab9eb55c57d5ec257dc86c3fa3851c8)

Currently displaying revision 3ab9eb55c57d5ec257dc86c3fa3851c8 , Show latest

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_corosync.c | 26 ++------------------------
 lib/cluster/membership.c       | 30 ++++++++++++++++++++++++++++--
 2 files changed, 30 insertions(+), 26 deletions(-)

Index: pacemaker-2.1.7+20231219.0f7f88312/daemons/attrd/attrd_corosync.c
===================================================================
--- pacemaker-2.1.7+20231219.0f7f88312.orig/daemons/attrd/attrd_corosync.c
+++ pacemaker-2.1.7+20231219.0f7f88312/daemons/attrd/attrd_corosync.c
@@ -166,28 +166,6 @@ broadcast_local_value(const attribute_t
     return v;
 }
 
-/*!
- * \internal
- * \brief Ensure a Pacemaker Remote node is in the correct peer cache
- *
- * \param[in] node_name  Name of Pacemaker Remote node to check
- */
-static void
-cache_remote_node(const char *node_name)
-{
-    /* If we previously assumed this node was an unseen cluster node,
-     * remove its entry from the cluster peer cache.
-     */
-    crm_node_t *dup = pcmk__search_cluster_node_cache(0, node_name, NULL);
-
-    if (dup && (dup->uuid == NULL)) {
-        reap_crm_member(0, node_name);
-    }
-
-    // Ensure node is in the remote peer cache
-    CRM_ASSERT(crm_remote_peer_get(node_name) != NULL);
-}
-
 #define state_text(state) pcmk__s((state), "in unknown state")
 
 /*!
@@ -209,7 +187,7 @@ attrd_lookup_or_create_value(GHashTable
 
     crm_element_value_int(xml, PCMK__XA_ATTR_IS_REMOTE, &is_remote);
     if (is_remote) {
-        cache_remote_node(host);
+        CRM_ASSERT(crm_remote_peer_get(host) != NULL);
     }
 
     if (v == NULL) {
@@ -272,7 +250,7 @@ attrd_peer_change_cb(enum crm_status_typ
 
     // Ensure remote nodes that come up are in the remote node cache
     } else if (!gone && is_remote) {
-        cache_remote_node(peer->uname);
+        CRM_ASSERT(crm_remote_peer_get(peer->uname) != NULL);
     }
 }
 
Index: pacemaker-2.1.7+20231219.0f7f88312/lib/cluster/membership.c
===================================================================
--- pacemaker-2.1.7+20231219.0f7f88312.orig/lib/cluster/membership.c
+++ pacemaker-2.1.7+20231219.0f7f88312/lib/cluster/membership.c
@@ -102,26 +102,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 pcmk__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 = pcmk__search_cluster_node_cache(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;
     }
 
@@ -130,7 +154,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;
     }
 
@@ -140,6 +165,7 @@ crm_remote_peer_get(const char *node_nam
 
     /* Update the entry's uname, ensuring peer status callbacks are called */
     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 (Revision 3ab9eb55c57d5ec257dc86c3fa3851c8)

Places