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

[Ruihan Li]

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.

> 
> 
> > 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