Hey, On Mon, Jan 23, 2017 at 02:53:54PM +0100, Michal Suchanek wrote: > From: Victor Toso <me@xxxxxxxxxxxxxx> > > 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@xxxxxxxxxx> > Signed-off-by: Michal Suchanek <msuchanek@xxxxxxx> > --- > v6 > - bring back the proper size check for clipboard messages > - sort messages to fixed and variable length and check size accordingly > - size the vdagent_message_min_size array with VD_AGENT_END_MESSAGE Fwiw, not all comments from Frediano in https://lists.freedesktop.org/archives/spice-devel/2017-January/035095.html were answered (I'd also at least use G_N_ELEMENTS rather than a direct check against VD_AGENT_END_MESSAGE). > --- > src/vdagentd/vdagentd.c | 111 +++++++++++++++++++++++++++++++++++++----------- > 1 file changed, 86 insertions(+), 25 deletions(-) > > diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c > index 785ce50..7e16cd2 100644 > --- a/src/vdagentd/vdagentd.c > +++ b/src/vdagentd/vdagentd.c > @@ -341,6 +341,88 @@ 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[VD_AGENT_END_MESSAGE] = > +{ > + -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(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 >= VD_AGENT_END_MESSAGE) { > + 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_FILE_XFER_START: > + case VD_AGENT_FILE_XFER_DATA: > + case VD_AGENT_CLIPBOARD: > + case VD_AGENT_CLIPBOARD_GRAB: > + 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_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) { > + syslog(LOG_ERR, "read: invalid message size: %u for message type: %u", > + message_header->size, message_header->type); > + return FALSE; > + } > + break; 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 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. > + default: > + g_warn_if_reached(); > + return FALSE; > + } > + return TRUE; > +} > + > static int virtio_port_read_complete( > struct vdagent_virtio_port *vport, > int port_nr, > @@ -349,26 +431,18 @@ static int virtio_port_read_complete( > { > uint32_t min_size = 0; With the introduction of vdagent_message_check_size(), this variable is no longer needed... > > - 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; > @@ -388,9 +462,6 @@ static int virtio_port_read_complete( > VD_AGENT_CAP_CLIPBOARD_SELECTION)) { > min_size += 4; > } ... once you remove the block before this which still computes min_size, which is no longer used. Christophe
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel