Re: [PATCH v2 2/9] 9pfs: validate count sent by client with T_readdir

[Greg Kurz <groug@kaod.org>]

On Wed, 18 Dec 2019 14:17:59 +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.
> 

Hmm... doesn't v9fs_do_readdir() already take care of that ?

> Note: we should probably also check for a minimum size of
> msize during T_version to avoid issues and/or too complicated
> count/size checks later on in other requests of that client.
> T_version should submit an msize that's at least as large as
> the largest request's header size.

Do you mean that the server should expose such an msize in the
R_version response ? The 9p spec only says that the server must
return an msize <= to the one proposed by the client [1]. Not
sure we can do more than to emit a warning and/or interrupt the
server if the client sends a silly size.

[1] https://9fans.github.io/plan9port/man/man9/version.html

> 
> 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 520177f40c..30e33b6573 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -2414,6 +2414,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);
> @@ -2422,6 +2423,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) {
> +        max_count = s->msize - P9_IOHDRSZ;

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