[Spice-devel] [vdagent-linux v8 1/4] vdagentd: early return on bad message size
Victor Toso
victortoso at redhat.com
Wed Feb 15 09:58:14 UTC 2017
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"
(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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170215/69547e53/attachment.sig>
More information about the Spice-devel
mailing list