Re: [RFC PATCH-for-7.2 3/5] hw/display/qxl: Pass requested buffer size to qxl_phys2virt()
On 28/11/22 17:29, Philippe Mathieu-Daudé wrote:
On 28/11/22 17:18, Philippe Mathieu-Daudé wrote:
On 28/11/22 16:41, Philippe Mathieu-Daudé wrote:
On 28/11/22 16:08, Gerd Hoffmann wrote:
Also at least one code path (processing SPICE_CURSOR_TYPE_MONO in
qxl_cursor) goes access chunk.data[] without calling
qxl_unpack_chunks(), that needs additional verification too (or
switch it to call qxl_unpack_chunks, or just drop it because nobody
uses mono chrome cursors anyway).
Per commit 36ffc122dc ("qxl: support mono cursors with inverted colors")
"Monochrome cursors are still used by Windows guests" (i.e. Win2008R2)
:/
Hmm I guess I'm missing something in qxl_cursor() following the
SPICE_CURSOR_TYPE_MONO case.
- cursor_alloc() allocate QEMUCursor* c but doesn't set c->data,
Sorry long day, cursor_alloc() does allocate c->data:
typedef struct QEMUCursor {
int width, height;
int hot_x, hot_y;
int refcount;
uint32_tdata[];
} QEMUCursor;
QEMUCursor *cursor_alloc(int width, int height)
{
QEMUCursor *c;
size_t datasize = width * height * sizeof(uint32_t);
if (width > 512 || height > 512) {
return NULL;
}
c = g_malloc0(sizeof(QEMUCursor) + datasize);
- nothing seems to set c->data
- cursor_set_mono() is called and *(c->data) is assigned...
?
Re: [RFC PATCH-for-7.2 3/5] hw/display/qxl: Pass requested buffer size to qxl_phys2virt()
On 28/11/22 17:18, Philippe Mathieu-Daudé wrote:
On 28/11/22 16:41, Philippe Mathieu-Daudé wrote:
On 28/11/22 16:08, Gerd Hoffmann wrote:
Also at least one code path (processing SPICE_CURSOR_TYPE_MONO in
qxl_cursor) goes access chunk.data[] without calling
qxl_unpack_chunks(), that needs additional verification too (or
switch it to call qxl_unpack_chunks, or just drop it because nobody
uses mono chrome cursors anyway).
Per commit 36ffc122dc ("qxl: support mono cursors with inverted colors")
"Monochrome cursors are still used by Windows guests" (i.e. Win2008R2)
:/
Hmm I guess I'm missing something in qxl_cursor() following the
SPICE_CURSOR_TYPE_MONO case.
- cursor_alloc() allocate QEMUCursor* c but doesn't set c->data,
- nothing seems to set c->data
- cursor_set_mono() is called and *(c->data) is assigned...
?
Re: [RFC PATCH-for-7.2 3/5] hw/display/qxl: Pass requested buffer size to qxl_phys2virt()
On 28/11/22 16:41, Philippe Mathieu-Daudé wrote:
On 28/11/22 16:08, Gerd Hoffmann wrote:
Also at least one code path (processing SPICE_CURSOR_TYPE_MONO in
qxl_cursor) goes access chunk.data[] without calling
qxl_unpack_chunks(), that needs additional verification too (or
switch it to call qxl_unpack_chunks, or just drop it because nobody
uses mono chrome cursors anyway).
Per commit 36ffc122dc ("qxl: support mono cursors with inverted colors")
"Monochrome cursors are still used by Windows guests" (i.e. Win2008R2)
:/
Re: [RFC PATCH-for-7.2 3/5] hw/display/qxl: Pass requested buffer size to qxl_phys2virt()
On Mon, Nov 28, 2022 at 04:41:14PM +0100, Philippe Mathieu-Daudé wrote:
> On 28/11/22 16:08, Gerd Hoffmann wrote:
> > > @@ -228,7 +230,7 @@ static void qxl_unpack_chunks(void *dest, size_t
> > > size, PCIQXLDevice *qxl,
> > > if (offset == size) {
> > > return;
> > > }
> > > -chunk = qxl_phys2virt(qxl, chunk->next_chunk, group_id);
> > > +chunk = qxl_phys2virt(qxl, chunk->next_chunk, group_id, bytes);
> > > if (!chunk) {
> > > return;
> > > }
> >
> > Naa, its not that simple. You get a QXLDataChunk passed in which
> > typically is verified *excluding* dynamically-sized chunk->data.
>
> OK so IIUC 1/ this line should be:
>
> chunk = qxl_phys2virt(qxl, chunk->next_chunk, group_id,
> sizeof(QXLDataChunk));
Depends on whenever you do (2) inside or outside the loop ;)
> but 2/ we should check chunk->data[chunk->data_size] is valid (within
> the MR) before calling the memcpy(), right?
Yes.
take care,
Gerd
Re: [RFC PATCH-for-7.2 3/5] hw/display/qxl: Pass requested buffer size to qxl_phys2virt()
On 28/11/22 16:08, Gerd Hoffmann wrote:
@@ -228,7 +230,7 @@ static void qxl_unpack_chunks(void *dest, size_t size,
PCIQXLDevice *qxl,
if (offset == size) {
return;
}
-chunk = qxl_phys2virt(qxl, chunk->next_chunk, group_id);
+chunk = qxl_phys2virt(qxl, chunk->next_chunk, group_id, bytes);
if (!chunk) {
return;
}
Naa, its not that simple. You get a QXLDataChunk passed in which
typically is verified *excluding* dynamically-sized chunk->data.
OK so IIUC 1/ this line should be:
chunk = qxl_phys2virt(qxl, chunk->next_chunk, group_id,
sizeof(QXLDataChunk));
but 2/ we should check chunk->data[chunk->data_size] is valid (within
the MR) before calling the memcpy(), right?
Also at least one code path (processing SPICE_CURSOR_TYPE_MONO in
qxl_cursor) goes access chunk.data[] without calling
qxl_unpack_chunks(), that needs additional verification too (or
switch it to call qxl_unpack_chunks, or just drop it because nobody
uses mono chrome cursors anyway).
OK I'll look at that.
Thanks,
Phil.
Re: [RFC PATCH-for-7.2 3/5] hw/display/qxl: Pass requested buffer size to qxl_phys2virt()
> @@ -228,7 +230,7 @@ static void qxl_unpack_chunks(void *dest, size_t size,
> PCIQXLDevice *qxl,
> if (offset == size) {
> return;
> }
> -chunk = qxl_phys2virt(qxl, chunk->next_chunk, group_id);
> +chunk = qxl_phys2virt(qxl, chunk->next_chunk, group_id, bytes);
> if (!chunk) {
> return;
> }
Naa, its not that simple. You get a QXLDataChunk passed in which
typically is verified *excluding* dynamically-sized chunk->data.
Also at least one code path (processing SPICE_CURSOR_TYPE_MONO in
qxl_cursor) goes access chunk.data[] without calling
qxl_unpack_chunks(), that needs additional verification too (or
switch it to call qxl_unpack_chunks, or just drop it because nobody
uses mono chrome cursors anyway).
take care,
Gerd
Re: [RFC PATCH-for-7.2 3/5] hw/display/qxl: Pass requested buffer size to qxl_phys2virt()
Hi
On Mon, Nov 28, 2022 at 5:48 PM Philippe Mathieu-Daudé
wrote:
> Currently qxl_phys2virt() doesn't check for buffer overrun.
> In order to do so in the next commit, pass the buffer size
> as argument.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> RFC: Please double-check qxl_render_update_area_unlocked()
> ---
> hw/display/qxl-logger.c | 11 ---
> hw/display/qxl-render.c | 12
> hw/display/qxl.c| 14 +-
> hw/display/qxl.h| 4 +++-
> 4 files changed, 28 insertions(+), 13 deletions(-)
>
> diff --git a/hw/display/qxl-logger.c b/hw/display/qxl-logger.c
> index 1bcf803db6..35c38f6252 100644
> --- a/hw/display/qxl-logger.c
> +++ b/hw/display/qxl-logger.c
> @@ -106,7 +106,7 @@ static int qxl_log_image(PCIQXLDevice *qxl,
> QXLPHYSICAL addr, int group_id)
> QXLImage *image;
> QXLImageDescriptor *desc;
>
> -image = qxl_phys2virt(qxl, addr, group_id);
> +image = qxl_phys2virt(qxl, addr, group_id, sizeof(QXLImage));
> if (!image) {
> return 1;
> }
> @@ -214,7 +214,8 @@ int qxl_log_cmd_cursor(PCIQXLDevice *qxl, QXLCursorCmd
> *cmd, int group_id)
> cmd->u.set.position.y,
> cmd->u.set.visible ? "yes" : "no",
> cmd->u.set.shape);
> -cursor = qxl_phys2virt(qxl, cmd->u.set.shape, group_id);
> +cursor = qxl_phys2virt(qxl, cmd->u.set.shape, group_id,
> + sizeof(QXLCursor));
> if (!cursor) {
> return 1;
> }
> @@ -236,6 +237,7 @@ int qxl_log_command(PCIQXLDevice *qxl, const char
> *ring, QXLCommandExt *ext)
> {
> bool compat = ext->flags & QXL_COMMAND_FLAG_COMPAT;
> void *data;
> +size_t datasz;
> int ret;
>
> if (!qxl->cmdlog) {
> @@ -249,15 +251,18 @@ int qxl_log_command(PCIQXLDevice *qxl, const char
> *ring, QXLCommandExt *ext)
>
> switch (ext->cmd.type) {
> case QXL_CMD_DRAW:
> +datasz = compat ? sizeof(QXLCompatDrawable) : sizeof(QXLDrawable);
> break;
> case QXL_CMD_SURFACE:
> +datasz = sizeof(QXLSurfaceCmd);
> break;
> case QXL_CMD_CURSOR:
> +datasz = sizeof(QXLCursorCmd);
> break;
> default:
> goto out;
> }
> -data = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
> +data = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id, datasz);
> if (!data) {
> return 1;
> }
> diff --git a/hw/display/qxl-render.c b/hw/display/qxl-render.c
> index ca217004bf..0a4bfa8aa6 100644
> --- a/hw/display/qxl-render.c
> +++ b/hw/display/qxl-render.c
> @@ -107,7 +107,9 @@ static void
> qxl_render_update_area_unlocked(PCIQXLDevice *qxl)
> qxl->guest_primary.resized = 0;
> qxl->guest_primary.data = qxl_phys2virt(qxl,
>
> qxl->guest_primary.surface.mem,
> -MEMSLOT_GROUP_GUEST);
> +MEMSLOT_GROUP_GUEST,
> +
> qxl->guest_primary.abs_stride
>
+* height);
>
Right, it looks like QXL device stride is in bytes.
Reviewed-by: Marc-André Lureau
> if (!qxl->guest_primary.data) {
> goto end;
> }
> @@ -228,7 +230,7 @@ static void qxl_unpack_chunks(void *dest, size_t size,
> PCIQXLDevice *qxl,
> if (offset == size) {
> return;
> }
> -chunk = qxl_phys2virt(qxl, chunk->next_chunk, group_id);
> +chunk = qxl_phys2virt(qxl, chunk->next_chunk, group_id, bytes);
> if (!chunk) {
> return;
> }
> @@ -295,7 +297,8 @@ fail:
> /* called from spice server thread context only */
> int qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt *ext)
> {
> -QXLCursorCmd *cmd = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
> +QXLCursorCmd *cmd = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id,
> + sizeof(QXLCursorCmd));
> QXLCursor *cursor;
> QEMUCursor *c;
>
> @@ -314,7 +317,8 @@ int qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt
> *ext)
> }
> switch (cmd->type) {
> case QXL_CURSOR_SET:
> -cursor = qxl_phys2virt(qxl, cmd->u.set.shape, ext->group_id);
> +cursor = qxl_phys2virt(qxl, cmd->u.set.shape, ext->group_id,
> + sizeof(QXLCursor));
> if (!cursor) {
> return 1;
> }
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index 5b10f697f1..231d733250 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -274,7 +274,8 @@ static void
> qxl_spice_monitors_config_async(PCIQXLDevice *qxl, int replay)
>QXL_IO_MONITORS_CONFIG_ASYNC));
> }
>
> -cfg = qxl_phys2virt(qxl, qxl->guest_monitors_config,
> MEMSLOT_GROUP_GUEST);
> +cfg = qxl_phys2virt(qxl, qxl->guest_monitors_config,
> MEMSLOT_GROUP_GUEST,
> +
