| Message ID | 1300732315-1997-1-git-send-email-harsh@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Mar 21, 2011 at 6:31 PM, Harsh Prateek Bora <harsh@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 <harsh@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
On 3/21/2011 2:16 PM, Stefan Hajnoczi wrote: > On Mon, Mar 21, 2011 at 6:31 PM, Harsh Prateek Bora > <harsh@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 <harsh@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; vs->nwnames = 0; will take care of v9fs_walk_complete() issue Stefan brought up. >> + 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? 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
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 >> <harsh@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 <harsh@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; > 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 >
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; } vs->fidp = lookup_fid(s, fid); diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h index e7d2326..119b12c 100644 --- a/hw/9pfs/virtio-9p.h +++ b/hw/9pfs/virtio-9p.h @@ -307,7 +307,7 @@ typedef struct V9fsStatStateDotl { typedef struct V9fsWalkState { V9fsPDU *pdu; size_t offset; - int16_t nwnames; + uint16_t nwnames; int name_idx; V9fsQID *qids; V9fsFidState *fidp;
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 <harsh@linux.vnet.ibm.com> --- hw/9pfs/virtio-9p.c | 5 ++++- hw/9pfs/virtio-9p.h | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-)