[v4,03/11] 9pfs: validate count sent by client with T_readdir

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" transport error message by
bad clients. This count value constraint uses msize - 11, because 11 is the
header size of R_readdir.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 hw/9pfs/9p.c | 8 ++++++++
 1 file changed, 8 insertions(+)

On Tue, 21 Jan 2020 00:50:33 +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" transport error message by
> bad clients. This count value constraint uses msize - 11, because 11 is the
> header size of R_readdir.
> 

This would be worth a comment...

> 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..18370183c4 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);
>  

... here. Something like:

    /* Enough space for a R_readdir header: size[4] Rreaddir tag[2] count[4] */

I can fix this in my tree, and actually done so since I've
applied patches 1 to 3.

Reviewed-by: Greg Kurz <groug@kaod.org>

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

On Mittwoch, 22. Januar 2020 15:11:07 CET Greg Kurz wrote:
> On Tue, 21 Jan 2020 00:50:33 +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" transport error message by
> > bad clients. This count value constraint uses msize - 11, because 11 is
> > the
> > header size of R_readdir.
> 
> This would be worth a comment...
> 
> > 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..18370183c4 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);
> 
> ... here. Something like:
> 
>     /* Enough space for a R_readdir header: size[4] Rreaddir tag[2] count[4]
> */
> 
> I can fix this in my tree, and actually done so since I've
> applied patches 1 to 3.

Fine with me, thanks Greg!

> Reviewed-by: Greg Kurz <groug@kaod.org>

Best regards,
Christian Schoenebeck