Re: [Qemu-devel] [PATCH] v9fs_walk: As per 9p2000 RFC, MAXWELEM >= nwnames >= 0.

[Stefan Hajnoczi]

On Mon, Mar 21, 2011 at 6:31 PM, Harsh Prateek Bora
<address@hidden> 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 <address@hidden>
> ---
>  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