On 28 June 2018 at 03:25, Jakob Bornecrantz <[email protected]> wrote: > On 2018-06-08 07:22, Dave Airlie wrote: >> >> From: Dave Airlie <[email protected]> >> >> 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. > > 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. Dave. _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
