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

[Greg Kurz <groug@kaod.org>]

On Thu, 16 Jan 2020 17:51:10 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

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

No idea. I'll have to look more.

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