From 59999197d40a0811585912bfac399c3492b61da2 Mon Sep 17 00:00:00 2001 From: =?utf8?q?J=C3=A9r=C3=A9mie=20Galarneau?= Date: Wed, 2 Mar 2022 19:27:31 -0500 Subject: [PATCH] Fix: consumerd: use-after-free of metadata bucket MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Observed issue ============== When consumer_stream_destroy() is called from, for example, the error path in setup_metadata(), consumer_stream_free() can end up being called twice on the same stream. Since the stream->metadata_bucket is not set to NULL after being destroyed, it leads to a use-after-free: ERROR: AddressSanitizer: heap-use-after-free on address 0x604000000318 READ of size 8 at 0x604000000318 thread T7 #0 in metadata_bucket_destroy #1 in consumer_stream_free #2 in consumer_stream_destroy #3 in setup_metadata #4 in lttng_ustconsumer_recv_cmd #5 in lttng_consumer_recv_cmd #6 in consumer_thread_sessiond_poll #7 in start_thread nptl/pthread_create.c:481 #8 in clone (/lib/x86_64-linux-gnu/libc.so.6+0xfcbde) 0x604000000318 is located 8 bytes inside of 48-byte region [0x604000000310,0x604000000340) freed by thread T7 here: #0 in __interceptor_free #1 in metadata_bucket_destroy #2 in consumer_stream_free #3 in consumer_stream_destroy #4 in clean_channel_stream_list #5 in consumer_del_channel #6 in consumer_stream_destroy #7 in setup_metadata #8 in lttng_ustconsumer_recv_cmd #9 in lttng_consumer_recv_cmd #10 in consumer_thread_sessiond_poll #11 in start_thread nptl/pthread_create.c:481 previously allocated by thread T7 here: #0 in __interceptor_calloc #1 in zmalloc #2 in metadata_bucket_create #3 in consumer_stream_enable_metadata_bucketization #4 in lttng_ustconsumer_set_stream_ops #5 in lttng_ustconsumer_on_recv_stream #6 in lttng_consumer_on_recv_stream #7 in create_ust_streams #8 in ask_channel #9 in lttng_ustconsumer_recv_cmd #10 in lttng_consumer_recv_cmd #11 in consumer_thread_sessiond_poll #12 in start_thread nptl/pthread_create.c:481 Thread T7 created by T0 here: #0 in __interceptor_pthread_create #1 in main #2 in __libc_start_main ../csu/libc-start.c:332 SUMMARY: AddressSanitizer: heap-use-after-free in metadata_bucket_destroy This can be easily reproduced by forcing a failure during the setup of the metadata reproducible using the following change: diff --git a/src/common/ust-consumer/ust-consumer.c b/src/common/ust-consumer/ust-consumer.c index fa1c71299..97ed59632 100644 --- a/src/common/ust-consumer/ust-consumer.c +++ b/src/common/ust-consumer/ust-consumer.c @@ -908,8 +908,7 @@ static int setup_metadata(struct lttng_consumer_local_data *ctx, uint64_t key) /* Send metadata stream to relayd if needed. */ if (metadata->metadata_stream->net_seq_idx != (uint64_t) -1ULL) { - ret = consumer_send_relayd_stream(metadata->metadata_stream, - metadata->pathname); + ret = -1; if (ret < 0) { ret = LTTCOMM_CONSUMERD_ERROR_METADATA; goto error; Cause ===== Channels have a list of streams that are being "setup" and are not yet monitored for consumption. During this setup phase, the streams are owned by the channel. On destruction of the channel, any stream in that list will thus be cleaned-up. When destroying a consumer stream, a reference to its channel is 'put'. This can result in the destruction of the channel. In the situation described above, the release of the channel's reference is done before the stream is removed from the channel's stream list. This causes the channel's clean-up to invoke (again) the current stream's clean-up, resulting in the double-free of the metadata bucket. This problem is present in a number of error paths. Solution ======== Some error paths already manually removed the consumer stream from it's channel's stream list before invoking consumer_stream_destroy(). The various error paths that have to deal with this possible situation are changed to simply invoke consumer_stream_destroy(). consumer_stream_destroy() is modified to always remove the stream from its channel's list before performing the rest of the clean-up. This ensures that those double clean-ups can't occur. Drawbacks ========= None. Reported-by: Vincent Whitchurch Tested-by: Vincent Whitchurch Signed-off-by: Jérémie Galarneau Change-Id: Ibeca9b675b86fc46be3f57826f7158de4da43df8 --- src/common/consumer/consumer-stream.c | 4 ++++ src/common/consumer/consumer.c | 1 - src/common/consumer/consumer.h | 6 +++++- src/common/kernel-consumer/kernel-consumer.c | 1 - src/common/ust-consumer/ust-consumer.c | 6 ++---- 5 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/common/consumer/consumer-stream.c b/src/common/consumer/consumer-stream.c index 0f60dfd08..c334edb15 100644 --- a/src/common/consumer/consumer-stream.c +++ b/src/common/consumer/consumer-stream.c @@ -517,6 +517,8 @@ struct lttng_consumer_stream *consumer_stream_create( goto error; } + stream->send_node = (typeof(stream->send_node)) + CDS_LIST_HEAD_INIT(stream->send_node); stream->chan = channel; stream->key = stream_key; stream->trace_chunk = trace_chunk; @@ -889,6 +891,8 @@ void consumer_stream_destroy(struct lttng_consumer_stream *stream, { assert(stream); + cds_list_del_init(&stream->send_node); + /* Stream is in monitor mode. */ if (stream->monitor) { struct lttng_consumer_channel *free_chan = NULL; diff --git a/src/common/consumer/consumer.c b/src/common/consumer/consumer.c index 3a102b467..fcb100e1a 100644 --- a/src/common/consumer/consumer.c +++ b/src/common/consumer/consumer.c @@ -185,7 +185,6 @@ static void clean_channel_stream_list(struct lttng_consumer_channel *channel) /* Delete streams that might have been left in the stream list. */ cds_list_for_each_entry_safe(stream, stmp, &channel->streams.head, send_node) { - cds_list_del(&stream->send_node); /* * Once a stream is added to this list, the buffers were created so we * have a guarantee that this call will succeed. Setting the monitor diff --git a/src/common/consumer/consumer.h b/src/common/consumer/consumer.h index edde62c7e..7bfd28379 100644 --- a/src/common/consumer/consumer.h +++ b/src/common/consumer/consumer.h @@ -416,6 +416,11 @@ struct lttng_consumer_stream { struct lttng_ht_node_u64 node_channel_id; /* HT node used in consumer_data.stream_list_ht */ struct lttng_ht_node_u64 node_session_id; + /* + * List used by channels to reference streams that are not yet globally + * visible. + */ + struct cds_list_head send_node; /* Pointer to associated channel. */ struct lttng_consumer_channel *chan; /* @@ -548,7 +553,6 @@ struct lttng_consumer_stream { char name[LTTNG_SYMBOL_NAME_LEN]; /* Internal state of libustctl. */ struct ustctl_consumer_stream *ustream; - struct cds_list_head send_node; /* On-disk circular buffer */ uint64_t tracefile_size_current; uint64_t tracefile_count_current; diff --git a/src/common/kernel-consumer/kernel-consumer.c b/src/common/kernel-consumer/kernel-consumer.c index c23caa836..b16a1d053 100644 --- a/src/common/kernel-consumer/kernel-consumer.c +++ b/src/common/kernel-consumer/kernel-consumer.c @@ -437,7 +437,6 @@ static int lttng_kconsumer_snapshot_metadata( ret = 0; error_snapshot: metadata_stream->read_subbuffer_ops.unlock(metadata_stream); - cds_list_del(&metadata_stream->send_node); consumer_stream_destroy(metadata_stream, NULL); metadata_channel->metadata_stream = NULL; rcu_read_unlock(); diff --git a/src/common/ust-consumer/ust-consumer.c b/src/common/ust-consumer/ust-consumer.c index f0d13ec44..83080edde 100644 --- a/src/common/ust-consumer/ust-consumer.c +++ b/src/common/ust-consumer/ust-consumer.c @@ -64,7 +64,7 @@ static void destroy_channel(struct lttng_consumer_channel *channel) health_code_update(); - cds_list_del(&stream->send_node); + cds_list_del_init(&stream->send_node); ustctl_destroy_stream(stream->ustream); lttng_trace_chunk_put(stream->trace_chunk); free(stream); @@ -200,7 +200,7 @@ static int send_stream_to_thread(struct lttng_consumer_stream *stream, * global. */ stream->globally_visible = 1; - cds_list_del(&stream->send_node); + cds_list_del_init(&stream->send_node); ret = lttng_pipe_write(stream_pipe, &stream, sizeof(stream)); if (ret < 0) { @@ -975,7 +975,6 @@ error: * will make sure to clean that list. */ consumer_stream_destroy(metadata->metadata_stream, NULL); - cds_list_del(&metadata->metadata_stream->send_node); metadata->metadata_stream = NULL; send_streams_error: error_no_stream: @@ -1058,7 +1057,6 @@ error_stream: * new metadata stream. */ consumer_stream_destroy(metadata_stream, NULL); - cds_list_del(&metadata_stream->send_node); metadata_channel->metadata_stream = NULL; error: -- 2.34.1