Fix: use after free on metadata cache reallocation

When the metadata cache is expanded (reallocated) by
lttng_metadata_printf(), the metadata cache reader
(lttng_metadata_output_channel()) may use freed memory, because the
metadata cache is not protected from concurrent read accesses. The
metadata cache updates are protected from each other by the sessions
mutex, but metadata cache reads do not hold the sessions mutex.
Actually, the comment on top of lttng_metadata_output_channel() stating
"We have exclusive access to our metadata buffer (protected by the
sessions_mutex)" is simply wrong, because this mutex is never held when
calling lttng_metadata_output_channel().

Promote the per-stream lock to the metadata cache used by each of those
metadata streams, thus ensuring mutual exclusion between metadata cache
reallocation and readers.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

diff --git a/lttng-abi.c b/lttng-abi.c
index 93cdce3e0f00ec37e007ae17a54bce290ee035df..8f63ad94c8fb053d9178aa91aa71d14d317f4e1b 100644 (file)
--- a/lttng-abi.c
+++ b/lttng-abi.c
@@ -622,9 +622,11 @@ unsigned int lttng_metadata_ring_buffer_poll(struct file *filp,
                if (finalized)
                        mask |= POLLHUP;
 
+               mutex_lock(&stream->metadata_cache->lock);
                if (stream->metadata_cache->metadata_written >
                                stream->metadata_out)
                        mask |= POLLIN;
+               mutex_unlock(&stream->metadata_cache->lock);
        }
 
        return mask;
@@ -922,7 +924,6 @@ int lttng_abi_open_metadata_stream(struct file *channel_file)
        metadata_stream->priv = buf;
        stream_priv = metadata_stream;
        metadata_stream->transport = channel->transport;
-       mutex_init(&metadata_stream->lock);
 
        /*
         * Since life-time of metadata cache differs from that of

diff --git a/lttng-events.c b/lttng-events.c
index 7eec04c0f269682a1aed08bbc501a9a992a127ea..24a7b3ca9101464f774a4ec0cc5469d1224b96de 100644 (file)
--- a/lttng-events.c
+++ b/lttng-events.c
@@ -138,6 +138,7 @@ struct lttng_session *lttng_session_create(void)
                goto err_free_cache;
        metadata_cache->cache_alloc = METADATA_CACHE_DEFAULT_SIZE;
        kref_init(&metadata_cache->refcount);
+       mutex_init(&metadata_cache->lock);
        session->metadata_cache = metadata_cache;
        INIT_LIST_HEAD(&metadata_cache->metadata_stream);
        memcpy(&metadata_cache->uuid, &session->uuid,
@@ -1439,10 +1440,12 @@ void lttng_session_lazy_sync_enablers(struct lttng_session *session)
 /*
  * Serialize at most one packet worth of metadata into a metadata
  * channel.
- * We have exclusive access to our metadata buffer (protected by the
- * sessions_mutex), so we can do racy operations such as looking for
- * remaining space left in packet and write, since mutual exclusion
- * protects us from concurrent writes.
+ * We grab the metadata cache mutex to get exclusive access to our metadata
+ * buffer and to the metadata cache. Exclusive access to the metadata buffer
+ * allows us to do racy operations such as looking for remaining space left in
+ * packet and write, since mutual exclusion protects us from concurrent writes.
+ * Mutual exclusion on the metadata cache allow us to read the cache content
+ * without racing against reallocation of the cache by updates.
  * Returns the number of bytes written in the channel, 0 if no data
  * was written and a negative value on error.
  */
@@ -1454,13 +1457,15 @@ int lttng_metadata_output_channel(struct lttng_metadata_stream *stream,
        size_t len, reserve_len;
 
        /*
-        * Ensure we support mutiple get_next / put sequences followed
-        * by put_next. The metadata stream lock internally protects
-        * reading the metadata cache. It can indeed be read
-        * concurrently by "get_next_subbuf" and "flush" operations on
-        * the buffer invoked by different processes.
+        * Ensure we support mutiple get_next / put sequences followed by
+        * put_next. The metadata cache lock protects reading the metadata
+        * cache. It can indeed be read concurrently by "get_next_subbuf" and
+        * "flush" operations on the buffer invoked by different processes.
+        * Moreover, since the metadata cache memory can be reallocated, we
+        * need to have exclusive access against updates even though we only
+        * read it.
         */
-       mutex_lock(&stream->lock);
+       mutex_lock(&stream->metadata_cache->lock);
        WARN_ON(stream->metadata_in < stream->metadata_out);
        if (stream->metadata_in != stream->metadata_out)
                goto end;
@@ -1490,13 +1495,15 @@ int lttng_metadata_output_channel(struct lttng_metadata_stream *stream,
        ret = reserve_len;
 
 end:
-       mutex_unlock(&stream->lock);
+       mutex_unlock(&stream->metadata_cache->lock);
        return ret;
 }
 
 /*
  * Write the metadata to the metadata cache.
  * Must be called with sessions_mutex held.
+ * The metadata cache lock protects us from concurrent read access from
+ * thread outputting metadata content to ring buffer.
  */
 int lttng_metadata_printf(struct lttng_session *session,
                          const char *fmt, ...)
@@ -1515,6 +1522,7 @@ int lttng_metadata_printf(struct lttng_session *session,
                return -ENOMEM;
 
        len = strlen(str);
+       mutex_lock(&session->metadata_cache->lock);
        if (session->metadata_cache->metadata_written + len >
                        session->metadata_cache->cache_alloc) {
                char *tmp_cache_realloc;
@@ -1534,6 +1542,7 @@ int lttng_metadata_printf(struct lttng_session *session,
                        session->metadata_cache->metadata_written,
                        str, len);
        session->metadata_cache->metadata_written += len;
+       mutex_unlock(&session->metadata_cache->lock);
        kfree(str);
 
        list_for_each_entry(stream, &session->metadata_cache->metadata_stream, list)
@@ -1542,6 +1551,7 @@ int lttng_metadata_printf(struct lttng_session *session,
        return 0;
 
 err:
+       mutex_unlock(&session->metadata_cache->lock);
        kfree(str);
        return -ENOMEM;
 }

diff --git a/lttng-events.h b/lttng-events.h
index 4d9bdfd0574390da27f7177c26d561683ae3c553..484534c4272948208a0b426468282c9ae78d8873 100644 (file)
--- a/lttng-events.h
+++ b/lttng-events.h
@@ -415,7 +415,6 @@ struct lttng_metadata_stream {
        wait_queue_head_t read_wait;    /* Reader buffer-level wait queue */
        struct list_head list;          /* Stream list */
        struct lttng_transport *transport;
-       struct mutex lock;
 };
 
 
@@ -461,6 +460,7 @@ struct lttng_metadata_cache {
        struct kref refcount;           /* Metadata cache usage */
        struct list_head metadata_stream;       /* Metadata stream list */
        uuid_le uuid;                   /* Trace session unique ID (copy) */
+       struct mutex lock;
 };
 
 void lttng_lock_sessions(void);