Will look into these ASAP.
On Mon, Nov 14, 2016 at 7:05 AM, Stefan Hajnoczi <stefa...@gmail.com> wrote:
> On Wed, Sep 28, 2016 at 10:45 PM, Stefan Hajnoczi <stefa...@gmail.com> wrote:
>> On Tue, Sep 27, 2016 at 09:09:49PM -0700, Ashish Mittal wrote:
>>
>> Review of .bdrv_open() and .bdrv_aio_writev() code paths.
>>
>> The big issues I see in this driver and libqnio:
>>
>> 1. Showstoppers like broken .bdrv_open() and leaking memory on every
>> reply message.
>> 2. Insecure due to missing input validation (network packets and
>> configuration) and incorrect string handling.
>> 3. Not fully asynchronous so QEMU and the guest may hang.
>>
>> Please think about the whole codebase and not just the lines I've
>> pointed out in this review when fixing these sorts of issues. There may
>> be similar instances of these bugs elsewhere and it's important that
>> they are fixed so that this can be merged.
>
> Ping?
>
> You didn't respond to the comments I raised. The libqnio buffer
> overflows and other issues from this email are still present.
>
> I put a lot of time into reviewing this patch series and libqnio. If
> you want to get reviews please address feedback before sending a new
> patch series.
>
>>
>>> +/*
>>> + * Structure per vDisk maintained for state
>>> + */
>>> +typedef struct BDRVVXHSState {
>>> + int fds[2];
>>> + int64_t vdisk_size;
>>> + int64_t vdisk_blocks;
>>> + int64_t vdisk_flags;
>>> + int vdisk_aio_count;
>>> + int event_reader_pos;
>>> + VXHSAIOCB *qnio_event_acb;
>>> + void *qnio_ctx;
>>> + QemuSpin vdisk_lock; /* Lock to protect BDRVVXHSState */
>>> + QemuSpin vdisk_acb_lock; /* Protects ACB */
>>
>> These comments are insufficient for documenting locking. Not all fields
>> are actually protected by these locks. Please order fields according to
>> lock coverage:
>>
>> typedef struct VXHSAIOCB {
>> ...
>>
>> /* Protected by BDRVVXHSState->vdisk_acb_lock */
>> int segments;
>> ...
>> };
>>
>> typedef struct BDRVVXHSState {
>> ...
>>
>> /* Protected by vdisk_lock */
>> QemuSpin vdisk_lock;
>> int vdisk_aio_count;
>> QSIMPLEQ_HEAD(aio_retryq, VXHSAIOCB) vdisk_aio_retryq;
>> ...
>> }
>>
>>> +static void vxhs_qnio_iio_close(BDRVVXHSState *s, int idx)
>>> +{
>>> + /*
>>> + * Close vDisk device
>>> + */
>>> + if (s->vdisk_hostinfo[idx].vdisk_rfd >= 0) {
>>> + iio_devclose(s->qnio_ctx, 0, s->vdisk_hostinfo[idx].vdisk_rfd);
>>
>> libqnio comment:
>> Why does iio_devclose() take an unused cfd argument? Perhaps it can be
>> dropped.
>>
>>> + s->vdisk_hostinfo[idx].vdisk_rfd = -1;
>>> + }
>>> +
>>> + /*
>>> + * Close QNIO channel against cached channel-fd
>>> + */
>>> + if (s->vdisk_hostinfo[idx].qnio_cfd >= 0) {
>>> + iio_close(s->qnio_ctx, s->vdisk_hostinfo[idx].qnio_cfd);
>>
>> libqnio comment:
>> Why does iio_devclose() take an int32_t cfd argument but iio_close()
>> takes a uint32_t cfd argument?
>>
>>> + s->vdisk_hostinfo[idx].qnio_cfd = -1;
>>> + }
>>> +}
>>> +
>>> +static int vxhs_qnio_iio_open(int *cfd, const char *of_vsa_addr,
>>> + int *rfd, const char *file_name)
>>> +{
>>> + /*
>>> + * Open qnio channel to storage agent if not opened before.
>>> + */
>>> + if (*cfd < 0) {
>>> + *cfd = iio_open(global_qnio_ctx, of_vsa_addr, 0);
>>
>> libqnio comments:
>>
>> 1.
>> There is a buffer overflow in qnio_create_channel(). strncpy() is used
>> incorrectly so long hostname or port (both can be 99 characters long)
>> will overflow channel->name[] (64 characters) or channel->port[] (8
>> characters).
>>
>> strncpy(channel->name, hostname, strlen(hostname) + 1);
>> strncpy(channel->port, port, strlen(port) + 1);
>>
>> The third argument must be the size of the *destination* buffer, not the
>> source buffer. Also note that strncpy() doesn't NUL-terminate the
>> destination string so you must do that manually to ensure there is a NUL
>> byte at the end of the buffer.
>>
>> 2.
>> channel is leaked in the "Failed to open single connection" error case
>> in qnio_create_channel().
>>
>> 3.
>> If host is longer the 63 characters then the ioapi_ctx->channels and
>> qnio_ctx->channels maps will use different keys due to string truncation
>> in qnio_create_channel(). This means "Channel already exists" in
>> qnio_create_channel() and possibly other things will not work as
>> expected.
>>
>>> + if (*cfd < 0) {
>>> + trace_vxhs_qnio_iio_open(of_vsa_addr);
>>> + return -ENODEV;
>>> + }
>>> + }
>>> +
>>> + /*
>>> + * Open vdisk device
>>> + */
>>> + *rfd = iio_devopen(global_qnio_ctx, *cfd, file_name, 0);
>>
>> libqnio comment:
>> Buffer overflow in iio_devopen() since chandev[128] is not large enough
>> to hold channel[100] + " " + devpath[arbitrary length] chars:
>>
>> sprintf(chandev, "%s %s", channel, devpath);
>>
>>> +
>>> + if (*rfd < 0) {
>>> + if (*cfd >= 0) {
>>
>> This check is always true. Otherwise the return -ENODEV would have been
>> taken above. The if statement isn't necessary.
>>
>>> +static void vxhs_check_failover_status(int res, void *ctx)
>>> +{
>>> + BDRVVXHSState *s = ctx;
>>> +
>>> + if (res == 0) {
>>> + /* found failover target */
>>> + s->vdisk_cur_host_idx = s->vdisk_ask_failover_idx;
>>> + s->vdisk_ask_failover_idx = 0;
>>> + trace_vxhs_check_failover_status(
>>> + s->vdisk_hostinfo[s->vdisk_cur_host_idx].hostip,
>>> + s->vdisk_guid);
>>> + qemu_spin_lock(&s->vdisk_lock);
>>> + OF_VDISK_RESET_IOFAILOVER_IN_PROGRESS(s);
>>> + qemu_spin_unlock(&s->vdisk_lock);
>>> + vxhs_handle_queued_ios(s);
>>> + } else {
>>> + /* keep looking */
>>> + trace_vxhs_check_failover_status_retry(s->vdisk_guid);
>>> + s->vdisk_ask_failover_idx++;
>>> + if (s->vdisk_ask_failover_idx == s->vdisk_nhosts) {
>>> + /* pause and cycle through list again */
>>> + sleep(QNIO_CONNECT_RETRY_SECS);
>>
>> This code is called from a QEMU thread via vxhs_aio_rw(). It is not
>> permitted to call sleep() since it will freeze QEMU and probably the
>> guest.
>>
>> If you need a timer you can use QEMU's timer APIs. See aio_timer_new(),
>> timer_new_ns(), timer_mod(), timer_del(), timer_free().
>>
>>> + s->vdisk_ask_failover_idx = 0;
>>> + }
>>> + res = vxhs_switch_storage_agent(s);
>>> + }
>>> +}
>>> +
>>> +static int vxhs_failover_io(BDRVVXHSState *s)
>>> +{
>>> + int res = 0;
>>> +
>>> + trace_vxhs_failover_io(s->vdisk_guid);
>>> +
>>> + s->vdisk_ask_failover_idx = 0;
>>> + res = vxhs_switch_storage_agent(s);
>>> +
>>> + return res;
>>> +}
>>> +
>>> +static void vxhs_iio_callback(int32_t rfd, uint32_t reason, void *ctx,
>>> + uint32_t error, uint32_t opcode)
>>
>> This function is doing too much. Especially the failover code should
>> run in the AioContext since it's complex. Don't do failover here
>> because this function is outside the AioContext lock. Do it from
>> AioContext using a QEMUBH like block/rbd.c.
>>
>>> +static int32_t
>>> +vxhs_qnio_iio_writev(void *qnio_ctx, uint32_t rfd, QEMUIOVector *qiov,
>>> + uint64_t offset, void *ctx, uint32_t flags)
>>> +{
>>> + struct iovec cur;
>>> + uint64_t cur_offset = 0;
>>> + uint64_t cur_write_len = 0;
>>> + int segcount = 0;
>>> + int ret = 0;
>>> + int i, nsio = 0;
>>> + int iovcnt = qiov->niov;
>>> + struct iovec *iov = qiov->iov;
>>> +
>>> + errno = 0;
>>> + cur.iov_base = 0;
>>> + cur.iov_len = 0;
>>> +
>>> + ret = iio_writev(qnio_ctx, rfd, iov, iovcnt, offset, ctx, flags);
>>
>> libqnio comments:
>>
>> 1.
>> There are blocking connect(2) and getaddrinfo(3) calls in iio_writev()
>> so this may hang for arbitrary amounts of time. This is not permitted
>> in .bdrv_aio_readv()/.bdrv_aio_writev(). Please make qnio actually
>> asynchronous.
>>
>> 2.
>> Where does client_callback() free reply? It looks like every reply
>> message causes a memory leak!
>>
>> 3.
>> Buffer overflow in iio_writev() since device[128] cannot fit the device
>> string generated from the vdisk_guid.
>>
>> 4.
>> Buffer overflow in iio_writev() due to
>> strncpy(msg->hinfo.target,device,strlen(device)) where device[128] is
>> larger than target[64]. Also note the previous comments about strncpy()
>> usage.
>>
>> 5.
>> I don't see any endianness handling or portable alignment of struct
>> fields in the network protocol code. Binary network protocols need to
>> take care of these issue for portability. This means libqnio compiled
>> for different architectures will not work. Do you plan to support any
>> other architectures besides x86?
>>
>> 6.
>> The networking code doesn't look robust: kvset uses assert() on input
>> from the network so the other side of the connection could cause SIGABRT
>> (coredump), the client uses the msg pointer as the cookie for the
>> response packet so the server can easily crash the client by sending a
>> bogus cookie value, etc. Even on the client side these things are
>> troublesome but on a server they are guaranteed security issues. I
>> didn't look into it deeply. Please audit the code.
>>
>>> +static int vxhs_qemu_init(QDict *options, BDRVVXHSState *s,
>>> + int *cfd, int *rfd, Error **errp)
>>> +{
>>> + QDict *backing_options = NULL;
>>> + QemuOpts *opts, *tcp_opts;
>>> + const char *vxhs_filename;
>>> + char *of_vsa_addr = NULL;
>>> + Error *local_err = NULL;
>>> + const char *vdisk_id_opt;
>>> + char *file_name = NULL;
>>> + size_t num_servers = 0;
>>> + char *str = NULL;
>>> + int ret = 0;
>>> + int i;
>>> +
>>> + opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
>>> + qemu_opts_absorb_qdict(opts, options, &local_err);
>>> + if (local_err) {
>>> + error_propagate(errp, local_err);
>>> + ret = -EINVAL;
>>> + goto out;
>>> + }
>>> +
>>> + vxhs_filename = qemu_opt_get(opts, VXHS_OPT_FILENAME);
>>> + if (vxhs_filename) {
>>> + trace_vxhs_qemu_init_filename(vxhs_filename);
>>> + }
>>> +
>>> + vdisk_id_opt = qemu_opt_get(opts, VXHS_OPT_VDISK_ID);
>>> + if (!vdisk_id_opt) {
>>> + error_setg(&local_err, QERR_MISSING_PARAMETER, VXHS_OPT_VDISK_ID);
>>> + ret = -EINVAL;
>>> + goto out;
>>> + }
>>> + s->vdisk_guid = g_strdup(vdisk_id_opt);
>>> + trace_vxhs_qemu_init_vdisk(vdisk_id_opt);
>>> +
>>> + num_servers = qdict_array_entries(options, VXHS_OPT_SERVER);
>>> + if (num_servers < 1) {
>>> + error_setg(&local_err, QERR_MISSING_PARAMETER, "server");
>>> + ret = -EINVAL;
>>> + goto out;
>>> + } else if (num_servers > VXHS_MAX_HOSTS) {
>>> + error_setg(&local_err, QERR_INVALID_PARAMETER, "server");
>>> + error_append_hint(errp, "Maximum %d servers allowed.\n",
>>> + VXHS_MAX_HOSTS);
>>> + ret = -EINVAL;
>>> + goto out;
>>> + }
>>> + trace_vxhs_qemu_init_numservers(num_servers);
>>> +
>>> + for (i = 0; i < num_servers; i++) {
>>> + str = g_strdup_printf(VXHS_OPT_SERVER"%d.", i);
>>> + qdict_extract_subqdict(options, &backing_options, str);
>>> +
>>> + /* Create opts info from runtime_tcp_opts list */
>>> + tcp_opts = qemu_opts_create(&runtime_tcp_opts, NULL, 0,
>>> &error_abort);
>>> + qemu_opts_absorb_qdict(tcp_opts, backing_options, &local_err);
>>> + if (local_err) {
>>> + qdict_del(backing_options, str);
>>
>> backing_options is leaked and there's no need to delete the str key.
>>
>>> + qemu_opts_del(tcp_opts);
>>> + g_free(str);
>>> + ret = -EINVAL;
>>> + goto out;
>>> + }
>>> +
>>> + s->vdisk_hostinfo[i].hostip = g_strdup(qemu_opt_get(tcp_opts,
>>> +
>>> VXHS_OPT_HOST));
>>> + s->vdisk_hostinfo[i].port = g_ascii_strtoll(qemu_opt_get(tcp_opts,
>>> +
>>> VXHS_OPT_PORT),
>>> + NULL, 0);
>>
>> This will segfault if the port option was missing.
>>
>>> +
>>> + s->vdisk_hostinfo[i].qnio_cfd = -1;
>>> + s->vdisk_hostinfo[i].vdisk_rfd = -1;
>>> + trace_vxhs_qemu_init(s->vdisk_hostinfo[i].hostip,
>>> + s->vdisk_hostinfo[i].port);
>>
>> It's not safe to use the %s format specifier for a trace event with a
>> NULL value. In the case where hostip is NULL this could crash on some
>> systems.
>>
>>> +
>>> + qdict_del(backing_options, str);
>>> + qemu_opts_del(tcp_opts);
>>> + g_free(str);
>>> + }
>>
>> backing_options is leaked.
>>
>>> +
>>> + s->vdisk_nhosts = i;
>>> + s->vdisk_cur_host_idx = 0;
>>> + file_name = g_strdup_printf("%s%s", vdisk_prefix, s->vdisk_guid);
>>> + of_vsa_addr = g_strdup_printf("of://%s:%d",
>>> +
>>> s->vdisk_hostinfo[s->vdisk_cur_host_idx].hostip,
>>> +
>>> s->vdisk_hostinfo[s->vdisk_cur_host_idx].port);
>>
>> Can we get here with num_servers == 0? In that case this would access
>> uninitialized memory. I guess num_servers == 0 does not make sense and
>> there should be an error case for it.
>>
>>> +
>>> + /*
>>> + * .bdrv_open() and .bdrv_create() run under the QEMU global mutex.
>>> + */
>>> + if (global_qnio_ctx == NULL) {
>>> + global_qnio_ctx = vxhs_setup_qnio();
>>
>> libqnio comment:
>> The client epoll thread should mask all signals (like
>> qemu_thread_create()). Otherwise it may receive signals that it cannot
>> deal with.
>>
>>> + if (global_qnio_ctx == NULL) {
>>> + error_setg(&local_err, "Failed vxhs_setup_qnio");
>>> + ret = -EINVAL;
>>> + goto out;
>>> + }
>>> + }
>>> +
>>> + ret = vxhs_qnio_iio_open(cfd, of_vsa_addr, rfd, file_name);
>>> + if (!ret) {
>>> + error_setg(&local_err, "Failed qnio_iio_open");
>>> + ret = -EIO;
>>> + }
>>
>> The return value of vxhs_qnio_iio_open() is 0 for success or -errno for
>> error.
>>
>> I guess you never ran this code! The block driver won't even open
>> successfully.
>>
>>> +
>>> +out:
>>> + g_free(file_name);
>>> + g_free(of_vsa_addr);
>>> + qemu_opts_del(opts);
>>> +
>>> + if (ret < 0) {
>>> + for (i = 0; i < num_servers; i++) {
>>> + g_free(s->vdisk_hostinfo[i].hostip);
>>> + }
>>> + g_free(s->vdisk_guid);
>>> + s->vdisk_guid = NULL;
>>> + errno = -ret;
>>
>> There is no need to set errno here. The return value already contains
>> the error and the caller doesn't look at errno.
>>
>>> + }
>>> + error_propagate(errp, local_err);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int vxhs_open(BlockDriverState *bs, QDict *options,
>>> + int bdrv_flags, Error **errp)
>>> +{
>>> + BDRVVXHSState *s = bs->opaque;
>>> + AioContext *aio_context;
>>> + int qemu_qnio_cfd = -1;
>>> + int device_opened = 0;
>>> + int qemu_rfd = -1;
>>> + int ret = 0;
>>> + int i;
>>> +
>>> + ret = vxhs_qemu_init(options, s, &qemu_qnio_cfd, &qemu_rfd, errp);
>>> + if (ret < 0) {
>>> + trace_vxhs_open_fail(ret);
>>> + return ret;
>>> + }
>>> +
>>> + device_opened = 1;
>>> + s->qnio_ctx = global_qnio_ctx;
>>> + s->vdisk_hostinfo[0].qnio_cfd = qemu_qnio_cfd;
>>> + s->vdisk_hostinfo[0].vdisk_rfd = qemu_rfd;
>>> + s->vdisk_size = 0;
>>> + QSIMPLEQ_INIT(&s->vdisk_aio_retryq);
>>> +
>>> + /*
>>> + * Create a pipe for communicating between two threads in different
>>> + * context. Set handler for read event, which gets triggered when
>>> + * IO completion is done by non-QEMU context.
>>> + */
>>> + ret = qemu_pipe(s->fds);
>>> + if (ret < 0) {
>>> + trace_vxhs_open_epipe('.');
>>> + ret = -errno;
>>> + goto errout;
>>
>> This leaks s->vdisk_guid, s->vdisk_hostinfo[i].hostip, etc.
>> bdrv_close() will not be called so this function must do cleanup itself.
>>
>>> + }
>>> + fcntl(s->fds[VDISK_FD_READ], F_SETFL, O_NONBLOCK);
>>> +
>>> + aio_context = bdrv_get_aio_context(bs);
>>> + aio_set_fd_handler(aio_context, s->fds[VDISK_FD_READ],
>>> + false, vxhs_aio_event_reader, NULL, s);
>>> +
>>> + /*
>>> + * Initialize the spin-locks.
>>> + */
>>> + qemu_spin_init(&s->vdisk_lock);
>>> + qemu_spin_init(&s->vdisk_acb_lock);
>>> +
>>> + return 0;
>>> +
>>> +errout:
>>> + /*
>>> + * Close remote vDisk device if it was opened earlier
>>> + */
>>> + if (device_opened) {
>>
>> This is always true. The device_opened variable can be removed.
>>
>>> +/*
>>> + * This allocates QEMU-VXHS callback for each IO
>>> + * and is passed to QNIO. When QNIO completes the work,
>>> + * it will be passed back through the callback.
>>> + */
>>> +static BlockAIOCB *vxhs_aio_rw(BlockDriverState *bs,
>>> + int64_t sector_num, QEMUIOVector *qiov,
>>> + int nb_sectors,
>>> + BlockCompletionFunc *cb,
>>> + void *opaque, int iodir)
>>> +{
>>> + VXHSAIOCB *acb = NULL;
>>> + BDRVVXHSState *s = bs->opaque;
>>> + size_t size;
>>> + uint64_t offset;
>>> + int iio_flags = 0;
>>> + int ret = 0;
>>> + void *qnio_ctx = s->qnio_ctx;
>>> + uint32_t rfd = s->vdisk_hostinfo[s->vdisk_cur_host_idx].vdisk_rfd;
>>> +
>>> + offset = sector_num * BDRV_SECTOR_SIZE;
>>> + size = nb_sectors * BDRV_SECTOR_SIZE;
>>> +
>>> + acb = qemu_aio_get(&vxhs_aiocb_info, bs, cb, opaque);
>>> + /*
>>> + * Setup or initialize VXHSAIOCB.
>>> + * Every single field should be initialized since
>>> + * acb will be picked up from the slab without
>>> + * initializing with zero.
>>> + */
>>> + acb->io_offset = offset;
>>> + acb->size = size;
>>> + acb->ret = 0;
>>> + acb->flags = 0;
>>> + acb->aio_done = VXHS_IO_INPROGRESS;
>>> + acb->segments = 0;
>>> + acb->buffer = 0;
>>> + acb->qiov = qiov;
>>> + acb->direction = iodir;
>>> +
>>> + qemu_spin_lock(&s->vdisk_lock);
>>> + if (OF_VDISK_FAILED(s)) {
>>> + trace_vxhs_aio_rw(s->vdisk_guid, iodir, size, offset);
>>> + qemu_spin_unlock(&s->vdisk_lock);
>>> + goto errout;
>>> + }
>>> + if (OF_VDISK_IOFAILOVER_IN_PROGRESS(s)) {
>>> + QSIMPLEQ_INSERT_TAIL(&s->vdisk_aio_retryq, acb, retry_entry);
>>> + s->vdisk_aio_retry_qd++;
>>> + OF_AIOCB_FLAGS_SET_QUEUED(acb);
>>> + qemu_spin_unlock(&s->vdisk_lock);
>>> + trace_vxhs_aio_rw_retry(s->vdisk_guid, acb, 1);
>>> + goto out;
>>> + }
>>> + s->vdisk_aio_count++;
>>> + qemu_spin_unlock(&s->vdisk_lock);
>>> +
>>> + iio_flags = (IIO_FLAG_DONE | IIO_FLAG_ASYNC);
>>> +
>>> + switch (iodir) {
>>> + case VDISK_AIO_WRITE:
>>> + vxhs_inc_acb_segment_count(acb, 1);
>>> + ret = vxhs_qnio_iio_writev(qnio_ctx, rfd, qiov,
>>> + offset, (void *)acb, iio_flags);
>>> + break;
>>> + case VDISK_AIO_READ:
>>> + vxhs_inc_acb_segment_count(acb, 1);
>>> + ret = vxhs_qnio_iio_readv(qnio_ctx, rfd, qiov,
>>> + offset, (void *)acb, iio_flags);
>>> + break;
>>> + default:
>>> + trace_vxhs_aio_rw_invalid(iodir);
>>> + goto errout;
>>
>> s->vdisk_aio_count must be decremented before returning.
>>
>>> +static void vxhs_close(BlockDriverState *bs)
>>> +{
>>> + BDRVVXHSState *s = bs->opaque;
>>> + int i;
>>> +
>>> + trace_vxhs_close(s->vdisk_guid);
>>> + close(s->fds[VDISK_FD_READ]);
>>> + close(s->fds[VDISK_FD_WRITE]);
>>> +
>>> + /*
>>> + * Clearing all the event handlers for oflame registered to QEMU
>>> + */
>>> + aio_set_fd_handler(bdrv_get_aio_context(bs), s->fds[VDISK_FD_READ],
>>> + false, NULL, NULL, NULL);
>>
>> Please remove the event handler before closing the fd. I don't think it
>> matters in this case but in other scenarios there could be race
>> conditions if another thread opens an fd and the file descriptor number
>> is reused.
