Re: [PATCH] char-mux: Don't overwrite the receive buffer

[Ruihan Li]

Hi,

On Mon, Jul 08, 2024 at 03:21:58PM GMT, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Jul 8, 2024 at 12:12 AM Ruihan Li <lrh2000@pku.edu.cn> wrote:
> 
> > Hi,
> >
> > Thanks for your quick review!
> >
> > On Sun, Jul 07, 2024 at 08:28:50PM GMT, Marc-André Lureau wrote:
> > > Hi
> > >
> > > On Sun, Jul 7, 2024 at 3:26 PM Ruihan Li <lrh2000@pku.edu.cn> wrote:
> > >
> > > > This commit fixes a bug that causes incorrect results when pasting more
> > > > than 32 bytes, the size of the receive buffer b->buffer, into the
> > virtio
> > > > console.
> > > >
> > > > Example (note that the last 32 bytes are always correct, but something
> > > > goes wrong just before the last 32 bytes):
> > > >
> > > >         Pasting
> > > >
> > abcdefghijklmnopqrstuvwxyz1234567890ABCDEFGHIJKLMNOPQRSTUVWXYZ!@#$%^&*()
> > > >         Received
> > > >
> > abcdefg)EFGHIJKLMNOPQRSTUVWXYZ!@#$%^&*()EFGHIJKLMNOPQRSTUVWXYZ!@#$%^&*()
> > > >
> > > > The root cause of this bug is as follows:
> > > >
> > > > The mux_chr_read function passes the data to the backend via
> > > > be->chr_read one byte at a time, either directly or via another
> > > > mux_chr_accept_input method. However, if the receive buffer is full,
> > > > there is a chance that the mux_chr_can_read method will return more
> > than
> > > > one byte, because in this case the method directly returns whatever
> > > > be->chr_can_read returns.
> > > >
> > > > This is problematic because if mux_chr_read passes a byte to the
> > backend
> > > > by calling be->chr_read, it will consume the entire backend buffer, at
> > > > least in the case of virtio. Once all backend buffers are used,
> > > > mux_chr_read writes all remaining bytes to the receive buffer
> > d->buffer,
> > > >
> > >
> > > My understanding of the code execution is:
> > > - mux_chr_can_read() returns be->chr_can_read(), say N, because d->buffer
> > > is already MUX_BUFFER_SIZE.
> > > - mux_chr_read() is called with N bytes
> > > - mux_chr_accept_input() flushes d->buffer, writing MUX_BUFFER_SIZE
> > > - be should still accept N-MUX_BUFFER_SIZE
> > > - mux_proc_byte() loops for N bytes
> > > - chr_read() should accept the N-MUX_BUFFER_SIZE
> > > - d->buffer is then filled with the remaining MUX_BUFFER_SIZE
> >
> > Note this:
> >         [..] if mux_chr_read passes a byte to the backend by calling
> >         be->chr_read, it will consume the entire backend buffer, at
> >         least in the case of virtio [..]
> >
> > At least in the case of virtio, if the guest provides a buffer of length
> > 4096, be->chr_can_read will report 4096. But if you then call
> > be->chr_read with one byte, the whole 4096 buffer will be used. After
> > that, be->chr_can_read will return zero instead of 4095.
> >
> > This should make sense since the device cannot change the number of
> > bytes in the buffer after it has made the buffer available to the CPU.
> >
> 
> Thanks, that helps explaining the incorrect behaviour.
> 
> I think the concept of extra buffer as introduced in commit
> bd9bdce694ccb76facc882363e4c337e8a88c918 ("Add input buffer to mux chr
> (patch by Tristan Gingold)") is flawed, as Jan Kiszka explained in commit
> a80bf99fa3dd829ecea88b9bfb4f7cf146208f07 ("char-mux: Use separate input
> buffers (Jan Kiszka)"):
>     Note: In contrast to the original author's claim, the buffering concept
>     still breaks down when the fifo of the currently active sub-device is
>     full. As we cannot accept futher data from this point on without risking
>     to loose it, we will also miss escape sequences, just like without all
>     that buffering. In short: There is no reliable escape sequence handling
>     without infinite buffers or the risk of loosing some data.
> 
> Maybe the best course is to remove the cycle buffer and either:
> - drop the data that be can't accept, but have always responsive mux (by
> default)
> - blocking, including mux, until the be can accept more data (not friendly)
> - or allow unlimited buffering?
> 
> Given that mux is meant for developers and qemu CLI users, I guess any of
> this would be acceptable.

Thanks for your comments.

However, I'm not really sure what you're talking about. If we make
mux_chr_can_read return either zero or one (as I've done in the patch),
do you mean that we are still at risk of losing some escape sequences?

In mux_proc_byte, we set d->term_got_escape to 1 when we see the escape
character. As far as I can see, the escape sequence is always handled
correctly. So I don't understand how losing escape sequences can happen.

Would you mind explaining this in more detail?

> 
> 
> >
> > >
> > >
> > > > but the number of remaining bytes can be larger than the buffer size.
> > > >
> > >
> > > By the above description, I don't see how it happens.
> > >
> > > This does not lead to security problems since it is a ring buffer, but
> > > > it does mess up the receive data.
> > > >
> > > > This can be fixed by having mux_chr_can_read return either zero or one.
> > > > This fix is not very efficient, but it is quite reasonable since
> > > > mux_chr_read also passes the data to the backend one byte at a time.
> > > >
> > >
> > > Could you share your testing setup? Even better if you could write a
> > test!
> >
> > This happens in https://github.com/asterinas/asterinas. Sorry, but I
> > don't have a minimal reproducible example, and I don't think I can make
> > one anytime soon.
> >
> > As for the tests, I'd like to know how to write such tests in QEMU. I
> > checked the documentation but didn't find anything, maybe I'm missing
> > something?
> >
> > >
> > >
> > > thanks
> > >
> > >
> > > > Signed-off-by: Ruihan Li <lrh2000@pku.edu.cn>
> > > > ---
> > > >  chardev/char-mux.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/chardev/char-mux.c b/chardev/char-mux.c
> > > > index ee2d47b..5c6eea2 100644
> > > > --- a/chardev/char-mux.c
> > > > +++ b/chardev/char-mux.c
> > > > @@ -210,8 +210,8 @@ static int mux_chr_can_read(void *opaque)
> > > >          return 1;
> > > >      }
> > > >
> > > > -    if (be && be->chr_can_read) {
> > > > -        return be->chr_can_read(be->opaque);
> > > > +    if (be && be->chr_can_read && be->chr_can_read(be->opaque)) {
> > > > +        return 1;
> > > >      }
> > > >
> > > >      return 0;
> > > > --
> > > > 2.45.2
> > > >
> > > >
> > > >
> > >
> > > --
> > > Marc-André Lureau
> >
> > Thanks,
> > Ruihan Li
> >
> >
> 
> -- 
> Marc-André Lureau

Thanks,
Ruihan Li