From dd28fbbc2edc0822965d402d927ce646326d6954 Mon Sep 17 00:00:00 2001
From: Li Qiang <liqiang6-s@360.cn>
Date: Tue, 1 Nov 2016 12:00:40 +0100
Subject: [PATCH 1/7] 9pfs: add xattrwalk_fid field in V9fsXattr struct
Currently, 9pfs sets the 'copied_len' field in V9fsXattr
to -1 to tag xattr walk fid. As the 'copied_len' is also
used to account for copied bytes, this may make confusion. This patch
add a bool 'xattrwalk_fid' to tag the xattr walk fid.
Suggested-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Li Qiang <liqiang6-s@360.cn>
Reviewed-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/9pfs/9p.c | 7 ++++---
hw/9pfs/9p.h | 1 +
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index e88cf257a2..ab18ef2adf 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -325,7 +325,7 @@ static int coroutine_fn v9fs_xattr_fid_clunk(V9fsPDU *pdu, V9fsFidState *fidp)
{
int retval = 0;
- if (fidp->fs.xattr.copied_len == -1) {
+ if (fidp->fs.xattr.xattrwalk_fid) {
/* getxattr/listxattr fid */
goto free_value;
}
@@ -3190,7 +3190,7 @@ static void coroutine_fn v9fs_xattrwalk(void *opaque)
*/
xattr_fidp->fs.xattr.len = size;
xattr_fidp->fid_type = P9_FID_XATTR;
- xattr_fidp->fs.xattr.copied_len = -1;
+ xattr_fidp->fs.xattr.xattrwalk_fid = true;
if (size) {
xattr_fidp->fs.xattr.value = g_malloc(size);
err = v9fs_co_llistxattr(pdu, &xattr_fidp->path,
@@ -3223,7 +3223,7 @@ static void coroutine_fn v9fs_xattrwalk(void *opaque)
*/
xattr_fidp->fs.xattr.len = size;
xattr_fidp->fid_type = P9_FID_XATTR;
- xattr_fidp->fs.xattr.copied_len = -1;
+ xattr_fidp->fs.xattr.xattrwalk_fid = true;
if (size) {
xattr_fidp->fs.xattr.value = g_malloc(size);
err = v9fs_co_lgetxattr(pdu, &xattr_fidp->path,
@@ -3279,6 +3279,7 @@ static void coroutine_fn v9fs_xattrcreate(void *opaque)
xattr_fidp = file_fidp;
xattr_fidp->fid_type = P9_FID_XATTR;
xattr_fidp->fs.xattr.copied_len = 0;
+ xattr_fidp->fs.xattr.xattrwalk_fid = false;
xattr_fidp->fs.xattr.len = size;
xattr_fidp->fs.xattr.flags = flags;
v9fs_string_init(&xattr_fidp->fs.xattr.name);
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index 2523a445f8..48065cc221 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -164,6 +164,7 @@ typedef struct V9fsXattr
void *value;
V9fsString name;
int flags;
+ bool xattrwalk_fid;
} V9fsXattr;
typedef struct V9fsDir {
From 8495f9ad26d398f01e208a53f1a5152483a16084 Mon Sep 17 00:00:00 2001
From: Li Qiang <liqiang6-s@360.cn>
Date: Tue, 1 Nov 2016 12:00:40 +0100
Subject: [PATCH 2/7] 9pfs: convert 'len/copied_len' field in V9fsXattr to the
type of uint64_t
The 'len' in V9fsXattr comes from the 'size' argument in setxattr()
function in guest. The setxattr() function's declaration is this:
int setxattr(const char *path, const char *name,
const void *value, size_t size, int flags);
and 'size' is treated as u64 in linux kernel client code:
int p9_client_xattrcreate(struct p9_fid *fid, const char *name,
u64 attr_size, int flags)
So the 'len' should have an type of 'uint64_t'.
The 'copied_len' in V9fsXattr is used to account for copied bytes, it
should also have an type of 'uint64_t'.
Suggested-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Li Qiang <liqiang6-s@360.cn>
Reviewed-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/9pfs/9p.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index 48065cc221..3976b7fe3d 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -159,8 +159,8 @@ typedef struct V9fsConf
typedef struct V9fsXattr
{
- int64_t copied_len;
- int64_t len;
+ uint64_t copied_len;
+ uint64_t len;
void *value;
V9fsString name;
int flags;
From 7e55d65c56a03dcd2c5d7c49d37c5a74b55d4bd6 Mon Sep 17 00:00:00 2001
From: Li Qiang <liqiang6-s@360.cn>
Date: Tue, 1 Nov 2016 12:00:40 +0100
Subject: [PATCH 3/7] 9pfs: fix integer overflow issue in xattr read/write
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The v9fs_xattr_read() and v9fs_xattr_write() are passed a guest
originated offset: they must ensure this offset does not go beyond
the size of the extended attribute that was set in v9fs_xattrcreate().
Unfortunately, the current code implement these checks with unsafe
calculations on 32 and 64 bit values, which may allow a malicious
guest to cause OOB access anyway.
Fix this by comparing the offset and the xattr size, which are
both uint64_t, before trying to compute the effective number of bytes
to read or write.
Suggested-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Li Qiang <liqiang6-s@360.cn>
Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-By: Guido Günther <agx@sigxcpu.org>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/9pfs/9p.c | 32 ++++++++++++--------------------
1 file changed, 12 insertions(+), 20 deletions(-)
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index ab18ef2adf..7705ead4b2 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1637,20 +1637,17 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
{
ssize_t err;
size_t offset = 7;
- int read_count;
- int64_t xattr_len;
+ uint64_t read_count;
V9fsVirtioState *v = container_of(s, V9fsVirtioState, state);
VirtQueueElement *elem = v->elems[pdu->idx];
- xattr_len = fidp->fs.xattr.len;
- read_count = xattr_len - off;
+ if (fidp->fs.xattr.len < off) {
+ read_count = 0;
+ } else {
+ read_count = fidp->fs.xattr.len - off;
+ }
if (read_count > max_count) {
read_count = max_count;
- } else if (read_count < 0) {
- /*
- * read beyond XATTR value
- */
- read_count = 0;
}
err = pdu_marshal(pdu, offset, "d", read_count);
if (err < 0) {
@@ -1979,23 +1976,18 @@ static int v9fs_xattr_write(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
{
int i, to_copy;
ssize_t err = 0;
- int write_count;
- int64_t xattr_len;
+ uint64_t write_count;
size_t offset = 7;
- xattr_len = fidp->fs.xattr.len;
- write_count = xattr_len - off;
- if (write_count > count) {
- write_count = count;
- } else if (write_count < 0) {
- /*
- * write beyond XATTR value len specified in
- * xattrcreate
- */
+ if (fidp->fs.xattr.len < off) {
err = -ENOSPC;
goto out;
}
+ write_count = fidp->fs.xattr.len - off;
+ if (write_count > count) {
+ write_count = count;
+ }
err = pdu_marshal(pdu, offset, "d", write_count);
if (err < 0) {
return err;
From 3b79ef2cf48805dc693a8b0c82e05e0abeaa64f8 Mon Sep 17 00:00:00 2001
From: Greg Kurz <groug@kaod.org>
Date: Tue, 1 Nov 2016 12:00:40 +0100
Subject: [PATCH 4/7] 9pfs: limit xattr size in xattrcreate
We shouldn't allow guests to create extended attribute with arbitrary sizes.
On linux hosts, the limit is XATTR_SIZE_MAX. Let's use it.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/9pfs/9p.c | 7 ++++++-
hw/9pfs/trace-events | 2 +-
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 7705ead4b2..27af007259 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -3247,7 +3247,7 @@ static void coroutine_fn v9fs_xattrcreate(void *opaque)
{
int flags;
int32_t fid;
- int64_t size;
+ uint64_t size;
ssize_t err = 0;
V9fsString name;
size_t offset = 7;
@@ -3262,6 +3262,11 @@ static void coroutine_fn v9fs_xattrcreate(void *opaque)
}
trace_v9fs_xattrcreate(pdu->tag, pdu->id, fid, name.data, size, flags);
+ if (size > XATTR_SIZE_MAX) {
+ err = -E2BIG;
+ goto out_nofid;
+ }
+
file_fidp = get_fid(pdu, fid);
if (file_fidp == NULL) {
err = -EINVAL;
diff --git a/hw/9pfs/trace-events b/hw/9pfs/trace-events
index 48d3d8abed..fb4de3d465 100644
--- a/hw/9pfs/trace-events
+++ b/hw/9pfs/trace-events
@@ -42,6 +42,6 @@ v9fs_mkdir(uint16_t tag, uint8_t id, int32_t fid, char* name, int mode, uint32_t
v9fs_mkdir_return(uint16_t tag, uint8_t id, int8_t type, int32_t version, int64_t path, int err) "tag %u id %u qid={type %d version %d path %"PRId64"} err %d"
v9fs_xattrwalk(uint16_t tag, uint8_t id, int32_t fid, int32_t newfid, char* name) "tag %d id %d fid %d newfid %d name %s"
v9fs_xattrwalk_return(uint16_t tag, uint8_t id, int64_t size) "tag %d id %d size %"PRId64
-v9fs_xattrcreate(uint16_t tag, uint8_t id, int32_t fid, char* name, int64_t size, int flags) "tag %d id %d fid %d name %s size %"PRId64" flags %d"
+v9fs_xattrcreate(uint16_t tag, uint8_t id, int32_t fid, char* name, uint64_t size, int flags) "tag %d id %d fid %d name %s size %"PRIu64" flags %d"
v9fs_readlink(uint16_t tag, uint8_t id, int32_t fid) "tag %d id %d fid %d"
v9fs_readlink_return(uint16_t tag, uint8_t id, char* target) "tag %d id %d name %s"
From dd654e0365c7b70df01920f1fca88dd7089eeb5d Mon Sep 17 00:00:00 2001
From: Greg Kurz <groug@kaod.org>
Date: Tue, 1 Nov 2016 12:00:40 +0100
Subject: [PATCH 5/7] 9pfs: xattrcreate requires non-opened fids
The xattrcreate operation only makes sense on a freshly cloned fid
actually, since any open state would be leaked because of the fid_type
change. This is indeed what the linux kernel client does:
fid = clone_fid(fid);
[...]
retval = p9_client_xattrcreate(fid, name, value_len, flags);
This patch also reverts commit ff55e94d23ae since we are sure that a fid
with type P9_FID_NONE doesn't have a previously allocated xattr.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/9pfs/9p.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 27af007259..547f3b5580 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -3272,6 +3272,11 @@ static void coroutine_fn v9fs_xattrcreate(void *opaque)
err = -EINVAL;
goto out_nofid;
}
+ if (file_fidp->fid_type != P9_FID_NONE) {
+ err = -EINVAL;
+ goto out_put_fid;
+ }
+
/* Make the file fid point to xattr */
xattr_fidp = file_fidp;
xattr_fidp->fid_type = P9_FID_XATTR;
@@ -3281,9 +3286,9 @@ static void coroutine_fn v9fs_xattrcreate(void *opaque)
xattr_fidp->fs.xattr.flags = flags;
v9fs_string_init(&xattr_fidp->fs.xattr.name);
v9fs_string_copy(&xattr_fidp->fs.xattr.name, &name);
- g_free(xattr_fidp->fs.xattr.value);
xattr_fidp->fs.xattr.value = g_malloc0(size);
err = offset;
+out_put_fid:
put_fid(pdu, file_fidp);
out_nofid:
pdu_complete(pdu, err);
From 49dd946bb5419681c8668b09a6d10f42bc707b78 Mon Sep 17 00:00:00 2001
From: Greg Kurz <groug@kaod.org>
Date: Tue, 1 Nov 2016 12:00:40 +0100
Subject: [PATCH 6/7] 9pfs: don't BUG_ON() if fid is already opened
A buggy or malicious guest could pass the id of an already opened fid and
cause QEMU to abort. Let's return EINVAL to the guest instead.
Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
hw/9pfs/9p.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 547f3b5580..1050b89ec7 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1361,7 +1361,10 @@ static void coroutine_fn v9fs_walk(void *opaque)
memcpy(&qids[name_idx], &qid, sizeof(qid));
}
if (fid == newfid) {
- BUG_ON(fidp->fid_type != P9_FID_NONE);
+ if (fidp->fid_type != P9_FID_NONE) {
+ err = -EINVAL;
+ goto out;
+ }
v9fs_path_copy(&fidp->path, &path);
} else {
newfidp = alloc_fid(s, newfid);
@@ -1443,7 +1446,10 @@ static void coroutine_fn v9fs_open(void *opaque)
err = -ENOENT;
goto out_nofid;
}
- BUG_ON(fidp->fid_type != P9_FID_NONE);
+ if (fidp->fid_type != P9_FID_NONE) {
+ err = -EINVAL;
+ goto out;
+ }
err = v9fs_co_lstat(pdu, &fidp->path, &stbuf);
if (err < 0) {
@@ -2540,7 +2546,10 @@ static int coroutine_fn v9fs_complete_rename(V9fsPDU *pdu, V9fsFidState *fidp,
err = -ENOENT;
goto out_nofid;
}
- BUG_ON(dirfidp->fid_type != P9_FID_NONE);
+ if (fidp->fid_type != P9_FID_NONE) {
+ err = -EINVAL;
+ goto out;
+ }
v9fs_co_name_to_path(pdu, &dirfidp->path, name->data, &new_path);
} else {
old_name = fidp->path.data;
@@ -2612,7 +2621,10 @@ static void coroutine_fn v9fs_rename(void *opaque)
err = -ENOENT;
goto out_nofid;
}
- BUG_ON(fidp->fid_type != P9_FID_NONE);
+ if (fidp->fid_type != P9_FID_NONE) {
+ err = -EINVAL;
+ goto out;
+ }
/* if fs driver is not path based, return EOPNOTSUPP */
if (!(pdu->s->ctx.export_flags & V9FS_PATHNAME_FSCONTEXT)) {
err = -EOPNOTSUPP;
From 79decce35b4d769fa878b048ab1a7b3e9045c9c6 Mon Sep 17 00:00:00 2001
From: Greg Kurz <groug@kaod.org>
Date: Tue, 1 Nov 2016 12:00:40 +0100
Subject: [PATCH 7/7] 9pfs: drop excessive error message from virtfs_reset()
The virtfs_reset() function is called either when the virtio-9p device
gets reset, or when the client starts a new 9P session. In both cases,
if it finds fids from a previous session, the following is printed in
the monitor:
9pfs:virtfs_reset: One or more uncluncked fids found during reset
For example, if a linux guest with a mounted 9P share is reset from the
monitor with system_reset, the message will be printed. This is excessive
since these fids are now clunked and the state is clean.
Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
hw/9pfs/9p.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 1050b89ec7..aea7e9d392 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -535,7 +535,7 @@ static int coroutine_fn v9fs_mark_fids_unreclaim(V9fsPDU *pdu, V9fsPath *path)
static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
{
V9fsState *s = pdu->s;
- V9fsFidState *fidp = NULL;
+ V9fsFidState *fidp;
/* Free all fids */
while (s->fid_list) {
@@ -548,11 +548,6 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
free_fid(pdu, fidp);
}
}
- if (fidp) {
- /* One or more unclunked fids found... */
- error_report("9pfs:%s: One or more uncluncked fids "
- "found during reset", __func__);
- }
}
#define P9_QID_TYPE_DIR 0x80