[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 2/9] 9pfs: validate count sent by client with T_readdir
From: |
Greg Kurz |
Subject: |
Re: [PATCH v2 2/9] 9pfs: validate count sent by client with T_readdir |
Date: |
Mon, 6 Jan 2020 18:49:10 +0100 |
On Mon, 06 Jan 2020 16:10:28 +0100
Christian Schoenebeck <address@hidden> wrote:
> On Montag, 6. Januar 2020 13:30:24 CET Greg Kurz wrote:
> > On Wed, 18 Dec 2019 14:17:59 +0100
> >
> > Christian Schoenebeck <address@hidden> 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 <address@hidden>
> > > ---
> > >
> > > 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
>
>