1. Patchew
  2. QEMU
  3. [PATCH v3 00/11] 9pfs: readdir optimization
  4. View patch

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

Christian Schoenebeck posted 11 patches 5 years, 10 months ago
Maintainers: Greg Kurz <groug@kaod.org>
There is a newer version of this series
[PATCH v3 03/11] 9pfs: validate count sent by client with T_readdir
Posted by Christian Schoenebeck 5 years, 10 months ago 
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) {
+        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;
-- 
2.20.1
Re: [PATCH v3 03/11] 9pfs: validate count sent by client with T_readdir
Posted by Greg Kurz 5 years, 10 months ago 
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]

> +        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;
Re: [PATCH v3 03/11] 9pfs: validate count sent by client with T_readdir
Posted by Christian Schoenebeck 5 years, 10 months ago 
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
Re: [PATCH v3 03/11] 9pfs: validate count sent by client with T_readdir
Posted by Greg Kurz 5 years, 10 months ago 
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
> 
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.