Re: [Qemu-devel] [PATCH V3 02/13] hw/9pfs: Add validation to marshal code

["Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>]

On Mon, 21 Nov 2011 19:06:07 +0530, "M. Mohan Kumar" <mohan@in.ibm.com> wrote:
> From: "M. Mohan Kumar" <mohan@in.ibm.com>
> 
> Add validatio check to {un}marshal code.
> 
> Signed-off-by: M. Mohan Kumar <mohan@in.ibm.com>
> ---
>  fsdev/virtio-9p-marshal.c |   97 ++++++++++++-------
>  fsdev/virtio-9p-marshal.h |    8 +-
>  hw/9pfs/virtio-9p.c       |  231 +++++++++++++++++++++++++++++++++------------
>  3 files changed, 236 insertions(+), 100 deletions(-)
> 
> diff --git a/fsdev/virtio-9p-marshal.c b/fsdev/virtio-9p-marshal.c
> index 2da0a34..74161df 100644
> --- a/fsdev/virtio-9p-marshal.c
> +++ b/fsdev/virtio-9p-marshal.c
> @@ -62,14 +62,14 @@ void v9fs_string_copy(V9fsString *lhs, V9fsString *rhs)
>  }
> 
> 
> -static size_t v9fs_packunpack(void *addr, struct iovec *sg, int sg_count,
> -                              size_t offset, size_t size, int pack)
> +static ssize_t v9fs_packunpack(void *addr, struct iovec *sg, int sg_count,
> +                              ssize_t offset, ssize_t size, int pack)
>  {
>      int i = 0;
> -    size_t copied = 0;
> +    ssize_t copied = 0;
> 
>      for (i = 0; size && i < sg_count; i++) {
> -        size_t len;
> +        ssize_t len;
>          if (offset >= sg[i].iov_len) {
>              /* skip this sg */
>              offset -= sg[i].iov_len;
> @@ -91,25 +91,29 @@ static size_t v9fs_packunpack(void *addr, struct iovec *sg, int sg_count,
>          }
>      }
> 
> +    /* could not copy requested 'size' */
> +    if (copied < 0) {
> +        return -1;
> +    }

I am not sure copied will be < 0 here even in error case


>      return copied;
>  }
> 
> -static size_t v9fs_unpack(void *dst, struct iovec *out_sg, int out_num,
> -                          size_t offset, size_t size)
> +static ssize_t v9fs_unpack(void *dst, struct iovec *out_sg, int out_num,
> +                          ssize_t offset, ssize_t size)
>  {
>      return v9fs_packunpack(dst, out_sg, out_num, offset, size, 0);
>  }
> 
> -size_t v9fs_pack(struct iovec *in_sg, int in_num, size_t offset,
> -                const void *src, size_t size)
> +ssize_t v9fs_pack(struct iovec *in_sg, int in_num, ssize_t offset,
> +                const void *src, ssize_t size)
>  {
>      return v9fs_packunpack((void *)src, in_sg, in_num, offset, size, 1);
>  }
> 
>  static int v9fs_copy_sg(struct iovec *src_sg, unsigned int num,
> -                        size_t offset, struct iovec *sg)
> +                        ssize_t offset, struct iovec *sg)
>  {
> -    size_t pos = 0;
> +    ssize_t pos = 0;
>      int i, j;
> 
>      j = 0;
> @@ -131,10 +135,10 @@ static int v9fs_copy_sg(struct iovec *src_sg, unsigned int num,
>      return j;
>  }
> 
> -size_t v9fs_unmarshal(struct iovec *out_sg, int out_num, size_t offset,
> -                int convert, const char *fmt, ...)
> +ssize_t v9fs_unmarshal(struct iovec *out_sg,
> +                int out_num, ssize_t offset, int convert, const char *fmt, ...)
>  {
> -    size_t old_offset = offset;
> +    ssize_t old_offset = offset, copied;
>      va_list ap;
>      int i;
> 
> @@ -143,13 +147,13 @@ size_t v9fs_unmarshal(struct iovec *out_sg, int out_num, size_t offset,
>          switch (fmt[i]) {
>          case 'b': {
>              uint8_t *valp = va_arg(ap, uint8_t *);
> -            offset += v9fs_unpack(valp, out_sg, out_num, offset, sizeof(*valp));
> +            copied = v9fs_unpack(valp, out_sg, out_num, offset, sizeof(*valp));
>              break;
>          }

Do we really need error checking for unpack ? unpack reads the value
from the buffer and store them in arguments passed. So what is the
failure condition here ? We always will have enough space to unpack.


>          case 'w': {
>              uint16_t val, *valp;
>              valp = va_arg(ap, uint16_t *);
> -            offset += v9fs_unpack(&val, out_sg, out_num, offset, sizeof(val));
> +            copied = v9fs_unpack(&val, out_sg, out_num, offset, sizeof(val));
>              if (convert) {
>                  *valp = le16_to_cpu(val);
>              } else {

-aneesh