Re: [Qemu-devel] [PATCH] hw/9pfs: use g_strdup_printf() instead of PATH_MAX limitation

[Markus Armbruster]

Chen Gang <address@hidden> writes:

> When path is truncated by PATH_MAX limitation, it causes QEMU to access
> incorrect file. So use original full path instead of PATH_MAX within
> 9pfs (need check/process ENOMEM for related memory allocation).
>
> Also find/fix several another related issues when failure occurs.
>
>  - check 'fh' in handle_name_to_path().
>
>  - need call v9fs_string_free() at 'err_out:' in local_unlinkat().
>
>  - sprintf() will cause memory overflow in "virtio-9p-local.c"

Sound like distinct bugs.  Have you considered fixing them in separate
patches for easier review and clearer commit messages?

> The related test:
>
>  - Environments (for qemu-devel):
>
>    - Host is under fedora17 desktop with ext4fs:
>
>      qemu-system-x86_64 -hda test.img -m 1024 \
>        -net nic,vlan=4,model=virtio,macaddr=00:16:35:AF:94:04 \
>        -net tap,vlan=4,ifname=tap4,script=no,downscript=no \
>        -device virtio-9p-pci,id=fs0,fsdev=fsdev0,mount_tag=hostshare \
>        -fsdev local,security_model=passthrough,id=fsdev0,\
>          path=/upstream/vm/data/share/1234567890abcdefghijklmnopqrstuvwxyz\
>            ABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890acdefghijklmnopqrstuvwxyz\
>            ABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890/111111111111111111111111111\
>            1111111111111111111111111111111111111111111111111111222222222222\
>            2222222222222222222222222222222222222222222222222222222222222222\
>            2222222222222222222222222222222222233333333333333333333333333333\
>            3333333333333333333333333333333333
>
>     - Guest is ubuntu12 server with 9pfs.
>
>       mount -t 9p -o trans=virtio,version=9p2000.L hostshare /share
>
>     - Limitations:
>
>       full path limitation is PATH_MAX (4096B include nul) under Linux.
>       file/dir node name maximized length is 256 (include nul) under ext4.
>
>  - Special test:
>
>     Under host, modify the file: "/upstream/vm/data/share/1234567890abcdefg\
[...]
>       ppppppppppppppppppppppppppppppppppppppp/test1234567890file.log"
>         (need enter dir firstly, then modify file, or can not open it).
>
>    Under guest, still allow modify "test1234567890file.log" (will generate
>    "test123456" file with contents).
>
>    After apply this patch, can not open "test1234567890file.log" under guest
>    (permission denied).
>
>
>  - Common test:
>
>    All are still OK after apply this path.
>
>      "mkdir -p", "create/open file/dir", "modify file/dir", "rm file/dir".
>      change various mount point paths under host and/or guest.
>
>
> Signed-off-by: Chen Gang <address@hidden>
> ---
>  hw/9pfs/cofs.c                 |  22 ++-
>  hw/9pfs/virtio-9p-handle.c     |  25 ++-
>  hw/9pfs/virtio-9p-local.c      | 433 
> +++++++++++++++++++++++++++++++----------
>  hw/9pfs/virtio-9p-posix-acl.c  |  76 ++++++--
>  hw/9pfs/virtio-9p-xattr-user.c |  36 +++-
>  hw/9pfs/virtio-9p-xattr.c      |  13 +-
>  hw/9pfs/virtio-9p-xattr.h      |  39 +++-
>  hw/9pfs/virtio-9p.h            |   6 +-
>  8 files changed, 504 insertions(+), 146 deletions(-)
>
> diff --git a/hw/9pfs/cofs.c b/hw/9pfs/cofs.c
> index 3891050..ba69965 100644
> --- a/hw/9pfs/cofs.c
> +++ b/hw/9pfs/cofs.c
> @@ -20,18 +20,31 @@
>  int v9fs_co_readlink(V9fsPDU *pdu, V9fsPath *path, V9fsString *buf)
>  {
>      int err;
> -    ssize_t len;
> +    ssize_t len, maxlen = PATH_MAX;
>      V9fsState *s = pdu->s;
>  
>      if (v9fs_request_cancelled(pdu)) {
>          return -EINTR;
>      }
> -    buf->data = g_malloc(PATH_MAX);
> +    buf->data = g_malloc(maxlen);
> +    if (!buf->data) {

Can't happen, because g_malloc() returns NULL only when its argument is
zero.  Many more instances below, also with other memory allocation
functions, such as g_strdup_printf().  Please clean up and resubmit.

> +        return -ENOMEM;
> +    }
[...]