[Spice-devel] [vdagent-linux v8 1/4] vdagentd: early return on bad message size
Christophe Fergeau
cfergeau at redhat.com
Thu Feb 16 14:05:25 UTC 2017
On Wed, Feb 15, 2017 at 10:58:14AM +0100, Victor Toso wrote:
> Hi,
>
> On Wed, Feb 15, 2017 at 10:48:04AM +0100, Victor Toso wrote:
> > From: Victor Toso <me at victortoso.com>
> >
> > 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().
>
> This is not 100% true anymore. We've also created a
> vdagent_message_check_size() which is the one that uses the
> vdagent_message_min_size[].
>
> Maybe:
>
> "This patch creates:
> * vdagent_message_min_size[] static array with the
> expected size for each message;
> * vdagent_message_check_size() which checks the size of message's
> payload based on the type of message and by using
> vdagent_message_min_size[] as reference"
Yup, looks good this way and the patch overall looked fine as I remember
it ;)
Christophe
>
> (maybe it got worse, comments are welcome :))
>
> >
> > 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 5ca0106..ea8482b 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;
>
> I do prefer creating a VDAgentMaxClipboard variable here as it was
> before.
>
> PS: I'll not ack a patch that I helped to write so, let me know if any
> other changes are needed here!
>
> toso
> > + 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
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- 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/20170216/5b2796f7/attachment-0001.sig>
More information about the Spice-devel
mailing list