[PATCH 0/2] fix use-after-free in network IO
Found by repeatedly created and destroyed channels with guest agent. More details in the patch. Oleg Vasilev (2): net: add debug logs remote: fix stream use-after-free src/remote/remote_daemon_stream.c | 13 +++++++------ src/rpc/virnetmessage.c | 5 +++++ 2 files changed, 12 insertions(+), 6 deletions(-) -- 2.41.0
Helped to debug next patch use-after-free. Signed-off-by: Oleg Vasilev <oleg.vasilev@virtuozzo.com> --- src/remote/remote_daemon_stream.c | 4 ++-- src/rpc/virnetmessage.c | 5 +++++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/remote/remote_daemon_stream.c b/src/remote/remote_daemon_stream.c index 3b7519d2cb..38a2b6cceb 100644 --- a/src/remote/remote_daemon_stream.c +++ b/src/remote/remote_daemon_stream.c @@ -318,8 +318,8 @@ daemonStreamFilter(virNetServerClient *client, msg->header.serial != stream->serial) goto cleanup; - VIR_DEBUG("Incoming client=%p, rx=%p, serial=%u, proc=%d, status=%d", - client, stream->rx, msg->header.proc, + VIR_DEBUG("Incoming client=%p, rx=%p, msg=%p, serial=%u, proc=%d, status=%d", + client, stream->rx, msg, msg->header.proc, msg->header.serial, msg->header.status); virNetMessageQueuePush(&stream->rx, msg); diff --git a/src/rpc/virnetmessage.c b/src/rpc/virnetmessage.c index 50cc335fd6..af0f9cb30b 100644 --- a/src/rpc/virnetmessage.c +++ b/src/rpc/virnetmessage.c @@ -103,6 +103,8 @@ void virNetMessageQueuePush(virNetMessage **queue, virNetMessage *msg) { virNetMessage *tmp = *queue; + VIR_DEBUG("queue=%p msg=%p", queue, msg); + if (tmp) { while (tmp->next) tmp = tmp->next; @@ -117,10 +119,13 @@ virNetMessage *virNetMessageQueueServe(virNetMessage **queue) { virNetMessage *tmp = *queue; + VIR_DEBUG("queue serve start queue=%p *queue=%p", queue, *queue); + if (tmp) { *queue = g_steal_pointer(&tmp->next); } + VIR_DEBUG("queue serve end queue=%p *queue=%p", queue, *queue); return tmp; } -- 2.41.0
Inside daemonStreamHandleWrite on stream completion (status=OK) we reuse msg object to send confirmation. Only after that, msg is poped from the queue and checked for continue. By that time, msg might've already been processed for the confirmation and freed. Signed-off-by: Oleg Vasilev <oleg.vasilev@virtuozzo.com> --- src/remote/remote_daemon_stream.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/remote/remote_daemon_stream.c b/src/remote/remote_daemon_stream.c index 38a2b6cceb..345c40b48c 100644 --- a/src/remote/remote_daemon_stream.c +++ b/src/remote/remote_daemon_stream.c @@ -740,10 +740,11 @@ static int daemonStreamHandleWrite(virNetServerClient *client, daemonClientStream *stream) { + virNetMessageStatus status = VIR_NET_OK; VIR_DEBUG("client=%p, stream=%p", client, stream); while (stream->rx && !stream->closed) { - virNetMessage *msg = stream->rx; + virNetMessage *msg = virNetMessageQueueServe(&stream->rx); int ret; if (msg->header.type == VIR_NET_STREAM_HOLE) { @@ -752,7 +753,8 @@ daemonStreamHandleWrite(virNetServerClient *client, * data. */ ret = daemonStreamHandleHole(client, stream, msg); } else if (msg->header.type == VIR_NET_STREAM) { - switch (msg->header.status) { + status = msg->header.status; + switch (status) { case VIR_NET_OK: ret = daemonStreamHandleFinish(client, stream, msg); break; @@ -776,7 +778,6 @@ daemonStreamHandleWrite(virNetServerClient *client, if (ret > 0) break; /* still processing data from msg */ - virNetMessageQueueServe(&stream->rx); if (ret < 0) { virNetMessageFree(msg); virNetServerClientImmediateClose(client); @@ -789,7 +790,7 @@ daemonStreamHandleWrite(virNetServerClient *client, * onto the wire, but this causes the client to reset * its active request count / throttling */ - if (msg->header.status == VIR_NET_CONTINUE) { + if (status == VIR_NET_CONTINUE) { virNetMessageClear(msg); msg->header.type = VIR_NET_REPLY; if (virNetServerClientSendMessage(client, msg) < 0) { -- 2.41.0
On 04/07/2023 13:10, Oleg Vasilev wrote:
Found by repeatedly created and destroyed channels with guest agent. More details in the patch.
Oleg Vasilev (2): net: add debug logs remote: fix stream use-after-free
src/remote/remote_daemon_stream.c | 13 +++++++------ src/rpc/virnetmessage.c | 5 +++++ 2 files changed, 12 insertions(+), 6 deletions(-)
Please look at. On 13/07/2023 12:49, Oleg Vasilev wrote:
On 04/07/2023 13:10, Oleg Vasilev wrote:
Found by repeatedly created and destroyed channels with guest agent. More details in the patch.
Oleg Vasilev (2): net: add debug logs remote: fix stream use-after-free
src/remote/remote_daemon_stream.c | 13 +++++++------ src/rpc/virnetmessage.c | 5 +++++ 2 files changed, 12 insertions(+), 6 deletions(-)
On 7/4/23 09:10, Oleg Vasilev wrote:
Found by repeatedly created and destroyed channels with guest agent. More details in the patch.
Oleg Vasilev (2): net: add debug logs remote: fix stream use-after-free
src/remote/remote_daemon_stream.c | 13 +++++++------ src/rpc/virnetmessage.c | 5 +++++ 2 files changed, 12 insertions(+), 6 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> and pushed. Michal
participants (2)
-
Michal Prívozník -
Oleg Vasilev