| Message ID | 57ff5d86.4a3c9d0a.90936.609f@mx.google.com |
|---|---|
| State | New |
| Headers | show |
On Thu, 13 Oct 2016 03:09:43 -0700 Li Qiang <liq3ea@gmail.com> wrote: > From: Li Qiang <liqiang6-s@360.cn> > > 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> > 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;
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;