[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v4 5/8] nbd/server: Include human-r
From: |
Eric Blake |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v4 5/8] nbd/server: Include human-readable message in structured errors |
Date: |
Thu, 19 Oct 2017 16:39:26 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 |
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
- Re: [Qemu-block] [PATCH v4 3/8] nbd: Expose constants and structs for structured read, (continued)
- [Qemu-block] [PATCH v4 5/8] nbd/server: Include human-readable message in structured errors, Eric Blake, 2017/10/14
- [Qemu-block] [PATCH v4 6/8] nbd/client: refactor nbd_receive_starttls, Eric Blake, 2017/10/14
- [Qemu-block] [PATCH v4 8/8] nbd: Move nbd_read() to common header, Eric Blake, 2017/10/14
- [Qemu-block] [PATCH v4 7/8] nbd/client: prepare nbd_receive_reply for structured reply, Eric Blake, 2017/10/14
- [Qemu-block] [PATCH v4 RFC 9/8] nbd: Minimal structured read for client, Vladimir Sementsov-Ogievskiy, 2017/10/17