From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:35582) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RSV1b-0005Z5-L7 for qemu-devel@nongnu.org; Mon, 21 Nov 2011 09:39:17 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RSV1W-00078W-QS for qemu-devel@nongnu.org; Mon, 21 Nov 2011 09:39:11 -0500 Received: from e7.ny.us.ibm.com ([32.97.182.137]:34577) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RSV1W-00078K-Ge for qemu-devel@nongnu.org; Mon, 21 Nov 2011 09:39:06 -0500 Received: from /spool/local by e7.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 21 Nov 2011 09:39:04 -0500 Received: from d01av02.pok.ibm.com (d01av02.pok.ibm.com [9.56.224.216]) by d01relay05.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id pALEcfZV244010 for ; Mon, 21 Nov 2011 09:38:42 -0500 Received: from d01av02.pok.ibm.com (loopback [127.0.0.1]) by d01av02.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id pALEcfRw001769 for ; Mon, 21 Nov 2011 12:38:41 -0200 From: "Aneesh Kumar K.V" In-Reply-To: <1321882578-7498-3-git-send-email-mohan@in.ibm.com> References: <1321882578-7498-1-git-send-email-mohan@in.ibm.com> <1321882578-7498-3-git-send-email-mohan@in.ibm.com> Date: Mon, 21 Nov 2011 20:08:33 +0530 Message-ID: <87ty5x5ymu.fsf@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Subject: Re: [Qemu-devel] [PATCH V3 02/13] hw/9pfs: Add validation to marshal code List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "M. Mohan Kumar" , qemu-devel@nongnu.org, stefanha@gmail.com, berrange@redhat.com On Mon, 21 Nov 2011 19:06:07 +0530, "M. Mohan Kumar" wrote: > From: "M. Mohan Kumar" > > Add validatio check to {un}marshal code. > > Signed-off-by: M. Mohan Kumar > --- > 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