Skip to content

Commit

Permalink
channel: don't disconnect a channel that is still being established
Browse files Browse the repository at this point in the history
Disconnecting the spice DISPLAY channel too fast can cause QEMU to
segfault inside libspiceserver, instead let's be nice and delay
disconnection requests until the full connection has been established.

This should be noted and debugged to have fixed upstream as it's
possible to crash a VM by abusing this issue.
  • Loading branch information
gnif committed May 23, 2022
1 parent 5873031 commit 1a91bbb
Show file tree
Hide file tree
Showing 4 changed files with 32 additions and 14 deletions.
34 changes: 22 additions & 12 deletions src/channel.c
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ PS_STATUS channel_connect(PSChannel * channel)
{
PS_STATUS status;

channel->doDisconnect = false;
channel->initDone = false;
channel->ackFrequency = 0;
channel->ackCount = 0;
Expand Down Expand Up @@ -102,7 +103,7 @@ PS_STATUS channel_connect(PSChannel * channel)
const SpiceLinkHeader * p = channel->getConnectPacket();
if ((size_t)channel_writeNL(channel, p, p->size + sizeof(*p)) != p->size + sizeof(*p))
{
channel_disconnect(channel);
channel_internal_disconnect(channel);
PS_LOG_ERROR("Failed to write the connect packet");
return PS_STATUS_ERROR;
}
Expand All @@ -111,22 +112,22 @@ PS_STATUS channel_connect(PSChannel * channel)
if ((status = channel_readNL(channel, &header, sizeof(header),
NULL)) != PS_STATUS_OK)
{
channel_disconnect(channel);
channel_internal_disconnect(channel);
PS_LOG_ERROR("Failed to read the reply to the connect packet");
return status;
}

if (header.magic != SPICE_MAGIC ||
header.major_version != SPICE_VERSION_MAJOR)
{
channel_disconnect(channel);
channel_internal_disconnect(channel);
PS_LOG_ERROR("Invalid spice magic and or version");
return PS_STATUS_ERROR;
}

if (header.size < sizeof(SpiceLinkReply))
{
channel_disconnect(channel);
channel_internal_disconnect(channel);
PS_LOG_ERROR("First message < sizeof(SpiceLinkReply)");
return PS_STATUS_ERROR;
}
Expand All @@ -135,13 +136,13 @@ PS_STATUS channel_connect(PSChannel * channel)
if ((status = channel_readNL(channel, reply, header.size,
NULL)) != PS_STATUS_OK)
{
channel_disconnect(channel);
channel_internal_disconnect(channel);
return status;
}

if (reply->error != SPICE_LINK_ERR_OK)
{
channel_disconnect(channel);
channel_internal_disconnect(channel);
PS_LOG_ERROR("Server reported link error: %d", reply->error);
return PS_STATUS_ERROR;
}
Expand All @@ -160,23 +161,23 @@ PS_STATUS channel_connect(PSChannel * channel)
auth.auth_mechanism = SPICE_COMMON_CAP_AUTH_SPICE;
if (channel_writeNL(channel, &auth, sizeof(auth)) != sizeof(auth))
{
channel_disconnect(channel);
channel_internal_disconnect(channel);
PS_LOG_ERROR("Failed to write the auth mechanisim packet");
return PS_STATUS_ERROR;
}

PSPassword pass;
if (!rsa_encryptPassword(reply->pub_key, g_ps.config.password, &pass))
{
channel_disconnect(channel);
channel_internal_disconnect(channel);
PS_LOG_ERROR("Failed to encrypt the password");
return PS_STATUS_ERROR;
}

if (channel_writeNL(channel, pass.data, pass.size) != pass.size)
{
rsa_freePassword(&pass);
channel_disconnect(channel);
channel_internal_disconnect(channel);
PS_LOG_ERROR("Failed to write the encrypted password");
return PS_STATUS_ERROR;
}
Expand All @@ -187,14 +188,14 @@ PS_STATUS channel_connect(PSChannel * channel)
if ((status = channel_readNL(channel, &linkResult, sizeof(linkResult),
NULL)) != PS_STATUS_OK)
{
channel_disconnect(channel);
channel_internal_disconnect(channel);
PS_LOG_ERROR("Failed to read the authentication response");
return status;
}

if (linkResult != SPICE_LINK_ERR_OK)
{
channel_disconnect(channel);
channel_internal_disconnect(channel);
PS_LOG_ERROR("Server reported link error: %u", linkResult);
return PS_STATUS_ERROR;
}
Expand All @@ -210,7 +211,7 @@ PS_STATUS channel_connect(PSChannel * channel)
return PS_STATUS_OK;
}

void channel_disconnect(PSChannel * channel)
void channel_internal_disconnect(PSChannel * channel)
{
if (!channel->connected)
return;
Expand Down Expand Up @@ -252,6 +253,15 @@ void channel_disconnect(PSChannel * channel)
free(channel->buffer);
channel->buffer = NULL;
channel->connected = false;
channel->doDisconnect = false;
}

void channel_disconnect(PSChannel * channel)
{
if (!channel->connected)
return;

channel->doDisconnect = true;
}

static PS_STATUS onMessage_setAck(PSChannel * channel)
Expand Down
2 changes: 2 additions & 0 deletions src/channel.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ Place, Suite 330, Boston, MA 02111-1307 USA

PS_STATUS channel_connect(PSChannel * channel);

void channel_internal_disconnect(PSChannel * channel);

void channel_disconnect(PSChannel * channel);

PSHandlerFn channel_onMessage(PSChannel * channel);
Expand Down
9 changes: 7 additions & 2 deletions src/ps.c
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ void purespice_disconnect()
g_ps.connected = false;

for(int i = PS_CHANNEL_MAX - 1; i >= 0; --i)
channel_disconnect(&g_ps.channels[i]);
channel_internal_disconnect(&g_ps.channels[i]);

close(g_ps.epollfd);

Expand Down Expand Up @@ -518,10 +518,15 @@ PSStatus purespice_process(int timeout)
done_disconnect:
++done;
events[i].data.ptr = NULL;
channel_disconnect(channel);
channel_internal_disconnect(channel);
}
}

// check for pending disconnects
for(int i = 0; i < PS_CHANNEL_MAX; ++i)
if (g_ps.channels[i].initDone && g_ps.channels[i].doDisconnect)
channel_internal_disconnect(&g_ps.channels[i]);

for(int i = 0; i < PS_CHANNEL_MAX; ++i)
if (g_ps.channels[i].connected)
return PS_STATUS_RUN;
Expand Down
1 change: 1 addition & 0 deletions src/ps.h
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ struct PSChannel

bool connected;
bool ready;
bool doDisconnect;
bool initDone;
int socket;
uint32_t ackFrequency;
Expand Down

0 comments on commit 1a91bbb

Please sign in to comment.