From 841b8d099c462cd4282c4ced8c2a6512899fd8d9 Mon Sep 17 00:00:00 2001 From: Jiajun Chen Date: Mon, 20 Jan 2020 15:11:39 +0100 Subject: [PATCH 1/5] 9pfs: local: Fix possible memory leak in local_link() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There is a possible memory leak while local_link return -1 without free odirpath and oname. Reported-by: Euler Robot Signed-off-by: Jaijun Chen Signed-off-by: Xiang Zheng Reviewed-by: Christian Schoenebeck Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Greg Kurz --- hw/9pfs/9p-local.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index ca641390fb..d0592c3b45 100644 --- a/hw/9pfs/9p-local.c +++ b/hw/9pfs/9p-local.c @@ -947,7 +947,7 @@ static int local_link(FsContext *ctx, V9fsPath *oldpath, if (ctx->export_flags & V9FS_SM_MAPPED_FILE && local_is_mapped_file_metadata(ctx, name)) { errno = EINVAL; - return -1; + goto out; } odirfd = local_opendir_nofollow(ctx, odirpath); From 846cf408a4c8055063f4a5a71ccf7ed030cdad30 Mon Sep 17 00:00:00 2001 From: Daniel Henrique Barboza Date: Mon, 20 Jan 2020 15:11:39 +0100 Subject: [PATCH 2/5] 9p: local: always return -1 on error in local_unlinkat_common local_unlinkat_common() is supposed to always return -1 on error. This is being done by jumps to the 'err_out' label, which is a 'return ret' call, and 'ret' is initialized with -1. Unfortunately there is a condition in which the function will return 0 on error: in a case where flags == AT_REMOVEDIR, 'ret' will be 0 when reaching map_dirfd = openat_dir(...) And, if map_dirfd == -1 and errno != ENOENT, the existing 'err_out' jump will execute 'return ret', when ret is still set to zero at that point. This patch fixes it by changing all 'err_out' labels by 'return -1' calls, ensuring that the function will always return -1 on error conditions. 'ret' can be left unintialized since it's now being used just to store the result of 'unlinkat' calls. CC: Greg Kurz Signed-off-by: Daniel Henrique Barboza [groug: changed prefix in title to be "9p: local:"] Signed-off-by: Greg Kurz --- hw/9pfs/9p-local.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index d0592c3b45..54e012e5b4 100644 --- a/hw/9pfs/9p-local.c +++ b/hw/9pfs/9p-local.c @@ -1076,7 +1076,7 @@ out: static int local_unlinkat_common(FsContext *ctx, int dirfd, const char *name, int flags) { - int ret = -1; + int ret; if (ctx->export_flags & V9FS_SM_MAPPED_FILE) { int map_dirfd; @@ -1094,12 +1094,12 @@ static int local_unlinkat_common(FsContext *ctx, int dirfd, const char *name, fd = openat_dir(dirfd, name); if (fd == -1) { - goto err_out; + return -1; } ret = unlinkat(fd, VIRTFS_META_DIR, AT_REMOVEDIR); close_preserve_errno(fd); if (ret < 0 && errno != ENOENT) { - goto err_out; + return -1; } } map_dirfd = openat_dir(dirfd, VIRTFS_META_DIR); @@ -1107,16 +1107,14 @@ static int local_unlinkat_common(FsContext *ctx, int dirfd, const char *name, ret = unlinkat(map_dirfd, name, 0); close_preserve_errno(map_dirfd); if (ret < 0 && errno != ENOENT) { - goto err_out; + return -1; } } else if (errno != ENOENT) { - goto err_out; + return -1; } } - ret = unlinkat(dirfd, name, flags); -err_out: - return ret; + return unlinkat(dirfd, name, flags); } static int local_remove(FsContext *ctx, const char *path) From 16724a173049ac29c7b5ade741da93a0f46edff7 Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Mon, 20 Jan 2020 15:11:39 +0100 Subject: [PATCH 3/5] 9p: init_in_iov_from_pdu can truncate the size init_in_iov_from_pdu might not be able to allocate the full buffer size requested, which comes from the client and could be larger than the transport has available at the time of the request. Specifically, this can happen with read operations, with the client requesting a read up to the max allowed, which might be more than the transport has available at the time. Today the implementation of init_in_iov_from_pdu throws an error, both Xen and Virtio. Instead, change the V9fsTransport interface so that the size becomes a pointer and can be limited by the implementation of init_in_iov_from_pdu. Change both the Xen and Virtio implementations to set the size to the size of the buffer they managed to allocate, instead of throwing an error. However, if the allocated buffer size is less than P9_IOHDRSZ (the size of the header) still throw an error as the case is unhandable. Signed-off-by: Stefano Stabellini CC: groug@kaod.org CC: anthony.perard@citrix.com CC: roman@zededa.com CC: qemu_oss@crudebyte.com [groug: fix 32-bit build] Signed-off-by: Greg Kurz --- hw/9pfs/9p.c | 33 ++++++++++++++++++++++----------- hw/9pfs/9p.h | 2 +- hw/9pfs/virtio-9p-device.c | 11 +++++++---- hw/9pfs/xen-9p-backend.c | 13 ++++++++----- 4 files changed, 38 insertions(+), 21 deletions(-) diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index 520177f40c..8b247b904e 100644 --- a/hw/9pfs/9p.c +++ b/hw/9pfs/9p.c @@ -2090,22 +2090,29 @@ out_nofid: * with qemu_iovec_destroy(). */ static void v9fs_init_qiov_from_pdu(QEMUIOVector *qiov, V9fsPDU *pdu, - size_t skip, size_t size, + size_t skip, size_t *size, bool is_write) { QEMUIOVector elem; struct iovec *iov; unsigned int niov; + size_t alloc_size = *size + skip; if (is_write) { - pdu->s->transport->init_out_iov_from_pdu(pdu, &iov, &niov, size + skip); + pdu->s->transport->init_out_iov_from_pdu(pdu, &iov, &niov, alloc_size); } else { - pdu->s->transport->init_in_iov_from_pdu(pdu, &iov, &niov, size + skip); + pdu->s->transport->init_in_iov_from_pdu(pdu, &iov, &niov, &alloc_size); + } + + if (alloc_size < skip) { + *size = 0; + } else { + *size = alloc_size - skip; } qemu_iovec_init_external(&elem, iov, niov); qemu_iovec_init(qiov, niov); - qemu_iovec_concat(qiov, &elem, skip, size); + qemu_iovec_concat(qiov, &elem, skip, *size); } static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp, @@ -2113,15 +2120,14 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp, { ssize_t err; size_t offset = 7; - uint64_t read_count; + size_t read_count; QEMUIOVector qiov_full; if (fidp->fs.xattr.len < off) { read_count = 0; - } else { + } else if (fidp->fs.xattr.len - off < max_count) { read_count = fidp->fs.xattr.len - off; - } - if (read_count > max_count) { + } else { read_count = max_count; } err = pdu_marshal(pdu, offset, "d", read_count); @@ -2130,7 +2136,7 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp, } offset += err; - v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset, read_count, false); + v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset, &read_count, false); err = v9fs_pack(qiov_full.iov, qiov_full.niov, 0, ((char *)fidp->fs.xattr.value) + off, read_count); @@ -2259,9 +2265,11 @@ static void coroutine_fn v9fs_read(void *opaque) QEMUIOVector qiov_full; QEMUIOVector qiov; int32_t len; + size_t size = max_count; - v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset + 4, max_count, false); + v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset + 4, &size, false); qemu_iovec_init(&qiov, qiov_full.niov); + max_count = size; do { qemu_iovec_reset(&qiov); qemu_iovec_concat(&qiov, &qiov_full, count, qiov_full.size - count); @@ -2504,6 +2512,7 @@ static void coroutine_fn v9fs_write(void *opaque) int32_t len = 0; int32_t total = 0; size_t offset = 7; + size_t size; V9fsFidState *fidp; V9fsPDU *pdu = opaque; V9fsState *s = pdu->s; @@ -2516,7 +2525,9 @@ static void coroutine_fn v9fs_write(void *opaque) return; } offset += err; - v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset, count, true); + size = count; + v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset, &size, true); + count = size; trace_v9fs_write(pdu->tag, pdu->id, fid, off, count, qiov_full.niov); fidp = get_fid(pdu, fid); diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h index 3904f82901..8d07a0b301 100644 --- a/hw/9pfs/9p.h +++ b/hw/9pfs/9p.h @@ -425,7 +425,7 @@ struct V9fsTransport { ssize_t (*pdu_vunmarshal)(V9fsPDU *pdu, size_t offset, const char *fmt, va_list ap); void (*init_in_iov_from_pdu)(V9fsPDU *pdu, struct iovec **piov, - unsigned int *pniov, size_t size); + unsigned int *pniov, size_t *size); void (*init_out_iov_from_pdu)(V9fsPDU *pdu, struct iovec **piov, unsigned int *pniov, size_t size); void (*push_and_notify)(V9fsPDU *pdu); diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c index b5a7c03f26..991e175c82 100644 --- a/hw/9pfs/virtio-9p-device.c +++ b/hw/9pfs/virtio-9p-device.c @@ -147,19 +147,22 @@ static ssize_t virtio_pdu_vunmarshal(V9fsPDU *pdu, size_t offset, } static void virtio_init_in_iov_from_pdu(V9fsPDU *pdu, struct iovec **piov, - unsigned int *pniov, size_t size) + unsigned int *pniov, size_t *size) { V9fsState *s = pdu->s; V9fsVirtioState *v = container_of(s, V9fsVirtioState, state); VirtQueueElement *elem = v->elems[pdu->idx]; size_t buf_size = iov_size(elem->in_sg, elem->in_num); - if (buf_size < size) { + if (buf_size < P9_IOHDRSZ) { VirtIODevice *vdev = VIRTIO_DEVICE(v); virtio_error(vdev, - "VirtFS reply type %d needs %zu bytes, buffer has %zu", - pdu->id + 1, size, buf_size); + "VirtFS reply type %d needs %zu bytes, buffer has %zu, less than minimum", + pdu->id + 1, *size, buf_size); + } + if (buf_size < *size) { + *size = buf_size; } *piov = elem->in_sg; diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c index 71eebe12dd..18fe5b7c92 100644 --- a/hw/9pfs/xen-9p-backend.c +++ b/hw/9pfs/xen-9p-backend.c @@ -187,7 +187,7 @@ static void xen_9pfs_init_out_iov_from_pdu(V9fsPDU *pdu, static void xen_9pfs_init_in_iov_from_pdu(V9fsPDU *pdu, struct iovec **piov, unsigned int *pniov, - size_t size) + size_t *size) { Xen9pfsDev *xen_9pfs = container_of(pdu->s, Xen9pfsDev, state); Xen9pfsRing *ring = &xen_9pfs->rings[pdu->tag % xen_9pfs->num_rings]; @@ -197,16 +197,19 @@ static void xen_9pfs_init_in_iov_from_pdu(V9fsPDU *pdu, g_free(ring->sg); ring->sg = g_new0(struct iovec, 2); - xen_9pfs_in_sg(ring, ring->sg, &num, pdu->idx, size); + xen_9pfs_in_sg(ring, ring->sg, &num, pdu->idx, *size); buf_size = iov_size(ring->sg, num); - if (buf_size < size) { + if (buf_size < P9_IOHDRSZ) { xen_pv_printf(&xen_9pfs->xendev, 0, "Xen 9pfs request type %d" - "needs %zu bytes, buffer has %zu\n", pdu->id, size, - buf_size); + "needs %zu bytes, buffer has %zu, less than minimum\n", + pdu->id, *size, buf_size); xen_be_set_state(&xen_9pfs->xendev, XenbusStateClosing); xen_9pfs_disconnect(&xen_9pfs->xendev); } + if (buf_size < *size) { + *size = buf_size; + } *piov = ring->sg; *pniov = num; From ff59c5ee78f11f0667c575b2b6c26a7d954658fb Mon Sep 17 00:00:00 2001 From: Daniel Henrique Barboza Date: Mon, 20 Jan 2020 15:11:39 +0100 Subject: [PATCH 4/5] virtfs-proxy-helper.c: remove 'err_out' label in setugid() 'err_out' can be removed and be replaced by 'return -errno' in its only instance in the function. CC: Greg Kurz Signed-off-by: Daniel Henrique Barboza Acked-by: Greg Kurz Signed-off-by: Greg Kurz --- fsdev/virtfs-proxy-helper.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c index 0d4de49dcf..aa1ab2590d 100644 --- a/fsdev/virtfs-proxy-helper.c +++ b/fsdev/virtfs-proxy-helper.c @@ -287,8 +287,7 @@ static int setugid(int uid, int gid, int *suid, int *sgid) *sgid = getegid(); if (setresgid(-1, gid, *sgid) == -1) { - retval = -errno; - goto err_out; + return -errno; } if (setresuid(-1, uid, *suid) == -1) { @@ -322,7 +321,6 @@ err_sgid: if (setresgid(-1, *sgid, *sgid) == -1) { abort(); } -err_out: return retval; } From b858e80a02ca64b9208499155f4dab4ef298b523 Mon Sep 17 00:00:00 2001 From: Daniel Henrique Barboza Date: Mon, 20 Jan 2020 15:11:39 +0100 Subject: [PATCH 5/5] 9pfs/9p.c: remove unneeded labels 'out' label in v9fs_xattr_write() and 'out_nofid' label in v9fs_complete_rename() can be replaced by appropriate return calls. CC: Greg Kurz Signed-off-by: Daniel Henrique Barboza Acked-by: Greg Kurz Signed-off-by: Greg Kurz --- hw/9pfs/9p.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index 8b247b904e..b0e445d6bd 100644 --- a/hw/9pfs/9p.c +++ b/hw/9pfs/9p.c @@ -2472,8 +2472,7 @@ static int v9fs_xattr_write(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp, if (fidp->fs.xattr.len < off) { - err = -ENOSPC; - goto out; + return -ENOSPC; } write_count = fidp->fs.xattr.len - off; if (write_count > count) { @@ -2499,7 +2498,7 @@ static int v9fs_xattr_write(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp, off += to_copy; write_count -= to_copy; } -out: + return err; } @@ -3067,8 +3066,7 @@ static int coroutine_fn v9fs_complete_rename(V9fsPDU *pdu, V9fsFidState *fidp, if (newdirfid != -1) { dirfidp = get_fid(pdu, newdirfid); if (dirfidp == NULL) { - err = -ENOENT; - goto out_nofid; + return -ENOENT; } if (fidp->fid_type != P9_FID_NONE) { err = -EINVAL; @@ -3111,7 +3109,6 @@ out: put_fid(pdu, dirfidp); } v9fs_path_free(&new_path); -out_nofid: return err; }