Re: [Xen-devel] [PATCH for-4.12 RFC] xen/console: Handle NUL character in buffer sent via CONSOLEIO_write

[Julien Grall <julien.grall@xxxxxxx>]

Hi Wei,

On 2/27/19 12:55 PM, Wei Liu wrote:
> On Tue, Feb 26, 2019 at 11:03:51PM +0000, Julien Grall wrote:
> > After upgrading Debian to Buster, I started noticing console mangling
> > when using zsh. This is happenning because output sent by zsh to the
> > console may contain NUL character in the middle of the buffer.
> >
> > Linux is sending the buffer as it is to Xen console via CONSOLEIO_write.
> > However, the implementation in Xen considers NUL character is used to
> > terminate the buffer and therefore will ignore anything after it.
> >
> > The actual documentation of CONSOLEIO_write is pretty limited. From the
> > declaration, the hypercall takes a buffer and size. So this could lead
> > to think the NUL character is allowed in the middle of the buffer.
> >
> > This patch updates the console API to pass the size along the buffer
> > down so we can remove the reliance on buffer terminating by a NUL
> > character.
> >
> > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> >
> > ---
> [...]
> > @@ -527,7 +527,7 @@ static inline void xen_console_write_debug_port(const char 
> > *buf, size_t len)
> >   static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int 
> > count)
> >   {
> >       char kbuf[128];
> > -    int kcount = 0;
> > +    unsigned int kcount = 0;
> >       struct domain *cd = current->domain;
> >   
> >       while ( count > 0 )
> > @@ -547,8 +547,8 @@ static long 
> > guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count)
> >               /* Use direct console output as it could be interactive */
> >               spin_lock_irq(&console_lock);
> >   
> > -            sercon_puts(kbuf);
> > -            video_puts(kbuf);
> > +            sercon_puts(kbuf, kcount);
> > +            video_puts(kbuf, kcount);
> >   
> I think you missed the non-hwdom branch in the same function. It still
> strips non-printable characters.
Good point. The non-printable characters was added by Daniel in commit 
48d50de8e0 " console: buffer and show origin of guest PV writes" without 
much explanation.
The only reason I can see is, as we buffer the guest writes, the console 
would be screwed if we split an escape sequence. Furthermore, for guest 
output, we will always append "(dX)" to the output. So I am not entirely 
sure what to do in the non-hwdom case.
Any opinions?

> >   int console_suspend(void)
> [...]
> > diff --git a/xen/drivers/char/consoled.c b/xen/drivers/char/consoled.c
> > index 552abf5766..3e849a2557 100644
> > --- a/xen/drivers/char/consoled.c
> > +++ b/xen/drivers/char/consoled.c
> > @@ -77,7 +77,7 @@ size_t consoled_guest_rx(void)
> >   
> >           if ( idx >= BUF_SZ )
> >           {
> > -            pv_console_puts(buf);
> > +            pv_console_puts(buf, BUF_SZ);
> >               idx = 0;
> >           }
> >       }
> > @@ -85,7 +85,7 @@ size_t consoled_guest_rx(void)
> >       if ( idx )
> >       {
> >           buf[idx] = '\0';
> Can this be deleted? Now that you've explicitly sized the buffer.
We probably can delete a few of the '\0' over the console code. I will 
have a look at it.
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel