[Spice-devel] [vdagent-win PATCH v2 2/9] Minor overflow checks improvements

[Jonathon Jongsma]

On Wed, 2018-06-27 at 14:57 +0100, Frediano Ziglio wrote:
> Although source of these data should be safe improve data checks
> to avoid some overflows.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  vdagent/vdagent.cpp | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/vdagent/vdagent.cpp b/vdagent/vdagent.cpp
> index e22687c..8294ea7 100644
> --- a/vdagent/vdagent.cpp
> +++ b/vdagent/vdagent.cpp
> @@ -1314,7 +1314,7 @@ VOID VDAgent::read_completion(DWORD err, DWORD
> bytes, LPOVERLAPPED overlapped)
>          count = sizeof(VDIChunk) - a->_read_pos;
>      } else if (a->_read_pos == sizeof(VDIChunk)) {
>          count = chunk->hdr.size;
> -        if (a->_read_pos + count > sizeof(a->_read_buf)) {
> +        if (count > sizeof(a->_read_buf) - a->_read_pos) {
>              vd_printf("chunk is too large, size %u port %u", chunk-
> >hdr.size, chunk->hdr.port);
>              a->_running = false;
>              return;
> @@ -1351,12 +1351,19 @@ void VDAgent::handle_chunk(VDIChunk* chunk)
>          uint32_t msg_size = sizeof(VDAgentMessage) + msg->size;
>          if (chunk->hdr.size == msg_size) {
>              dispatch_message(msg, chunk->hdr.port);
> -        } else {
> -            ASSERT(chunk->hdr.size < msg_size);
> +        } else if (chunk->hdr.size < msg_size) {
>              _in_msg = (VDAgentMessage*)new uint8_t[msg_size];
>              memcpy(_in_msg, chunk->data, chunk->hdr.size);
>              _in_msg_pos = chunk->hdr.size;
> +        } else {
> +            vd_printf("Invalid VDAgentMessage message");
> +            _running = false;
> +            return;
>          }


This chunk of the patch isn't really about preventing an overflow. It's
changing an assert to a warning message. So I kind of want to say that 
it should be a separate commit. But it is pretty closely related to the
below change, which *does* avoid an overflow...


> +    } else if (chunk->hdr.size > sizeof(VDAgentMessage) + _in_msg-
> >size - _in_msg_pos) {
> +        vd_printf("Invalid VDAgentMessage message");
> +        _running = false;
> +        return;
>      } else {
>          memcpy((uint8_t*)_in_msg + _in_msg_pos, chunk->data, chunk-
> >hdr.size);
>          _in_msg_pos += chunk->hdr.size;


It's a little bit hard to keep track of what is potentially
uninitialized among these member variables. I thought that _in_msg.size
could potentially be uninitialized here, but it looks like we're
protected by the following line above:

        if (chunk->hdr.size < sizeof(VDAgentMessage)) {
            return;
        }

So the code looks correct, albeit a bit hard to read.


Acked-by: Jonathon Jongsma <jjongsma at redhat.com>