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

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

[Harsh Prateek Bora]

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;
     }
 
     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;
-- 
1.7.1.1

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
<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

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

[Venkateswararao Jujjuri (JV)]

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

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

[Venkateswararao Jujjuri (JV)]

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
>