>
> 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>
> ---
> src/vdagentd/vdagentd.c | 56
> ++++++++++++++++++++++++++++++-------------------
> 1 file changed, 34 insertions(+), 22 deletions(-)
>
> diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> index a1faf23..4eb8e80 100644
> --- a/src/vdagentd/vdagentd.c
> +++ b/src/vdagentd/vdagentd.c
> @@ -77,6 +77,25 @@ static int retval = 0;
> static int client_connected = 0;
> static int max_clipboard = -1;
>
> +static gsize vdagent_message_min_size[VD_AGENT_END_MESSAGE] = {
+const
what happens if protocol add some messages so VD_AGENT_END_MESSAGE
get increased?
I think would be better to declare as
static const gsize vdagent_message_min_size[] = {
so all messages listed are supported
I would use something smaller that gsize but I'm just paranoid.
> + -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 */
> +};
> +
> /* utility functions */
> /* vdagentd <-> spice-client communication handling */
> static void send_capabilities(struct vdagent_virtio_port *vport,
> @@ -329,10 +348,20 @@ static int virtio_port_read_complete(
> return 0;
> }
>
> + if (message_header->type >= VD_AGENT_END_MESSAGE) {
Here G_N_ELEMENTS and you should check for 0 too.
> + syslog(LOG_WARNING, "unknown message type %d, ignoring",
> + message_header->type);
> + return 0;
> + }
> +
> + if (message_header->size <
> vdagent_message_min_size[message_header->type]) {
> + syslog(LOG_ERR, "read: invalid message size: %u for message type:
> %u",
> + message_header->size, message_header->type);
> + return 0;
> + }
> +
> switch (message_header->type) {
> case VD_AGENT_MOUSE_STATE:
> - if (message_header->size != sizeof(VDAgentMouseState))
> - goto size_error;
what happens if message is longer? Previously an error, now I think
the rest of the message is ignored
> vdagentd_uinput_do_mouse(&uinput, (VDAgentMouseState *)data);
> if (!uinput) {
> /* Try to re-open the tablet */
> @@ -354,14 +383,10 @@ static int virtio_port_read_complete(
> }
> 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;
> @@ -381,9 +406,6 @@ static int virtio_port_read_complete(
> VD_AGENT_CAP_CLIPBOARD_SELECTION)) {
> min_size += 4;
> }
> - if (message_header->size < min_size) {
> - goto size_error;
> - }
This should not be removed, it checks for 4 bytes more than the standard.
You could also reuse the array you added to compute min_size in the
previous lines.
> do_client_clipboard(vport, message_header, data);
> break;
> case VD_AGENT_FILE_XFER_START:
> @@ -395,31 +417,21 @@ 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;
Same as VD_AGENT_MOUSE_STATE
> + case VD_AGENT_MAX_CLIPBOARD: {
> VDAgentMaxClipboard *msg = (VDAgentMaxClipboard *)data;
> syslog(LOG_DEBUG, "Set max clipboard: %d", msg->max);
> max_clipboard = msg->max;
> break;
> + }
Why the brackets are not necessary?
Could be written as
max_clipboard = ((VDAgentMaxClipboard *)data)->max;
syslog(LOG_DEBUG, "Set max clipboard: %d", max_clipboard);
> 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,
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel