Re: [PATCH v3 03/11] 9pfs: validate count sent by client with T_readdir

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Donnerstag, 16. Januar 2020 14:33:42 CET Greg Kurz wrote:
> On Mon, 13 Jan 2020 23:22:08 +0100
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > A good 9p client sends T_readdir with "count" parameter that's
> > sufficiently smaller than client's initially negotiated msize
> > (maximum message size). We perform a check for that though to
> > avoid the server to be interrupted with a "Failed to encode
> > VirtFS reply type 41" error message by bad clients.
> > 
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> > 
> >  hw/9pfs/9p.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index a5fbe821d4..30da2fedf3 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -2426,6 +2426,7 @@ static void coroutine_fn v9fs_readdir(void *opaque)
> > 
> >      int32_t count;
> >      uint32_t max_count;
> >      V9fsPDU *pdu = opaque;
> > 
> > +    V9fsState *s = pdu->s;
> > 
> >      retval = pdu_unmarshal(pdu, offset, "dqd", &fid,
> >      
> >                             &initial_offset, &max_count);
> > 
> > @@ -2434,6 +2435,13 @@ static void coroutine_fn v9fs_readdir(void *opaque)
> > 
> >      }
> >      trace_v9fs_readdir(pdu->tag, pdu->id, fid, initial_offset,
> >      max_count);
> > 
> > +    if (max_count > s->msize - P9_IOHDRSZ) {
> 
> P9_IOHDRSZ relates to Twrite. The Rreaddir message has a smaller header
> of size 11:
> 
> size[4] Rreaddir tag[2] count[4]

Right, looks like I have falsely picked P9_IOHDRSZ after looking at:

static size_t v9fs_readdir_data_size(V9fsString *name)
{
    /*
     * Size of each dirent on the wire: size of qid (13) + size of offset (8)
     * size of type (1) + size of name.size (2) + strlen(name.data)
     */
    return 24 + v9fs_string_size(name);
}

I'll have to correct that in the test cases as well. So no need to comment on 
them for now.

But if you have an idea about the issue mentioned in cover letter (patch 7), 
let me know. I have a feeling that there is some problem with the test 
environment, because I also get strange error messages when I just add some 
more e.g. noop 9pfs test cases (empty test cases doing nothing) or by copy 
pasting existing tests and then running 

tests/qos-test -l

which obviously should just list the test cases, but not executing any of 
them. I'd end up with "cannot push stack" error messages for some reason.

> > +        max_count = s->msize - P9_IOHDRSZ;
> > +        warn_report_once(
> > +            "9p: bad client: T_readdir with count > msize - P9_IOHDRSZ"
> > +        );
> > +    }
> > +
> > 
> >      fidp = get_fid(pdu, fid);
> >      if (fidp == NULL) {
> >      
> >          retval = -EINVAL;

Best regards,
Christian Schoenebeck