> On Wed, 2019-01-09 at 03:14 -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/red-qxl.c | 1 -
> > > server/red-stream-device.c | 66 ++++++++++++++++++++++++++++++++++++--
> > > server/red-stream-device.h | 8 +++++
> > > server/reds.h | 3 ++
> > > 4 files changed, 74 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/server/red-qxl.c b/server/red-qxl.c
> > > index ebc14a46..e174758f 100644
> > > --- a/server/red-qxl.c
> > > +++ b/server/red-qxl.c
> > > @@ -41,7 +41,6 @@
> > > #include "red-qxl.h"
> > >
> > >
> > > -#define MAX_DEVICE_ADDRESS_LEN 256
> >
> > Maybe this should be defined in the protocol ?
>
> Not sure, maybe?
>
Mainly I was thinking about while reading some patches so I didn't (and I still
don't) have the overall look.
However looks like that various components (from spice-server to the 2 agents)
all rely on this 256 limit. So I think should be in the spice-protocol file
like many other limits.
> > > #define MAX_MONITORS_COUNT 16
> > >
> > > struct QXLState {
> > > diff --git a/server/red-stream-device.c b/server/red-stream-device.c
> > > index 215ddbe7..bbd6874f 100644
> > > --- a/server/red-stream-device.c
> > > +++ b/server/red-stream-device.c
> > > @@ -23,7 +23,6 @@
> > >
> > > #include "stream-channel.h"
> > > #include "cursor-channel.h"
> > > -#include "reds.h"
> > >
> > > #define MAX_GUEST_CAPABILITIES_BYTES ((STREAM_CAP_END+7)/8)
> > >
> > > @@ -37,6 +36,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 +49,7 @@ struct StreamDevice {
> > > CursorChannel *cursor_channel;
> > > SpiceTimer *close_timer;
> > > uint32_t frame_mmtime;
> > > + StreamDeviceDisplayInfo device_display_info;
> > > };
> > >
> > > struct StreamDeviceClass {
> > > @@ -64,8 +65,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 +147,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 +279,58 @@ 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;
> > > + }
> >
> > Isn't enough the static buffer?
>
> Maybe. There's "uint8_t buf[STREAM_MSG_CAPABILITIES_MAX_BYTES];" in msg
> so it probably is, but I don't wat to rely on some unrelated constants
> incidentially being right for this to work.
>
> I'm not a fan of this mechanism, I even briefly considered changing it,
> but for some reason I forgot dropped the idea...
>
Yes, this resize stuff is not that great at the end.
> > > +
> > > + /* 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 = display_info_msg->device_address_len;
> > > + if (device_address_len > MAX_DEVICE_ADDRESS_LEN) {
> > > + g_error("Received a device address longer than %u (%lu), "
> > > + "will be truncated!", MAX_DEVICE_ADDRESS_LEN,
> > > device_address_len);
> > > + 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.
>
> Could you be more specific? I don't see it.
>
Sorry, I miss the assignment after the size check.
I just noted that g_error is fatal so the agent can easily create a DoS
crashing the VM.
> > > +
> > > + dev->device_display_info.device_address[device_address_len] = '\0';
> > > //
> > > terminate the string
> >
> > buffer overflow writing if device_address_len == MAX_DEVICE_ADDRESS_LEN
>
> Right, I'll fix that.
>
> > > + dev->device_display_info.id = display_info_msg->id;
> > > + dev->device_display_info.device_display_id =
> > > display_info_msg->device_display_id;
> >
> > No endian fixes.
>
> I'll fix that too.
>
> > > +
> > > + g_debug("Received DeviceDisplayInfo from the streaming agent: id %u,
> > > "
> > > + "device_address %s, device_display_id %u",
> > > + dev->device_display_info.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..9e71cb88 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 "reds.h"
> >
> > spaghetti code, circular dependency
>
> Ok, I'll move the constant to display-limits.h as suggested below and
> remove this header here.
>
> Cheers,
> Lukas
>
As said above looking more patches seems that spice-protocol is
even better.
> > > #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 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.h b/server/reds.h
> > > index 8260ab5c..675e10d5 100644
> > > --- a/server/reds.h
> > > +++ b/server/reds.h
> > > @@ -30,6 +30,9 @@
> > > #include "main-dispatcher.h"
> > > #include "migration-protocol.h"
> > >
> > > +
> > > +#define MAX_DEVICE_ADDRESS_LEN 256
> > > +
> >
> > It would be better a header like display-limits.h.
> >
> > > static inline QXLInterface * qxl_get_interface(QXLInstance *qxl)
> > > {
> > > return SPICE_CONTAINEROF(qxl->base.sif, QXLInterface, base);
> >
> > Frediano
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel