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

[Greg Kurz <groug@kaod.org>]

On Mon, 06 Jan 2020 16:10:28 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Montag, 6. Januar 2020 13:30:24 CET Greg Kurz wrote:
> > 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 ?
> 
> No, unfortunately it doesn't. It just looks at the "count" parameter 
> transmitted with client's T_readdir request, but not at session's msize.

Indeed.

> So a bad client could send a T_readdir request with a "count" parameter that's 
> substantially higher than session's msize, which would lead to that mentioned 
> transport error ATM. Hence I suggested this patch here to address it.
> 
> You can easily reproduce this issue:
> 
> 1. Omit this patch 2 (since it would fix it).
> 
> 2. Apply patch 3 and patch 4 of this patch set, which assemble a T_readdir
>    test case combined, then stop patches here (i.e. don't apply subsequent 
>    patches of this patch set, since e.g. patch 6 would increase test client's 
>    msize).
> 
> 3. Set QTEST_V9FS_SYNTH_READDIR_NFILES in hw/9pfs/9p-synth.h to a high number
>    (e.g. several thousands).
> 
> 4. Run the T_readdir test case:
> 
>    cd build
>    make && make tests/qos-test
>    export QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64
>    tests/qos-test -p $(tests/qos-test -l | grep readdir/basic)
> 
> Result: Since the test client's msize is quite small at this point (4kB), test 
> client would transmit a very large "count" parameter with T_readdir request to 
> retrieve all QTEST_V9FS_SYNTH_READDIR_NFILES files, and you'll end up getting 
> the quoted transport error message on server (see precise error message 
> above).
> 

I don't observe this behavior with:

-#define QTEST_V9FS_SYNTH_READDIR_NFILES 100
+#define QTEST_V9FS_SYNTH_READDIR_NFILES 20000

/x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio-9p-tests/fs/readdir/basic: **
ERROR:/home/greg/Work/qemu/qemu-9p/tests/virtio-9p-test.c:597:fs_readdir: assertion failed (nentries == QTEST_V9FS_SYNTH_READDIR_NFILES + 2): (101 == 20002)

The server didn't hit the transport error...

> > > 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
> 
> My idea was to "handle it as an error" immediately when server receives a 
> T_version request with a "msize" argument transmitted by client that would be 
> way too small for anything.
> 
> Because if client sends T_version with msize < P9_IOHDRSZ then it is obvious 
> that this msize would be way too small for handling any subsequent 9p request 
> at all.
> 
> > > 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 ?
> 
> Exactly, that's why I added that comment to the commit log of this patch for 
> now to make this circumstance clear as yet TODO.
> 
> This issue with T_version and msize needs to be addressed anyway, because it 
> will popup again and again with various other request types and also with 
> transport aspects like previously discussed on a transport buffer size issue 
> (submitters of that transport patch on CC here for that reason):
> https://lists.gnu.org/archive/html/qemu-devel/2019-12/msg04701.html
> 
> The required patch to address this overall minimum msize issue would be very 
> small: just comparing client's transmitted "msize" parameter of client's 
> T_version request and if that transmitted msize is smaller than a certain 
> absolute minimum msize then raise an error immediately to prevent the session 
> to start.
> 
> But there are some decisions to be made, that's why I haven't provided a patch 
> for this min. msize issue in this patch series yet:
> 
> 1. What is the minimum msize to be allowed with T_version?
> 
>    P9_IOHDRSZ would be IMO too small, because P9_IOHDRSZ is the size of the 
>    common header portion of all IO requests. So it would rather make sense IMO
>    reviewing the protocol and pick the largest header size among all possible 
>    requests supported by 9pfs server ATM. The advantage of this approach would 
>    be that overall code would be easier too maintain, since we don't have to 
>    add minimum msize checks in any (or even all) individual request type 
>    handlers. T_version handler of server would already enforce a minimum msize 
>    and prevent the session if msize too small, and that's it.
> 
> 2. How to handle that error with T_version exactly?
> 
>    As far as I can see it, it was originally not considered that T_version 
>    might react with an error response at all. The specs are ambiguous about 
>    this topic. All you can find is that if the transmitted protocol version
>    is not supported by server, then server should respond with "unknown" with 
>    its R_version response, but should not respond with R_error in that case.
> 
>    The specs do not prohibit R_error for T_version in general, but I could 
>    imagine that some clients might not expect if we would send R_error. On the 
>    other hand responding with R_version "unknown" would not be appropriate for 
>    this msize error either, because that would mean to the client that the 
>    protocol version was not supported, which is not the case. So it would 
>    leave client uninformed about the actual reason/cause of the error.
> 
> 3. Are there any undocumented special msizes, e.g. msize=0 for "don't care" ?
> 
>    Probably not, but you might have seen more client implementations than me.
> 
> Best regards,
> Christian Schoenebeck
> 
>