9pfs: fix integer overflow issue in xattr read/write
PATCH 1/1 9pfs: fix integer overflow issue in xattr read/write
From: Li Qiang <[email protected]>
In 9pfs xattr read/write function, it mix to use unsigned/signed ,32/64 bits integers. This will causes oob read/write issues. This patch fix this.
--- hw/9pfs/9p.c | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-)
hw/9pfs/9p.c +13 -16
@@ -1642,21 +1642,21 @@ 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; +uint64_t xattr_len;V9fsVirtioState *v =container_of (s, V9fsVirtioState, state);VirtQueueElement *elem = v->elems[pdu->idx]; xattr_len = fidp->fs.xattr.len; +if (xattr_len < off) { + read_count =0 ; + goto over_read_count; + } read_count = xattr_len - off;if (read_count > max_count) { read_count = max_count; - }else if (read_count <0 ) { -/* -* read beyond XATTR value -*/ - read_count =0 ; } +over_read_count: err =pdu_marshal (pdu, offset,"d" , read_count);if (err <0 ) {return err;
@@ -1982,22 +1982,19 @@ 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; +uint64_t xattr_len;size_t offset =7 ; xattr_len = fidp->fs.xattr.len; +if (xattr_len < off) { + err = -ENOSPC ; + goto out; + } 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 -*/ - err = -ENOSPC ; - goto out; } err =pdu_marshal (pdu, offset,"d" , write_count);if (err <0 ) {
Signature · 1 line
1.8.3.1
Hi Li,
I agree with the idea behind this patch but I have the impression that some more work is needed. See below.
Quoted 6 lines · On Tue, 11 Oct 2016 21:29:19 -0700 Li Qiang <[email protected]> wrote:
From: Li Qiang <[email protected]>
In 9pfs xattr read/write function, it mix to use unsigned/signed ,32/64 bits integers. This will causes oob read/write issues. This patch fix this.
The root cause for OOB to happen is that the off argument is an unint64_t coming from the guest: if it exceeds xattr_len more than INT_MAX+2, then write_count will be equal to INT_MAX and pass the write_count < 0 check. The use of proper types is needed to detect that.
What about this:
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.
Let's 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.
Quoted 10 lines
Signed-off-by Li Qiang <[email protected]>--- hw/9pfs/9p.c | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-)hw/9pfs/9p.c +0
-0
hw/9pfs/9p.c static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp, · line 1646
Quoted 7 lines
hw/9pfs/9p.c +2
-2@@ -1642,21 +1642,21 @@ 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; +uint64_t xattr_len;
I don't think xattr_len is needed. See below.
hw/9pfs/9p.c static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp, · line 1650
Quoted 4 lines
hw/9pfs/9p.c +0
-0V9fsVirtioState *v =container_of (s, V9fsVirtioState, state);VirtQueueElement *elem = v->elems[pdu->idx]; xattr_len = fidp->fs.xattr.len;
typedef struct V9fsXattr
{
int64_t copied_len;
int64_t len;I believe len should be uint64_t since it comes from the size argument to setxattr() in the guest:
int setxattr(const char *path, const char *name,
const void *value, size_t size, int flags);
and it is treated as u64 in the linux kernel client code:
int p9_client_xattrcreate(struct p9_fid *fid, const char *name,
u64 attr_size, int flags)
I guess copied_len should also be turned to uint64_t as well since its main use is to account for copied bytes. And introduce a xattrwalk_fid bool instead of setting copied_len to -1.
I suggest you fix the types in some prelimary patches and then build this patch on top.
hw/9pfs/9p.c static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp, · line 1653
Quoted 3 lines
hw/9pfs/9p.c +3
-0+if (xattr_len < off) { + read_count =0 ; + goto over_read_count;
goto should only be used when doing rollback on error paths, which is not the case here. Please use a regular if...else construct instead.
Quoted 15 lines
hw/9pfs/9p.c +2
-5+ } read_count = xattr_len - off;if (read_count > max_count) { read_count = max_count; - }else if (read_count <0 ) { -/* -* read beyond XATTR value -*/ - read_count =0 ; } +over_read_count: err =pdu_marshal (pdu, offset,"d" , read_count);if (err <0 ) {return err;
hw/9pfs/9p.c static int v9fs_xattr_write(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp, · line 1986
Quoted 7 lines
hw/9pfs/9p.c +2
-2@@ -1982,22 +1982,19 @@ 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; +uint64_t xattr_len;
Same remark: xattr_len not needed.
Quoted 11 lines
hw/9pfs/9p.c +4
-0size_t offset =7 ; xattr_len = fidp->fs.xattr.len; +if (xattr_len < off) { + err = -ENOSPC ; + goto out; + } write_count = xattr_len - off;if (write_count > count) { write_count = count;
hw/9pfs/9p.c static int v9fs_xattr_write(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp, · line 2000
Quoted 10 lines
hw/9pfs/9p.c +0
-7- }else if (write_count <0 ) { -/* -* write beyond XATTR value len specified in -* xattrcreate -*/ - err = -ENOSPC ; - goto out; } err =pdu_marshal (pdu, offset,"d" , write_count);if (err <0 ) {
Cheers.
--
Greg