On 2018-06-27 23:26, Dave Airlie wrote:
On 28 June 2018 at 03:25, Jakob Bornecrantz <ja...@collabora.com> wrote:
On 2018-06-08 07:22, Dave Airlie wrote:
From: Dave Airlie <airl...@redhat.com>
The vtest protocol is pretty simple but also pretty dumb, and
the v1 caps query was fixed size, with no nice way to expand it,
however the server also ignores any command it doesn't understand.
So we can query v2 caps by sending a v2 followed by a v1, if the
v2 is ignored we know it's an old vtest server, and the we get
a v2 answer then we can just read the v1 answer and discard it.
---
.../winsys/virgl/vtest/virgl_vtest_socket.c | 30
+++++++++++++++++++---
src/gallium/winsys/virgl/vtest/vtest_protocol.h | 2 ++
2 files changed, 28 insertions(+), 4 deletions(-)
diff --git a/src/gallium/winsys/virgl/vtest/virgl_vtest_socket.c
b/src/gallium/winsys/virgl/vtest/virgl_vtest_socket.c
index adec26b66b8..d25f9a3bd9e 100644
--- a/src/gallium/winsys/virgl/vtest/virgl_vtest_socket.c
+++ b/src/gallium/winsys/virgl/vtest/virgl_vtest_socket.c
@@ -129,12 +129,14 @@ int virgl_vtest_connect(struct virgl_vtest_winsys
*vws)
int virgl_vtest_send_get_caps(struct virgl_vtest_winsys *vws,
struct virgl_drm_caps *caps)
{
- uint32_t get_caps_buf[VTEST_HDR_SIZE];
+ uint32_t get_caps_buf[VTEST_HDR_SIZE * 2];
uint32_t resp_buf[VTEST_HDR_SIZE];
-
+ uint32_t caps_size = sizeof(struct virgl_caps_v2);
int ret;
get_caps_buf[VTEST_CMD_LEN] = 0;
- get_caps_buf[VTEST_CMD_ID] = VCMD_GET_CAPS;
+ get_caps_buf[VTEST_CMD_ID] = VCMD_GET_CAPS2;
+ get_caps_buf[VTEST_CMD_LEN + 2] = 0;
+ get_caps_buf[VTEST_CMD_ID + 2] = VCMD_GET_CAPS;
virgl_block_write(vws->sock_fd, &get_caps_buf,
sizeof(get_caps_buf));
@@ -142,7 +144,27 @@ int virgl_vtest_send_get_caps(struct
virgl_vtest_winsys *vws,
if (ret <= 0)
return 0;
- ret = virgl_block_read(vws->sock_fd, &caps->caps, sizeof(struct
virgl_caps_v1));
+ if (resp_buf[1] == 2) {
+ struct virgl_caps_v1 dummy;
+ uint32_t resp_size = resp_buf[0] - 1;
+ uint32_t dummy_size = 0;
+ if (resp_size > caps_size) {
+ dummy_size = resp_size - caps_size;
+ resp_size = caps_size;
+ }
+
+ ret = virgl_block_read(vws->sock_fd, &caps->caps, resp_size);
+
+ if (dummy_size)
+ ret = virgl_block_read(vws->sock_fd, &dummy, dummy_size);
Isn't there a risk that the "dummy_size" is larger then the struct
virgl_caps_v1 dummy and cause it to write over the stack? (I am assuming you
are using the dummy here as a place to put the extra caps the host is
exposing but the driver isn't supporting).
We've pretty much fixed caps_v1 size for ever, the protocol won't return
anything other than the 308 byte struct we have now.
I may be wrong here, but isn't the "if (dummy_size)" read dealing with
the case when the "struct virgl_caps_v2" has grown on the host but not
the guest size? And has nothing to do with caps_v1 other then that is
what dummy happens to be.
So in the case the host has extended the v2 caps struct with more then
308 bytes and the driver hasn't been updated. Wont that cause the
dummy_size to be larger then sizeof(struct virgl_caps_v1) and cause it
to smash the stack? I mean it is doubtful to ever happen but it seems a
bit of a repeat of what happened with the v1 to v2 switch.
Wouldn't it be better if we had a virgl_block_skip function?
We don't really know what a block is, it's just a unix socket, if we find
ourselves doing this more then maybe a dummy refactor might be neeeded
but hopefully this is a once off bad protocol design fix. We may want a new
protocol here anyways that is more robust anyways.
Okay sounds good.
Cheers, Jakob.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
