When redirecting a USB webcam over a slow link, it's currently possible
to hit an assertion in spice-server by running cheese (application using
the webcam), killing the client with ctrl+c and then restarting the
client:
qemu-kvm: spicevmc.c:324: spicevmc_red_channel_alloc_msg_rcv_buf:
Assertion `!state->recv_from_client_buf' failed.
This happens when red_peer_handle_incoming tries to allocate memory for
a message using spicevmc:
handler->msg = handler->cb->alloc_msg_buf(handler->opaque, msg_type,
msg_size);
red_peer_handle_incoming() is called when there is client data to be
read, and does
- call alloc_msg_buf() to allocate memory for the message
- read the message
- if the read was partial, return early, the main loop will call again
red_peer_handle_incoming() when there is more data available for that
channel
- parse the message
- call release_msg_buf() to free the message
For channels based on spicevmc (usbredir and port), alloc_msg_buf()
stores message data in SpiceVmcState::recv_from_client_buf and before
allocating new memory, it asserts that it's NULL.
This is what causes this crash in the following scenario:
- SpiceVmc::alloc_msg_buf() is called and allocates memory for a new
message in SpiceVmcState::recv_from_client_buf
- red_peer_handle_incoming() returns early as all the spicevmc message
data hasn't been received yet
- the client gets killed
- the main channel notices the disconnect and calls
main_dispatcher_client_disconnect() which will disconnect all the
channels
- SpiceVmc::on_disconnect is called
- after the new client connects, SpiceVmc::alloc_msg_buf() is called,
notices that SpiceVmcState::recv_from_client_buf is already set, and
asserts()
This commit makes sure the partial SpiceVmcState::recv_from_client_buf
data is cleared on disconnect so that the assert does not trigger.
This fixes https://bugzilla.redhat.com/show_bug.cgi?id=1264113
In some case the member specified to SPICE_CONTAINEROF was not
exactly the same type of the pointer passed.
This can cause issues if structure changes so use proper member.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Fabiano Fidêncio <fidencio@redhat.com>
This avoids compilation errors with -Werror on 32 bit systems as the
pointer size differs from that of a QXLPHYSICAL.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
This avoids a compilation error with -Werror on 32 bit systems as the
pointer size differs from that of an uint64_t.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
The size_t definition is different between 32 and 64 bit systems so that
neither '%u' nor '%lu' work for both. '%zu' should be used instead.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Some integer type definitions are different between 32 and 64 bit
systems which causes problems in printf. The PRI macros automatically
provide the printf format appropriate for the system.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
After spice_char_device_state_destroy is called spicevmc should not keep
reference to that memory. state->chardev_st and sin->st point to the
same SpiceCharDeviceState and both should be set to NULL when it is
destroyed.
As SpiceCharDeviceState is only unref'ed on
spice_char_device_state_destroy the same device could be destroyed more
then once so the pointers that are freed should be set to NULL.
Related: https://bugzilla.redhat.com/show_bug.cgi?id=1281455
When no client is connect we should not need to keep the memory pool
used by char-device. In most situations this is not significant but
when using webdav this could mean freeing MAX_POOL_SIZE bytes
Related: https://bugs.freedesktop.org/show_bug.cgi?id=91350
Otherwise the amount of unused memory could grow while transfering big
chunks of data. This change only means that once the memory was used it
will not be stored again after the limit was reached.
Related: https://bugs.freedesktop.org/show_bug.cgi?id=91350
There are places were the could should definetly free the
SpiceCharDeviceWriteBuffer and places that it should only unref it. The
current use of spice_char_device_write_buffer_free was missleading.
This patch creates the spice_char_device_write_buffer_unref and properly
call these two functions.
Related: https://bugs.freedesktop.org/show_bug.cgi?id=91350
Don't disconnect the display channel, when unsupported compression is
requested from the client. Not changing the compression is enough.
https://bugs.freedesktop.org/show_bug.cgi?id=92821
Acked-by: Victor Toso <victortoso@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Same approach as in spice_server_char_device_wakeup().
Avoid segmentation fault when the webdav channel (spice port channel) is
used with the vnc display:
#0 0x00007ffff7aab734 in spice_char_device_state_opaque_get (dev=0x0)
at char_device.c:720
#1 0x00007ffff7b0850c in spice_server_port_event (sin=<optimized out>, event=<optimized out>) at spicevmc.c:578
#2 0x0000555555787ba4 in set_guest_connected (port=<optimized out>, guest_connected=1) at hw/char/virtio-console.c:89
#3 0x0000555555678d7c in control_out (len=<optimized out>, buf=0x55555775c3a0, vser=0x5555578d1540) at /home/pgrunt/RH/qemu/hw/char/virtio-serial-bus.c:404
#4 0x0000555555678d7c in control_out (vdev=0x5555578d1540, vq=0x555557941bc8)
at /home/pgrunt/RH/qemu/hw/char/virtio-serial-bus.c:441
#5 0x000055555588eb98 in aio_dispatch (ctx=0x5555562e1a50) at aio-posix.c:160
#6 0x00005555558829ee in aio_ctx_dispatch (source=<optimized out>, callback=<optimized out>, user_data=<optimized out>) at async.c:226
#7 0x00007ffff2010e3a in g_main_context_dispatch () at /lib64/libglib-2.0.so.0
#8 0x000055555588d8fb in main_loop_wait () at main-loop.c:211
#9 0x000055555588d8fb in main_loop_wait (timeout=<optimized out>)
at main-loop.c:256
#10 0x000055555588d8fb in main_loop_wait (nonblocking=<optimized out>)
at main-loop.c:504
#11 0x000055555561b664 in main () at vl.c:1891
This fixes a display glitch in xspice which is caused when
a surface create is queued, but then a direct call to update
the area is issued. Unless we flush the queue, the surface
does not exist, and we fail.
Signed-off-by: Jeremy White <jwhite@codeweavers.com>
With multiple cards configured you can have multiple workers running in
different thread.
With such configuration static variables not syncronized could lead
to undefined behavior.
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This commit also updates the spice-common submodule
Christophe Fergeau (7):
Add marshaller test case
build-sys: Use ${PKG_CONFIG} rather than pkg-config
build-sys: Rework SPICE_CHECK_* m4 macros
build-sys: Add gio-2.0 to SPICE_CHECK_GLIB2
build-sys: Fix error in SPICE_CHECK_LZ4 description
build-sys: Set automake conditional in SPICE_CHECK_SMARTCARD
build-sys: Rename SUPPORT_GL to HAVE_GL
Javier Celaya (1):
Fix linearization of several marshallers with one item
Lukas Venhoda (3):
ssl-verify: Only check addr length when using IP addr
m4: Require glib version >= 2.22
ssl-verify: Changed IPv4 hostname to IPv6
cinfo.dest is allocated in spice_jpeg_mem_dest but never freed.
Note that jpeg_destroy_compress does not free this field as is
supposed to be a buffer provided by jpeg caller.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
There was not check for data_size field so one could set data to
a small set of data and data_size much bigger than size of data
leading to buffer overflow.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
The guest can attempt to increase the number of segments while
spice-server is reading them.
Make sure we don't copy more then the allocated segments.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
start pointer points to a QXLPathSeg structure.
Before reading from the structure, make sure the structure is contained
in the memory range checked.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Limit number of chunks to a given amount to avoid guest trying to
allocate too much memory. Using circular or nested chunks lists
guest could try to allocate huge amounts of memory.
Considering the list can be infinite and guest can change data this
also prevents strange security attacks from guest.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Free linked list if client tries to do nasty things
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Do not read multiple times data from guest as this can be changed by
other guest vcpus. This causes races and security problems if these
data are used for buffer allocation or checks.
Actually, the 'data' member can't change during read as it is just a
pointer to a fixed array contained in qxl. However, this change will
make it clear that there can be no race condition.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
If bpp is int the formula can lead to weird overflows. width and height
are uint16_t so the formula is:
size_t = u16 * (u16 * int + const_int) / const_int;
so it became
size_t = (int) u16 * ((int) u16 * int + const_int) / const_int;
However the (int) u16 * (int) u16 can then became negative to overflow.
Under 64 bit architectures size_t is 64 and int usually 32 so converting
this negative 32 bit number to a unsigned 64 bit lead to a very big
number as the signed is extended and then converted to unsigned.
Using unsigned arithmetic prevent extending the sign.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Do not read multiple time an array size that can be changed.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Do not read multiple times data from guest as this could be changed
by other vcpu threads.
This causes races and security problems if these data are used for
buffer allocation or checks.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Do not read multiple time an array size that can be changed.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The overflow may lead to buffer overflow as the row size computed from
width (bitmap->x) can be bigger than the size in bytes (bitmap->stride).
This can make spice-server accept the invalid sizes.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Not security risk as just for read.
However, this could be used to attempt integer overflows in the
following lines.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Check format is valid.
Check stride is at least the size of required bytes for a row.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Prevent integer overflow when computing image sizes.
Image index computations are done using 32 bit so this can cause easily
security issues. MAX_DATA_CHUNK is larger than the virtual
card limit, so this is not going to cause change in behaviours.
Comparing size calculation results with MAX_DATA_CHUNK will allow us to
catch overflows.
Prevent guest from allocating large amount of memory.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
A driver can overwrite surface state creating a surface with the same
id of a previous one.
Also can try to destroy surfaces that are not created.
Both requests cause invalid internal states that could lead to crashes
or memory corruptions.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Do not just give warning and continue to use an invalid index into
an array.
Resolves: CVE-2015-5260
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
