[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 3/3] 9pfs: fix integer overflow issue in xatt
From: |
Greg Kurz |
Subject: |
Re: [Qemu-devel] [PATCH v3 3/3] 9pfs: fix integer overflow issue in xattr read/write |
Date: |
Thu, 13 Oct 2016 15:19:13 +0200 |
On Thu, 13 Oct 2016 03:09:43 -0700
Li Qiang <address@hidden> wrote:
> From: Li Qiang <address@hidden>
>
> 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 <address@hidden>
> Signed-off-by: Li Qiang <address@hidden>
> ---
>
Reviewed-by: Greg Kurz <address@hidden>
> 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;