From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=43696 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q1mnt-0003FK-OF for qemu-devel@nongnu.org; Mon, 21 Mar 2011 17:38:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q1mns-0001Ni-AG for qemu-devel@nongnu.org; Mon, 21 Mar 2011 17:38:21 -0400 Received: from e5.ny.us.ibm.com ([32.97.182.145]:60580) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q1mns-0001Na-7Q for qemu-devel@nongnu.org; Mon, 21 Mar 2011 17:38:20 -0400 Received: from d01dlp01.pok.ibm.com (d01dlp01.pok.ibm.com [9.56.224.56]) by e5.ny.us.ibm.com (8.14.4/8.13.1) with ESMTP id p2LLCtmE014860 for ; Mon, 21 Mar 2011 17:12:55 -0400 Received: from d01relay04.pok.ibm.com (d01relay04.pok.ibm.com [9.56.227.236]) by d01dlp01.pok.ibm.com (Postfix) with ESMTP id 3616238C8038 for ; Mon, 21 Mar 2011 17:38:03 -0400 (EDT) Received: from d03av02.boulder.ibm.com (d03av02.boulder.ibm.com [9.17.195.168]) by d01relay04.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p2LLbXHW148888 for ; Mon, 21 Mar 2011 17:37:34 -0400 Received: from d03av02.boulder.ibm.com (loopback [127.0.0.1]) by d03av02.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p2LLbXg5032149 for ; Mon, 21 Mar 2011 15:37:33 -0600 Message-ID: <4D87C51A.6090204@linux.vnet.ibm.com> Date: Mon, 21 Mar 2011 14:37:30 -0700 From: "Venkateswararao Jujjuri (JV)" MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] v9fs_walk: As per 9p2000 RFC, MAXWELEM >= nwnames >= 0. References: <1300732315-1997-1-git-send-email-harsh@linux.vnet.ibm.com> <4D87C471.2000004@linux.vnet.ibm.com> In-Reply-To: <4D87C471.2000004@linux.vnet.ibm.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Harsh Prateek Bora Cc: Stefan Hajnoczi , qemu-devel@nongnu.org, aneesh.kumar@linux.vnet.ibm.com On 3/21/2011 2:34 PM, Venkateswararao Jujjuri (JV) wrote: > On 3/21/2011 2:16 PM, Stefan Hajnoczi wrote: >> On Mon, Mar 21, 2011 at 6:31 PM, Harsh Prateek Bora >> wrote: >>> The nwnames field in TWALK message is assumed to be >=0 and <= MAXWELEM >>> which is defined as macro P9_MAXWELEM (16) in virtio-9p.h as per 9p2000 RFC. >>> Appropriate changes are required in V9fsWalkState and v9fs_walk. >>> >>> Signed-off-by: Harsh Prateek Bora >>> --- >>> hw/9pfs/virtio-9p.c | 5 ++++- >>> hw/9pfs/virtio-9p.h | 2 +- >>> 2 files changed, 5 insertions(+), 2 deletions(-) >>> >>> diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c >>> index 9b44bd0..b782a19 100644 >>> --- a/hw/9pfs/virtio-9p.c >>> +++ b/hw/9pfs/virtio-9p.c >>> @@ -1805,7 +1805,7 @@ static void v9fs_walk(V9fsState *s, V9fsPDU *pdu) >>> vs->offset += pdu_unmarshal(vs->pdu, vs->offset, "ddw", &fid, >>> &newfid, &vs->nwnames); >>> >>> - if (vs->nwnames) { >>> + if (vs->nwnames <= P9_MAXWELEM) { >>> vs->wnames = qemu_mallocz(sizeof(vs->wnames[0]) * vs->nwnames); >>> >>> vs->qids = qemu_mallocz(sizeof(vs->qids[0]) * vs->nwnames); >>> @@ -1814,6 +1814,9 @@ static void v9fs_walk(V9fsState *s, V9fsPDU *pdu) >>> vs->offset += pdu_unmarshal(vs->pdu, vs->offset, "s", >>> &vs->wnames[i]); >>> } >>> + } else { >>> + err = -EINVAL; > vs->nwnames = 0; will take care of v9fs_walk_complete() issue Stefan brought up. >>> + goto out; Or you can have same check "if (vs->nwnames <= P9_MAXWELEM) {" in the v9fs_walk_complete() too; basically you need to be consistant with the checking. - JV >>> } >> >> v9fs_walk_complete() will attempt to free wnames, qids, and the wnames >> strings. Freeing the strings will crash because we're indexing into >> an array based off a NULL pointer. >> >> It would be very handy to have a PDU-level test suite. You could then >> construct a PDU with an invalid nwnames field and exercise this code >> path. Perhaps a debug ioctl in Linux v9fs that allows a userspace >> tool to pass PDUs through will do? > > Ioctl way was ruled out in the past discussion. But yes we have in plan on to > introduce > a test suite of this kind. Malahal (who is on vacation) will start this work > from early April. > > Thanks, > JV > >> >> Stefan >