From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42999) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bufus-0002n5-PW for qemu-devel@nongnu.org; Thu, 13 Oct 2016 09:19:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bufun-0005u7-Pn for qemu-devel@nongnu.org; Thu, 13 Oct 2016 09:19:21 -0400 Received: from 5.mo179.mail-out.ovh.net ([46.105.43.140]:52699) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bufun-0005tc-F3 for qemu-devel@nongnu.org; Thu, 13 Oct 2016 09:19:17 -0400 Received: from player792.ha.ovh.net (b7.ovh.net [213.186.33.57]) by mo179.mail-out.ovh.net (Postfix) with ESMTP id 905E0FF9F14 for ; Thu, 13 Oct 2016 15:19:16 +0200 (CEST) Date: Thu, 13 Oct 2016 15:19:13 +0200 From: Greg Kurz Message-ID: <20161013151913.52d72014@bahia> In-Reply-To: <57ff5d86.4a3c9d0a.90936.609f@mx.google.com> References: <1476353383-4679-1-git-send-email-Qiang(liqiang6-s@360.cn)> <57ff5d86.4a3c9d0a.90936.609f@mx.google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 3/3] 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 On Thu, 13 Oct 2016 03:09:43 -0700 Li Qiang wrote: > From: Li Qiang > > 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 > Signed-off-by: Li Qiang > --- > Reviewed-by: Greg Kurz > Changes since v2: > -make the solution of 'copied_len/len' in V9fsXattr type issue to a separate patch. > -add detailed changelog. > > Changes since v1: > -delete 'xattr_len'. > > 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 e902eed..6df85b8 100644 > --- a/hw/9pfs/9p.c > +++ b/hw/9pfs/9p.c > @@ -1642,20 +1642,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) { > @@ -1982,23 +1979,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;