On Mon, 06 Jan 2020 16:10:28 +0100 Christian Schoenebeck <qemu_...@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_...@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_...@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 > >