On Tue, 2019-01-15 at 08:31 -0500, Frediano Ziglio wrote:
> >
> > Receives the GraphicsDeviceInfo message from the streaming agent and
> > stores the data in a list on the streaming device.
> >
> > Signed-off-by: Lukáš Hrázký <lhrazky@xxxxxxxxxx>
> > ---
> > server/display-limits.h | 3 ++
> > server/red-qxl.c | 2 +-
> > server/red-stream-device.c | 67 ++++++++++++++++++++++++++++++++++++--
> > server/red-stream-device.h | 8 +++++
> > server/reds.c | 1 +
> > 5 files changed, 78 insertions(+), 3 deletions(-)
> >
> > diff --git a/server/display-limits.h b/server/display-limits.h
> > index e875149b..d79d3211 100644
> > --- a/server/display-limits.h
> > +++ b/server/display-limits.h
> > @@ -25,4 +25,7 @@
> > /** Maximum number of streams created by spice-server */
> > #define NUM_STREAMS 50
> >
> > +/** Maximum length of the device address string */
> > +#define MAX_DEVICE_ADDRESS_LEN 256
> > +
> > #endif /* DISPLAY_LIMITS_H_ */
> > diff --git a/server/red-qxl.c b/server/red-qxl.c
> > index a56d9a52..943ccb08 100644
> > --- a/server/red-qxl.c
> > +++ b/server/red-qxl.c
> > @@ -37,11 +37,11 @@
> > #include "dispatcher.h"
> > #include "red-parse-qxl.h"
> > #include "red-channel-client.h"
> > +#include "display-limits.h"
> >
> > #include "red-qxl.h"
> >
> >
> > -#define MAX_DEVICE_ADDRESS_LEN 256
> > #define MAX_MONITORS_COUNT 16
> >
> > struct QXLState {
> > diff --git a/server/red-stream-device.c b/server/red-stream-device.c
> > index 215ddbe7..20bf115d 100644
> > --- a/server/red-stream-device.c
> > +++ b/server/red-stream-device.c
> > @@ -37,6 +37,7 @@ struct StreamDevice {
> > StreamMsgCapabilities capabilities;
> > StreamMsgCursorSet cursor_set;
> > StreamMsgCursorMove cursor_move;
> > + StreamMsgDeviceDisplayInfo device_display_info;
> > uint8_t buf[STREAM_MSG_CAPABILITIES_MAX_BYTES];
> > } *msg;
> > uint32_t msg_pos;
> > @@ -49,6 +50,7 @@ struct StreamDevice {
> > CursorChannel *cursor_channel;
> > SpiceTimer *close_timer;
> > uint32_t frame_mmtime;
> > + StreamDeviceDisplayInfo device_display_info;
> > };
> >
> > struct StreamDeviceClass {
> > @@ -64,8 +66,8 @@ static void char_device_set_state(RedCharDevice *char_dev,
> > int state);
> > typedef bool StreamMsgHandler(StreamDevice *dev, SpiceCharDeviceInstance
> > *sin)
> > SPICE_GNUC_WARN_UNUSED_RESULT;
> >
> > -static StreamMsgHandler handle_msg_format, handle_msg_data,
> > handle_msg_cursor_set,
> > - handle_msg_cursor_move, handle_msg_capabilities;
> > +static StreamMsgHandler handle_msg_format, handle_msg_device_display_info,
> > handle_msg_data,
> > + handle_msg_cursor_set, handle_msg_cursor_move, handle_msg_capabilities;
> >
> > static bool handle_msg_invalid(StreamDevice *dev, SpiceCharDeviceInstance
> > *sin,
> > const char *error_msg)
> > SPICE_GNUC_WARN_UNUSED_RESULT;
> > @@ -146,6 +148,13 @@ stream_device_partial_read(StreamDevice *dev,
> > SpiceCharDeviceInstance *sin)
> > handled = handle_msg_format(dev, sin);
> > }
> > break;
> > + case STREAM_TYPE_DEVICE_DISPLAY_INFO:
> > + if (dev->hdr.size > sizeof(StreamMsgDeviceDisplayInfo) +
> > MAX_DEVICE_ADDRESS_LEN) {
> > + handled = handle_msg_invalid(dev, sin,
> > "StreamMsgDeviceDisplayInfo too large");
> > + } else {
> > + handled = handle_msg_device_display_info(dev, sin);
> > + }
> > + break;
> > case STREAM_TYPE_DATA:
> > if (dev->hdr.size > 32*1024*1024) {
> > handled = handle_msg_invalid(dev, sin, "STREAM_DATA too large");
> > @@ -271,6 +280,60 @@ handle_msg_format(StreamDevice *dev,
> > SpiceCharDeviceInstance *sin)
> > return true;
> > }
> >
> > +static bool
> > +handle_msg_device_display_info(StreamDevice *dev, SpiceCharDeviceInstance
> > *sin)
> > +{
> > + SpiceCharDeviceInterface *sif = spice_char_device_get_interface(sin);
> > +
> > + if (spice_extra_checks) {
> > + spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
> > + spice_assert(dev->hdr.type == STREAM_TYPE_DEVICE_DISPLAY_INFO);
> > + }
> > +
> > + if (dev->msg_len < dev->hdr.size) {
> > + dev->msg = g_realloc(dev->msg, dev->hdr.size);
> > + dev->msg_len = dev->hdr.size;
> > + }
> > +
> > + /* read from device */
> > + ssize_t n = sif->read(sin, dev->msg->buf + dev->msg_pos, dev->hdr.size -
> > dev->msg_pos);
> > + if (n <= 0) {
> > + return dev->msg_pos == dev->hdr.size;
> > + }
> > +
> > + dev->msg_pos += n;
> > + if (dev->msg_pos != dev->hdr.size) { /* some bytes are still missing */
> > + return false;
> > + }
> > +
> > + StreamMsgDeviceDisplayInfo *display_info_msg =
> > &dev->msg->device_display_info;
> > +
> > + size_t device_address_len =
> > GUINT32_FROM_LE(display_info_msg->device_address_len);
> > + if (device_address_len > MAX_DEVICE_ADDRESS_LEN) {
> > + g_error("Received a device address longer than %u (%zu), "
> > + "will be truncated!", MAX_DEVICE_ADDRESS_LEN,
> > device_address_len);
>
> DoS, g_error will abort() Qemu.
I'll put a warning there, though I think it should be logged as an
error...
> > + device_address_len =
> > sizeof(dev->device_display_info.device_address);
> > + }
> > +
> > + strncpy(dev->device_display_info.device_address,
> > + (char*) display_info_msg->device_address,
> > + device_address_len);
>
> buffer overflow reading, is not guaranteed that the address
> fit in the message, you need to check it.
Right, I'll fix it.
> > +
> > + // make sure the string is terminated
> > + dev->device_display_info.device_address[device_address_len - 1] = '\0';
> > +
>
> buffer overflow writing if devide_address_len is 0, also device_address won't
> be terminated causing a future buffer overflow reading.
Same here. Similar problem in vd_agent, I'll fix that as well.
> > + dev->device_display_info.stream_id =
> > GUINT32_FROM_LE(display_info_msg->stream_id);
> > + dev->device_display_info.device_display_id =
> > GUINT32_FROM_LE(display_info_msg->device_display_id);
> > +
> > + g_debug("Received DeviceDisplayInfo from the streaming agent: stream_id
> > %u, "
> > + "device_address %s, device_display_id %u",
> > + dev->device_display_info.stream_id,
> > + dev->device_display_info.device_address,
> > + dev->device_display_info.device_display_id);
> > +
> > + return true;
> > +}
> > +
> > static bool
> > handle_msg_capabilities(StreamDevice *dev, SpiceCharDeviceInstance *sin)
> > {
> > diff --git a/server/red-stream-device.h b/server/red-stream-device.h
> > index f0a5b5c1..996be016 100644
> > --- a/server/red-stream-device.h
> > +++ b/server/red-stream-device.h
> > @@ -19,6 +19,7 @@
> > #ifndef STREAM_DEVICE_H
> > #define STREAM_DEVICE_H
> >
> > +#include "display-limits.h"
> > #include "char-device.h"
> >
> > G_BEGIN_DECLS
> > @@ -41,6 +42,13 @@ typedef struct StreamDeviceClass StreamDeviceClass;
> > (G_TYPE_INSTANCE_GET_CLASS((obj), TYPE_STREAM_DEVICE,
> > StreamDeviceClass))
> >
> > GType stream_device_get_type(void) G_GNUC_CONST;
> > +
> > +typedef struct StreamDeviceDisplayInfo {
> > + uint32_t stream_id;
> > + char device_address[MAX_DEVICE_ADDRESS_LEN];
> > + uint32_t device_display_id;
> > +} StreamDeviceDisplayInfo;
> > +
> > StreamDevice *stream_device_connect(RedsState *reds, SpiceCharDeviceInstance
> > *sin);
> >
> > /* Create channel for the streaming device.
> > diff --git a/server/reds.c b/server/reds.c
> > index 88a82419..48796ba6 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -74,6 +74,7 @@
> > #include "glib-compat.h"
> > #include "net-utils.h"
> > #include "red-stream-device.h"
> > +#include "display-limits.h"
>
> This should not be necessary.
True, removing it.
Thanks,
Lukas
> >
> > #define REDS_MAX_STAT_NODES 100
> >
>
> Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel