Re: [Qemu-devel] [PATCH] 9pfs: fix integer overflow issue in xattr read/write

[Greg Kurz <groug@kaod.org>]

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 <liq3ea@gmail.com> wrote:

> From: Li Qiang <liqiang6-s@360.cn>
> 
> 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 <liqiang6-s@360.cn>
> ---
>  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