On Mon, Mar 21, 2011 at 6:31 PM, Harsh Prateek Bora <ha...@linux.vnet.ibm.com> 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 <ha...@linux.vnet.ibm.com> > --- > 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; > + goto out; > }
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? Stefan