[Spice-devel] [PATCH spice-server 2/4] spicevmc: Avoids DoS if client is not able to get data faster enough

[Victor Toso]

On Mon, Jun 17, 2019 at 04:40:09PM +0100, Frediano Ziglio wrote:
> This fix half (one direction) of
> https://gitlab.freedesktop.org/spice/spice/issues/29.
> Specifically if you have attempt to transfer a file to the client
> using WebDAV.
> Previously the queue to the client was unbound. If client was not
> getting data fast enough the server started queuing data.
> To easily test this you can use a tool to limit bandwidth and
> transfer a big file (I used a "dd if=/dev/zero bs=1M of=test") from
> the guest.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/spicevmc.c | 33 ++++++++++++++++++++++++++-------
>  1 file changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/server/spicevmc.c b/server/spicevmc.c
> index 8c41573ae..02e90199c 100644
> --- a/server/spicevmc.c
> +++ b/server/spicevmc.c
> @@ -34,14 +34,14 @@
>  #include "reds.h"
>  #include "migration-protocol.h"
>  
> -/* todo: add flow control. i.e.,
> - * (a) limit the tokens available for the client
> - * (b) limit the tokens available for the server
> - */
>  /* 64K should be enough for all but the largest writes + 32 bytes hdr */
>  #define BUF_SIZE (64 * 1024 + 32)
>  #define COMPRESS_THRESHOLD 1000
>  
> +// limit of the queued data, at this limit we stop reading from device to
> +// avoid DoS
> +#define QUEUED_DATA_LIMIT (1024*1024)
> +

Looks fine,
Acked-by: Victor Toso <victortoso at redhat.com>

>  SPICE_DECLARE_TYPE(RedCharDeviceSpiceVmc, red_char_device_spicevmc, CHAR_DEVICE_SPICEVMC);
>  #define RED_TYPE_CHAR_DEVICE_SPICEVMC red_char_device_spicevmc_get_type()
>  
> @@ -83,6 +83,7 @@ struct RedCharDeviceSpiceVmcClass
>  static RedCharDevice *red_char_device_spicevmc_new(SpiceCharDeviceInstance *sin,
>                                                     RedsState *reds,
>                                                     RedVmcChannel *channel);
> +static void spicevmc_red_channel_queue_data(RedVmcChannel *channel, RedVmcPipeItem *item);
>  
>  G_DEFINE_TYPE(RedCharDeviceSpiceVmc, red_char_device_spicevmc, RED_TYPE_CHAR_DEVICE)
>  
> @@ -96,6 +97,7 @@ struct RedVmcChannel
>      RedVmcPipeItem *pipe_item;
>      RedCharDeviceWriteBuffer *recv_from_client_buf;
>      uint8_t port_opened;
> +    uint32_t queued_data;
>      RedStatCounter in_data;
>      RedStatCounter in_compressed;
>      RedStatCounter in_decompressed;
> @@ -337,7 +339,7 @@ static RedPipeItem *spicevmc_chardev_read_msg_from_dev(RedCharDevice *self,
>  
>      sif = spice_char_device_get_interface(sin);
>  
> -    if (!channel->rcc) {
> +    if (!channel->rcc || channel->queued_data >= QUEUED_DATA_LIMIT) {
>          return NULL;
>      }
>  
> @@ -360,14 +362,14 @@ static RedPipeItem *spicevmc_chardev_read_msg_from_dev(RedCharDevice *self,
>  
>          msg_item_compressed = try_compress_lz4(channel, n, msg_item);
>          if (msg_item_compressed != NULL) {
> -            red_channel_client_pipe_add_push(channel->rcc, &msg_item_compressed->base);
> +            spicevmc_red_channel_queue_data(channel, msg_item_compressed);
>              return NULL;
>          }
>  #endif
>          stat_inc_counter(channel->out_data, n);
>          msg_item->uncompressed_data_size = n;
>          msg_item->buf_used = n;
> -        red_channel_client_pipe_add_push(channel->rcc, &msg_item->base);
> +        spicevmc_red_channel_queue_data(channel, msg_item);
>          return NULL;
>      }
>      channel->pipe_item = msg_item;
> @@ -609,11 +611,19 @@ static void spicevmc_red_channel_release_msg_rcv_buf(RedChannelClient *rcc,
>      }
>  }
>  
> +static void
> +spicevmc_red_channel_queue_data(RedVmcChannel *channel, RedVmcPipeItem *item)
> +{
> +    channel->queued_data += item->buf_used;
> +    red_channel_client_pipe_add_push(channel->rcc, &item->base);
> +}
> +
>  static void spicevmc_red_channel_send_data(RedChannelClient *rcc,
>                                             SpiceMarshaller *m,
>                                             RedPipeItem *item)
>  {
>      RedVmcPipeItem *i = SPICE_UPCAST(RedVmcPipeItem, item);
> +    RedVmcChannel *channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
>  
>      /* for compatibility send using not compressed data message */
>      if (i->type == SPICE_DATA_COMPRESSION_TYPE_NONE) {
> @@ -630,6 +640,14 @@ static void spicevmc_red_channel_send_data(RedChannelClient *rcc,
>      red_pipe_item_ref(item);
>      spice_marshaller_add_by_ref_full(m, i->buf, i->buf_used,
>                                       marshaller_unref_pipe_item, item);
> +
> +    // account for sent data and wake up device if was blocked
> +    uint32_t old_queued_data = channel->queued_data;
> +    channel->queued_data -= i->buf_used;
> +    if (channel->chardev &&
> +        old_queued_data >= QUEUED_DATA_LIMIT && channel->queued_data < QUEUED_DATA_LIMIT) {
> +        red_char_device_wakeup(channel->chardev);
> +    }
>  }
>  
>  static void spicevmc_red_channel_send_migrate_data(RedChannelClient *rcc,
> @@ -768,6 +786,7 @@ static void spicevmc_connect(RedChannel *channel, RedClient *client,
>          return;
>      }
>      vmc_channel->rcc = rcc;
> +    vmc_channel->queued_data = 0;
>      red_channel_client_ack_zero_messages_window(rcc);
>  
>      if (strcmp(sin->subtype, "port") == 0) {
> -- 
> 2.20.1
> 
> _______________________________________________
> 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: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190924/19b05cad/attachment.sig>