Re: [Qemu-devel] [PATCH v4 5/8] nbd/server: Include human-readable message in structured errors

[Eric Blake]

On 10/14/2017 08:01 PM, Eric Blake wrote:
> The NBD spec permits including a human-readable error string if
> structured replies are in force, so we might as well send the
> client the message that we logged on any error.
> 
> Signed-off-by: Eric Blake <address@hidden>
> ---
>  nbd/server.c     | 22 ++++++++++++++++------
>  nbd/trace-events |  2 +-
>  2 files changed, 17 insertions(+), 7 deletions(-)
> 

>      assert(nbd_err);
> -    trace_nbd_co_send_structured_error(handle, nbd_err);
> +    trace_nbd_co_send_structured_error(handle, nbd_err,
> +                                       nbd_err_lookup(nbd_err), msg);
>      set_be_chunk(&chunk.h, NBD_REPLY_FLAG_DONE, NBD_REPLY_TYPE_ERROR, handle,
>                   sizeof(chunk) - sizeof(chunk.h));

Bug - it's a bad idea to not include the message length in the overall
length, because the client then gets out of sync with the server (it
reads only 6 bytes instead of 6+strlen(msg) bytes, and expects the
message to start with the magic number for the next reply).

>      stl_be_p(&chunk.error, nbd_err);
> -    stw_be_p(&chunk.message_length, 0);
> +    stw_be_p(&chunk.message_length, iov[1].iov_len);

But this also highlights a bug in 9/8, where we have:

> +static int nbd_parse_error_payload(NBDStructuredReplyChunk *chunk,
> +                                   uint8_t *payload, int *request_ret,
> +                                   Error **errp)
> +{
> +    uint32_t error;
> +    uint16_t message_size;
> +
> +    assert(chunk->type & (1 << 15));
> +
> +    if (chunk->length < sizeof(error) + sizeof(message_size)) {
> +        error_setg(errp,
> +                   "Protocol error: invalid payload for structured error");
> +        return -EINVAL;
> +    }
> +
> +    error = nbd_errno_to_system_errno(payload_advance32(&payload));
> +    if (error == 0) {
> +        error_setg(errp, "Protocol error: server sent structured error chunk"
> +                         "with error = 0");
> +        return -EINVAL;
> +    }
> +
> +    *request_ret = error;
> +    message_size = payload_advance16(&payload);
> +    error_setg_errno(errp, error, "%.*s", message_size, payload);

Whoops - no sanity check that message_size fits within chunk->length.
So when we read message_length 33 (when the server sends a message 33
bytes long), we are then dereferencing up to 33 bytes of garbage beyond
the end of payload.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature