Re: [PATCH] 9p: init_in_iov_from_pdu can truncate the size

[Greg Kurz]

On Wed, 18 Dec 2019 16:42:51 -0800
Stefano Stabellini <address@hidden> wrote:

> From: Stefano Stabellini <address@hidden>
> 
> init_in_iov_from_pdu might not be able to allocate the full buffer size
> requested, which comes from the client and could be larger than the
> transport has available at the time of the request. Specifically, this
> can happen with read operations, with the client requesting a read up to
> the max allowed, which might be more than the transport has available at
> the time.
> 
> Today the implementation of init_in_iov_from_pdu throws an error, both
> Xen and Virtio.
> 
> Instead, change the V9fsTransport interface so that the size becomes a
> pointer and can be limited by the implementation of
> init_in_iov_from_pdu.
> 
> Change both the Xen and Virtio implementations to set the size to the
> size of the buffer they managed to allocate, instead of throwing an
> error.
> 
> Signed-off-by: Stefano Stabellini <address@hidden>
> CC: address@hidden
> CC: address@hidden
> ---
>  hw/9pfs/9p.c               | 22 +++++++++++++++-------
>  hw/9pfs/9p.h               |  2 +-
>  hw/9pfs/virtio-9p-device.c | 10 +++-------
>  hw/9pfs/xen-9p-backend.c   | 12 ++++--------
>  4 files changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index bdf7919abf..d6c89ce608 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -1682,22 +1682,30 @@ out_nofid:
>   * with qemu_iovec_destroy().
>   */
>  static void v9fs_init_qiov_from_pdu(QEMUIOVector *qiov, V9fsPDU *pdu,
> -                                    size_t skip, size_t size,
> +                                    size_t skip, size_t *size,
>                                      bool is_write)
>  {
>      QEMUIOVector elem;
>      struct iovec *iov;
>      unsigned int niov;
> +    size_t alloc_size = *size + skip;
>  
>      if (is_write) {
> -        pdu->s->transport->init_out_iov_from_pdu(pdu, &iov, &niov, size + 
> skip);
> +        pdu->s->transport->init_out_iov_from_pdu(pdu, &iov, &niov, 
> alloc_size);
>      } else {
> -        pdu->s->transport->init_in_iov_from_pdu(pdu, &iov, &niov, size + 
> skip);
> +        pdu->s->transport->init_in_iov_from_pdu(pdu, &iov, &niov, 
> &alloc_size);
> +    }
> +
> +    if (alloc_size < skip)
> +    {
> +        *size = 0;
> +    } else {
> +        *size = alloc_size - skip;
>      }
>      qemu_iovec_init_external(&elem, iov, niov);
>      qemu_iovec_init(qiov, niov);
> -    qemu_iovec_concat(qiov, &elem, skip, size);
> +    qemu_iovec_concat(qiov, &elem, skip, *size);
>  }
>  
>  static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
> @@ -1722,7 +1730,7 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, 
> V9fsFidState *fidp,
>      }
>      offset += err;
>  
> -    v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset, read_count, false);
> +    v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset, &read_count, false);


>      err = v9fs_pack(qiov_full.iov, qiov_full.niov, 0,
>                      ((char *)fidp->fs.xattr.value) + off,
>                      read_count);
> @@ -1852,7 +1860,7 @@ static void coroutine_fn v9fs_read(void *opaque)
>          QEMUIOVector qiov;
>          int32_t len;
>  
> -        v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset + 4, max_count, 
> false);
> +        v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset + 4, &max_count, 
> false);

This doesn't build:

hw/9pfs/9p.c: In function ‘v9fs_read’:
hw/9pfs/9p.c:2271:62: error: passing argument 4 of ‘v9fs_init_qiov_from_pdu’ 
from incompatible pointer type [-Werror=incompatible-pointer-types]
 2271 |         v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset + 4, 
&max_count, false);
      |                                                              ^~~~~~~~~~
      |                                                              |
      |                                                              uint32_t * 
{aka unsigned int *}
hw/9pfs/9p.c:2093:58: note: expected ‘size_t *’ {aka ‘long unsigned int *’} but 
argument is of type ‘uint32_t *’ {aka ‘unsigned int *’}
 2093 |                                     size_t skip, size_t *size,
      |                                                  ~~~~~~~~^~~~

>          qemu_iovec_init(&qiov, qiov_full.niov);
>          do {
>              qemu_iovec_reset(&qiov);
> @@ -2085,7 +2093,7 @@ static void coroutine_fn v9fs_write(void *opaque)
>          return;
>      }
>      offset += err;
> -    v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset, count, true);
> +    v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset, &count, true);

Same here.

hw/9pfs/9p.c: In function ‘v9fs_write’:
hw/9pfs/9p.c:2527:54: error: passing argument 4 of ‘v9fs_init_qiov_from_pdu’ 
from incompatible pointer type [-Werror=incompatible-pointer-types]
 2527 |     v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset, &count, true);
      |                                                      ^~~~~~
      |                                                      |
      |                                                      uint32_t * {aka 
unsigned int *}
hw/9pfs/9p.c:2093:58: note: expected ‘size_t *’ {aka ‘long unsigned int *’} but 
argument is of type ‘uint32_t *’ {aka ‘unsigned int *’}
 2093 |                                     size_t skip, size_t *size,
      |                                                  ~~~~~~~~^~~~

>      trace_v9fs_write(pdu->tag, pdu->id, fid, off, count, qiov_full.niov);
>  
>      fidp = get_fid(pdu, fid);
> diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> index 8883761b2c..50f7e21da6 100644
> --- a/hw/9pfs/9p.h
> +++ b/hw/9pfs/9p.h
> @@ -365,7 +365,7 @@ struct V9fsTransport {
>      ssize_t     (*pdu_vunmarshal)(V9fsPDU *pdu, size_t offset, const char 
> *fmt,
>                                    va_list ap);
>      void        (*init_in_iov_from_pdu)(V9fsPDU *pdu, struct iovec **piov,
> -                                        unsigned int *pniov, size_t size);
> +                                        unsigned int *pniov, size_t *size);
>      void        (*init_out_iov_from_pdu)(V9fsPDU *pdu, struct iovec **piov,
>                                           unsigned int *pniov, size_t size);
>      void        (*push_and_notify)(V9fsPDU *pdu);
> diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> index 775e8ff766..68873c3f5f 100644
> --- a/hw/9pfs/virtio-9p-device.c
> +++ b/hw/9pfs/virtio-9p-device.c
> @@ -145,19 +145,15 @@ static ssize_t virtio_pdu_vunmarshal(V9fsPDU *pdu, 
> size_t offset,
>  }
>  
>  static void virtio_init_in_iov_from_pdu(V9fsPDU *pdu, struct iovec **piov,
> -                                        unsigned int *pniov, size_t size)
> +                                        unsigned int *pniov, size_t *size)
>  {
>      V9fsState *s = pdu->s;
>      V9fsVirtioState *v = container_of(s, V9fsVirtioState, state);
>      VirtQueueElement *elem = v->elems[pdu->idx];
>      size_t buf_size = iov_size(elem->in_sg, elem->in_num);
>  
> -    if (buf_size < size) {
> -        VirtIODevice *vdev = VIRTIO_DEVICE(v);
> -
> -        virtio_error(vdev,
> -                     "VirtFS reply type %d needs %zu bytes, buffer has %zu",
> -                     pdu->id + 1, size, buf_size);
> +    if (buf_size < *size) {
> +        *size = buf_size;
>      }

As suggested by Christian in some other mail, it could still make sense to
raise the error if there isn't even enough space to pack a 9p message header.

>  
>  
>      *piov = elem->in_sg;
> diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
> index 3f54a21c76..3994a356d4 100644
> --- a/hw/9pfs/xen-9p-backend.c
> +++ b/hw/9pfs/xen-9p-backend.c
> @@ -187,7 +187,7 @@ static void xen_9pfs_init_out_iov_from_pdu(V9fsPDU *pdu,
>  static void xen_9pfs_init_in_iov_from_pdu(V9fsPDU *pdu,
>                                            struct iovec **piov,
>                                            unsigned int *pniov,
> -                                          size_t size)
> +                                          size_t *size)
>  {
>      Xen9pfsDev *xen_9pfs = container_of(pdu->s, Xen9pfsDev, state);
>      Xen9pfsRing *ring = &xen_9pfs->rings[pdu->tag % xen_9pfs->num_rings];
> @@ -197,15 +197,11 @@ static void xen_9pfs_init_in_iov_from_pdu(V9fsPDU *pdu,
>      g_free(ring->sg);
>  
>      ring->sg = g_malloc0(sizeof(*ring->sg) * 2);
> -    xen_9pfs_in_sg(ring, ring->sg, &num, pdu->idx, size);
> +    xen_9pfs_in_sg(ring, ring->sg, &num, pdu->idx, *size);
>  

This chunk doesn't apply on master:

<<<<<<<
    ring->sg = g_new0(struct iovec, 2);
    xen_9pfs_in_sg(ring, ring->sg, &num, pdu->idx, size);
=======
    ring->sg = g_malloc0(sizeof(*ring->sg) * 2);
    xen_9pfs_in_sg(ring, ring->sg, &num, pdu->idx, *size);
>>>>>>>


>      buf_size = iov_size(ring->sg, num);
> -    if (buf_size  < size) {
> -        xen_pv_printf(&xen_9pfs->xendev, 0, "Xen 9pfs request type %d"
> -                "needs %zu bytes, buffer has %zu\n", pdu->id, size,
> -                buf_size);
> -        xen_be_set_state(&xen_9pfs->xendev, XenbusStateClosing);
> -        xen_9pfs_disconnect(&xen_9pfs->xendev);
> +    if (buf_size  < *size) {
> +        *size = buf_size;
>      }
>  
>      *piov = ring->sg;