From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52791) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1buJep-0003EG-Gr for qemu-devel@nongnu.org; Wed, 12 Oct 2016 09:33:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1buJek-0002MS-AC for qemu-devel@nongnu.org; Wed, 12 Oct 2016 09:33:19 -0400 Received: from 4.mo179.mail-out.ovh.net ([46.105.36.149]:38806) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1buJek-0002Lr-1J for qemu-devel@nongnu.org; Wed, 12 Oct 2016 09:33:14 -0400 Received: from player792.ha.ovh.net (b7.ovh.net [213.186.33.57]) by mo179.mail-out.ovh.net (Postfix) with ESMTP id E1AA6FFC844 for ; Wed, 12 Oct 2016 15:33:10 +0200 (CEST) Date: Wed, 12 Oct 2016 15:33:05 +0200 From: Greg Kurz Message-ID: <20161012153305.216b972e@bahia> In-Reply-To: <57fdbc36.486b240a.941a3.ac8a@mx.google.com> References: <57fdbc36.486b240a.941a3.ac8a@mx.google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] 9pfs: fix integer overflow issue in xattr read/write List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Li Qiang Cc: qemu-devel@nongnu.org, Li Qiang Hi Li, I agree with the idea behind this patch but I have the impression that some more work is needed. See below. On Tue, 11 Oct 2016 21:29:19 -0700 Li Qiang wrote: > From: Li Qiang > > 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. > Signed-off-by: Li Qiang > --- > hw/9pfs/9p.c | 29 +++++++++++++---------------- > 1 file changed, 13 insertions(+), 16 deletions(-) > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > index e4040dc..8b50bfb 100644 > --- a/hw/9pfs/9p.c > +++ b/hw/9pfs/9p.c > @@ -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. > V9fsVirtioState *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. > + 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. > + } > 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; Same remark: xattr_len not needed. > 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) { Cheers. -- Greg