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 <ebl...@redhat.com> > --- > 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