On Fri, 13 May 2016, Christophe Fergeau wrote:
[...]
> Yeah, the only reason for these questions is to know whether a malicious
> client could trigger misbehaviour in this part of the code by sending us
> some unexpected data. I agree when all is good, this does not make
> sense/should not happen, but I'd prefer to be 100% sure it cannot happen
> regardless of what we receive from the guest.
By 'malicious client' I guess you don't mean Spicy (since the video
doesn't flow upstream). So presumably you're concerned by a buggy/rogue
QXL driver in a VM.
In the video streaming case, both stream_stride and bitmap->stride come
from RedDrawable.u.copy:
* bitmap is the SpiceCopy.src_bitmap field
* stream_stride is computed from the SpiceCopy.src_area field
(src_area.right - src_area.left).
Now if the content of SpiceCopy cannot be trusted, the GStreamer encoder
is the wrong place to check for issues : there are at least three places
where the SpiceCopy bitmap can be used: the MJPEG encoder, the GStreamer
encoder and the regular screen update code (which may or may not involve
the JPEG encoder) [1].
Duplicating checks all over the place can only lead to maintenance
issues and incomplete checks. So validating the SpiceCopy structure
should be done before it is sent down one or the other path. The best
place would probably be either before red_get_native_drawable() is
called, inside red_get_native_drawable(), or in the relevant
red_get_XXX_ptr() helper.
However I don't see any such check there or down the path:
* red-parse-qxl.c never dereferences src_area.
* dcc-send.c does not check consistency.
In fact I suggest trying variations of the following patch:
diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index 0dafbef..965b5ef 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -682,6 +682,8 @@ static int red_get_copy_ptr(RedMemSlotInfo *slots, int group_id,
return 1;
}
red_get_rect_ptr(&red->src_area, &qxl->src_area);
+ red->src_area.right += 10;
+ red->src_area.bottom += 10;
red->rop_descriptor = qxl->rop_descriptor;
red->scale_mode = qxl->scale_mode;
red_get_qmask_ptr(slots, group_id, &red->mask, &qxl->mask, flags);
Everything with go fine, modulo significant display issues in the
client[2], until the MJPEG encoder tries to handle the first frame and
the JPEG library kills the process with:
Improper call to JPEG library in state 101
[1] To be fair it looks like this last case is handled by sending the
whole bitmap (see spice_marshall_Copy() & spice_marshall_Image()),
even if only a small bit is supposed to be copied. That does not
seem very bandwidth efficient but it side-steps any src_bitmap vs.
src_area consistency issues.
[2] So the client does not validate the SpiceCopy object either. Whether
its behavior is sensible is not clear however.
--
Francois Gouget <fgouget@xxxxxxxxxxxxxxx>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel