[Spice-devel] [PATCH v6 4/5] vdagentd: early return on bad message size
Christophe Fergeau
cfergeau at redhat.com
Mon Jan 30 12:11:40 UTC 2017
On Fri, Jan 27, 2017 at 03:37:17PM +0100, Michal Suchánek wrote:
> On Wed, 25 Jan 2017 08:24:55 +0100
> Christophe Fergeau <cfergeau at redhat.com> wrote:
>
> > On Tue, Jan 24, 2017 at 07:52:11PM +0100, Michal Suchánek wrote:
> > > On Tue, 24 Jan 2017 09:45:37 +0100
> > > Christophe Fergeau <cfergeau at redhat.com> wrote:
> > > >
> > > > In this big switch, VD_AGENT_FILE_XFER_* and
> > > > VD_AGENT_CLIENT_DISCONNECTED were previously not subject to
> > > > a size check, VD_AGENT_DISPLAY_CONFIG and VD_AGENT_REPLY did not
> > > > appear in the switch, and
> > >
> > > They appear in the min_size array, however. So they should be
> > > handled. This is now a separate size check function that can be
> > > used to check check size of any message, even messages the
> > > dispatcher does not handle.
> > >
> > > Also the XFER messages need to be size checked later when they are
> > > swapped. Actually the fields of the messages are accessed already
> > > so it is an error to not check they are actually present.
> > >
> > > > VD_AGENT_CLIPBOARD_RELEASE/VD_AGENT_CLIPBOARD_RELEASE were doing
> > > > a < comparison, not a !=. I'm not necessarily opposed to these
> > > > changes, but I'd keep them for an additional commit, or at least
> > > > explain why this is ok to do in the commit log.
> > >
> > > This is definitely because they were lumped together with the
> > > variable length clipboard messages.
> >
> > Yeah, as I said, I don't think it's wrong to have them there, but I'd
> > add them in a separate message, so that there's a mostly mechanical
> > commit which adds the new function, but is strictly equivalent to the
> > old code in terms of what is tested and how it's tested. Then once you
> > have the function, you can refine things, add missing enum values,
> > refine the <= vs != tests, ...
>
> There was never a point when the code was equivalent after removing the
> size checks from the dispatch switch and putting them into a separate
> piece of code.
>
> There were unintentional errors in the first attempt taking out the
> pieces one by one and collecting them elsewhere.
>
> So here I just implement new separate check which is based on the
> headers which define the messages as variable or fixed size and remove
> the old checks which were based on historical evolution and were
> obviously more or less incorrect in places.
>
> There was never an intent to make to code bug to bug equivalent.
I did not imply there was such an intent/such a time, however from a
maintainability perspective (easier review, more accurate git bisect in
case of problems, ..), I think it's better to have first a code-movement
only patch not changing behaviour, and then adjusting the size checks,
with proper documentation in the log as to why they are valid changes.
This way, the only big patch is the code movement one, and the reviewer
can focus on seeing if everything is still the same, rather than having
to figure that some part are the same, and then guess the reasons why
other parts are not the same. And if git bisect ever point at this
commit, it's clear that before/after this patch, there should be *no*
behaviour changes.
Then you can have smaller patches on top of that adjusting the new
function to add missing size checks, adjust too lax ones, ...
I've tried to do this in the attached patches.
Christophe
-------------- next part --------------
From d8a09b50f80e5423823fdd1189cbaaabede70216 Mon Sep 17 00:00:00 2001
From: Victor Toso <me at victortoso.com>
Date: Mon, 23 Jan 2017 14:53:54 +0100
Subject: [vdagent-linux 1/3] vdagentd: early return on bad message size
The payload size for each message should be the size of the expected
struct or bigger when it contain arrays of no fixed size.
This patch creates the vdagent_message_min_size[] static array with
the expected size for each message so we can check on
virtio_port_read_complete().
Signed-off-by: Victor Toso <victortoso at redhat.com>
Signed-off-by: Michal Suchanek <msuchanek at suse.de>
---
src/vdagentd/vdagentd.c | 133 +++++++++++++++++++++++++++++++++---------------
1 file changed, 91 insertions(+), 42 deletions(-)
diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
index 785ce50..7855611 100644
--- a/src/vdagentd/vdagentd.c
+++ b/src/vdagentd/vdagentd.c
@@ -341,34 +341,109 @@ static void do_client_file_xfer(struct vdagent_virtio_port *vport,
udscs_write(conn, msg_type, 0, 0, data, message_header->size);
}
+static gsize vdagent_message_min_size[] =
+{
+ -1, /* Does not exist */
+ sizeof(VDAgentMouseState), /* VD_AGENT_MOUSE_STATE */
+ sizeof(VDAgentMonitorsConfig), /* VD_AGENT_MONITORS_CONFIG */
+ sizeof(VDAgentReply), /* VD_AGENT_REPLY */
+ sizeof(VDAgentClipboard), /* VD_AGENT_CLIPBOARD */
+ sizeof(VDAgentDisplayConfig), /* VD_AGENT_DISPLAY_CONFIG */
+ sizeof(VDAgentAnnounceCapabilities), /* VD_AGENT_ANNOUNCE_CAPABILITIES */
+ sizeof(VDAgentClipboardGrab), /* VD_AGENT_CLIPBOARD_GRAB */
+ sizeof(VDAgentClipboardRequest), /* VD_AGENT_CLIPBOARD_REQUEST */
+ sizeof(VDAgentClipboardRelease), /* VD_AGENT_CLIPBOARD_RELEASE */
+ sizeof(VDAgentFileXferStartMessage), /* VD_AGENT_FILE_XFER_START */
+ sizeof(VDAgentFileXferStatusMessage), /* VD_AGENT_FILE_XFER_STATUS */
+ sizeof(VDAgentFileXferDataMessage), /* VD_AGENT_FILE_XFER_DATA */
+ 0, /* VD_AGENT_CLIENT_DISCONNECTED */
+ sizeof(VDAgentMaxClipboard), /* VD_AGENT_MAX_CLIPBOARD */
+ sizeof(VDAgentAudioVolumeSync), /* VD_AGENT_AUDIO_VOLUME_SYNC */
+};
+
+static gboolean vdagent_message_check_size(const VDAgentMessage *message_header)
+{
+ uint32_t min_size = 0;
+
+ if (message_header->protocol != VD_AGENT_PROTOCOL) {
+ syslog(LOG_ERR, "message with wrong protocol version ignoring");
+ return FALSE;
+ }
+
+ if (!message_header->type ||
+ message_header->type >= G_N_ELEMENTS(vdagent_message_min_size)) {
+ syslog(LOG_WARNING, "unknown message type %d, ignoring",
+ message_header->type);
+ return FALSE;
+ }
+
+ min_size = vdagent_message_min_size[message_header->type];
+ if (VD_AGENT_HAS_CAPABILITY(capabilities, capabilities_size,
+ VD_AGENT_CAP_CLIPBOARD_SELECTION)) {
+ switch (message_header->type) {
+ case VD_AGENT_CLIPBOARD_GRAB:
+ case VD_AGENT_CLIPBOARD_REQUEST:
+ case VD_AGENT_CLIPBOARD:
+ case VD_AGENT_CLIPBOARD_RELEASE:
+ min_size += 4;
+ }
+ }
+
+ switch (message_header->type) {
+ case VD_AGENT_MONITORS_CONFIG:
+ case VD_AGENT_CLIPBOARD:
+ case VD_AGENT_CLIPBOARD_GRAB:
+ case VD_AGENT_CLIPBOARD_REQUEST:
+ case VD_AGENT_CLIPBOARD_RELEASE:
+ case VD_AGENT_AUDIO_VOLUME_SYNC:
+ case VD_AGENT_ANNOUNCE_CAPABILITIES:
+ if (message_header->size < min_size) {
+ syslog(LOG_ERR, "read: invalid message size: %u for message type: %u",
+ message_header->size, message_header->type);
+ return FALSE;
+ }
+ break;
+ case VD_AGENT_MOUSE_STATE:
+ case VD_AGENT_MAX_CLIPBOARD:
+ if (message_header->size != min_size) {
+ syslog(LOG_ERR, "read: invalid message size: %u for message type: %u",
+ message_header->size, message_header->type);
+ return FALSE;
+ }
+ break;
+ case VD_AGENT_FILE_XFER_START:
+ case VD_AGENT_FILE_XFER_DATA:
+ case VD_AGENT_FILE_XFER_STATUS:
+ case VD_AGENT_CLIENT_DISCONNECTED:
+ /* No size checks for these at the moment */
+ break;
+ case VD_AGENT_DISPLAY_CONFIG:
+ case VD_AGENT_REPLY:
+ default:
+ g_warn_if_reached();
+ return FALSE;
+ }
+ return TRUE;
+}
+
static int virtio_port_read_complete(
struct vdagent_virtio_port *vport,
int port_nr,
VDAgentMessage *message_header,
uint8_t *data)
{
- uint32_t min_size = 0;
-
- if (message_header->protocol != VD_AGENT_PROTOCOL) {
- syslog(LOG_ERR, "message with wrong protocol version ignoring");
+ if (!vdagent_message_check_size(message_header))
return 0;
- }
switch (message_header->type) {
case VD_AGENT_MOUSE_STATE:
- if (message_header->size != sizeof(VDAgentMouseState))
- goto size_error;
do_client_mouse(&uinput, (VDAgentMouseState *)data);
break;
case VD_AGENT_MONITORS_CONFIG:
- if (message_header->size < sizeof(VDAgentMonitorsConfig))
- goto size_error;
do_client_monitors(vport, port_nr, message_header,
(VDAgentMonitorsConfig *)data);
break;
case VD_AGENT_ANNOUNCE_CAPABILITIES:
- if (message_header->size < sizeof(VDAgentAnnounceCapabilities))
- goto size_error;
do_client_capabilities(vport, message_header,
(VDAgentAnnounceCapabilities *)data);
break;
@@ -376,21 +451,6 @@ static int virtio_port_read_complete(
case VD_AGENT_CLIPBOARD_REQUEST:
case VD_AGENT_CLIPBOARD:
case VD_AGENT_CLIPBOARD_RELEASE:
- switch (message_header->type) {
- case VD_AGENT_CLIPBOARD_GRAB:
- min_size = sizeof(VDAgentClipboardGrab); break;
- case VD_AGENT_CLIPBOARD_REQUEST:
- min_size = sizeof(VDAgentClipboardRequest); break;
- case VD_AGENT_CLIPBOARD:
- min_size = sizeof(VDAgentClipboard); break;
- }
- if (VD_AGENT_HAS_CAPABILITY(capabilities, capabilities_size,
- VD_AGENT_CAP_CLIPBOARD_SELECTION)) {
- min_size += 4;
- }
- if (message_header->size < min_size) {
- goto size_error;
- }
do_client_clipboard(vport, message_header, data);
break;
case VD_AGENT_FILE_XFER_START:
@@ -402,31 +462,20 @@ static int virtio_port_read_complete(
vdagent_virtio_port_reset(vport, VDP_CLIENT_PORT);
do_client_disconnect();
break;
- case VD_AGENT_MAX_CLIPBOARD:
- if (message_header->size != sizeof(VDAgentMaxClipboard))
- goto size_error;
- VDAgentMaxClipboard *msg = (VDAgentMaxClipboard *)data;
- syslog(LOG_DEBUG, "Set max clipboard: %d", msg->max);
- max_clipboard = msg->max;
+ case VD_AGENT_MAX_CLIPBOARD: {
+ max_clipboard = ((VDAgentMaxClipboard *)data)->max;
+ syslog(LOG_DEBUG, "Set max clipboard: %d", max_clipboard);
break;
+ }
case VD_AGENT_AUDIO_VOLUME_SYNC:
- if (message_header->size < sizeof(VDAgentAudioVolumeSync))
- goto size_error;
-
do_client_volume_sync(vport, port_nr, message_header,
(VDAgentAudioVolumeSync *)data);
break;
default:
- syslog(LOG_WARNING, "unknown message type %d, ignoring",
- message_header->type);
+ g_warn_if_reached();
}
return 0;
-
-size_error:
- syslog(LOG_ERR, "read: invalid message size: %u for message type: %u",
- message_header->size, message_header->type);
- return 0;
}
static void virtio_write_clipboard(uint8_t selection, uint32_t msg_type,
--
2.9.3
-------------- next part --------------
From 8fe387c7b96223863de731575eb1e45c513b6fce Mon Sep 17 00:00:00 2001
From: Christophe Fergeau <cfergeau at redhat.com>
Date: Mon, 30 Jan 2017 12:57:55 +0100
Subject: [vdagent-linux 2/3] Add missing size checks
---
src/vdagentd/vdagentd.c | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
index 7855611..3fc7a45 100644
--- a/src/vdagentd/vdagentd.c
+++ b/src/vdagentd/vdagentd.c
@@ -391,6 +391,8 @@ static gboolean vdagent_message_check_size(const VDAgentMessage *message_header)
switch (message_header->type) {
case VD_AGENT_MONITORS_CONFIG:
+ case VD_AGENT_FILE_XFER_START:
+ case VD_AGENT_FILE_XFER_DATA:
case VD_AGENT_CLIPBOARD:
case VD_AGENT_CLIPBOARD_GRAB:
case VD_AGENT_CLIPBOARD_REQUEST:
@@ -404,21 +406,17 @@ static gboolean vdagent_message_check_size(const VDAgentMessage *message_header)
}
break;
case VD_AGENT_MOUSE_STATE:
- case VD_AGENT_MAX_CLIPBOARD:
- if (message_header->size != min_size) {
- syslog(LOG_ERR, "read: invalid message size: %u for message type: %u",
- message_header->size, message_header->type);
- return FALSE;
- }
- break;
- case VD_AGENT_FILE_XFER_START:
- case VD_AGENT_FILE_XFER_DATA:
case VD_AGENT_FILE_XFER_STATUS:
- case VD_AGENT_CLIENT_DISCONNECTED:
- /* No size checks for these at the moment */
- break;
case VD_AGENT_DISPLAY_CONFIG:
case VD_AGENT_REPLY:
+ case VD_AGENT_MAX_CLIPBOARD:
+ case VD_AGENT_CLIENT_DISCONNECTED:
+ if (message_header->size != min_size) {
+ syslog(LOG_ERR, "read: invalid message size: %u for message type: %u",
+ message_header->size, message_header->type);
+ return FALSE;
+ }
+ break;
default:
g_warn_if_reached();
return FALSE;
--
2.9.3
-------------- next part --------------
From b281b9e1929c4c762d2e45e563467921ade6cfa1 Mon Sep 17 00:00:00 2001
From: Christophe Fergeau <cfergeau at redhat.com>
Date: Mon, 30 Jan 2017 12:58:06 +0100
Subject: [vdagent-linux 3/3] Adjust size checks
---
src/vdagentd/vdagentd.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
index 3fc7a45..08df55e 100644
--- a/src/vdagentd/vdagentd.c
+++ b/src/vdagentd/vdagentd.c
@@ -395,8 +395,6 @@ static gboolean vdagent_message_check_size(const VDAgentMessage *message_header)
case VD_AGENT_FILE_XFER_DATA:
case VD_AGENT_CLIPBOARD:
case VD_AGENT_CLIPBOARD_GRAB:
- case VD_AGENT_CLIPBOARD_REQUEST:
- case VD_AGENT_CLIPBOARD_RELEASE:
case VD_AGENT_AUDIO_VOLUME_SYNC:
case VD_AGENT_ANNOUNCE_CAPABILITIES:
if (message_header->size < min_size) {
@@ -409,6 +407,8 @@ static gboolean vdagent_message_check_size(const VDAgentMessage *message_header)
case VD_AGENT_FILE_XFER_STATUS:
case VD_AGENT_DISPLAY_CONFIG:
case VD_AGENT_REPLY:
+ case VD_AGENT_CLIPBOARD_REQUEST:
+ case VD_AGENT_CLIPBOARD_RELEASE:
case VD_AGENT_MAX_CLIPBOARD:
case VD_AGENT_CLIENT_DISCONNECTED:
if (message_header->size != min_size) {
--
2.9.3
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170130/51b6693d/attachment-0001.sig>
More information about the Spice-devel
mailing list