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

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Donnerstag, 19. Dezember 2019 23:36:07 CET Stefano Stabellini wrote:
> On Thu, 19 Dec 2019, Christian Schoenebeck wrote:
> > On Donnerstag, 19. Dezember 2019 01:42:51 CET Stefano Stabellini wrote:
> > > From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > > 
> > > 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.
> > 
> > I haven't looked thoroughly at this yet, but that's about addressing a
> > temporary, not a permanent transport buffer size limitation, right?
> 
> Yes, that is correct.

One more thing ...

> 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;
>      }
> 
>      *piov = elem->in_sg;

Here could be a problem: what happens if the currently available transport 
buffer size is extremely small, i.e. less than P9_IOHDRSZ? I am not sure that 
would be handled safely everywhere. So maybe it would make sense to make 
transport buffer size < P9_IOHDRSZ an error case here?

Best regards,
Christian Schoenebeck