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