[virglrenderer-devel] [PATCH] virgl/vtest: add support to vtest for new cap getting.

[Jakob Bornecrantz]

On 2018-06-27 23:26, Dave Airlie wrote:
> On 28 June 2018 at 03:25, Jakob Bornecrantz <jakob at collabora.com> wrote:
>> On 2018-06-08 07:22, Dave Airlie wrote:
>>>
>>> From: Dave Airlie <airlied at 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.