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,

On 04/03/2019 11:21, George Dunlap wrote:
> On 3/1/19 10:59 PM, Daniel De Graaf wrote:
> > On 2/27/19 1:45 PM, Julien Grall wrote:
> > > 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.
> zsh is sending a NUL character to the console in the middle of the
> buffer?  Why would it do that?  Is that defined behavior, or does it
> just happen to work because Xen is the first console device to act
> strange as a result?
I have no idea why new version of ZSH is adding '\0' in the buffer. But, to be 
honest, it does not matter to know it.
What matters is an application using the POSIX call write() is free to put a 
'\0' in the middle of the stream. It is up to the reader (i.e screen/gdb...) to 
decide how to interpret the characters.
> There's an argument to be made that 1) zsh shouldn't be sending NUL
> characters, and/or that 2) Linux should be the one 'sanitizing' input it
> sends to the console.
Your arguments seems to be based on the assumption the console is only used by a 
human. Console can be used for other purpose, such as communication with an 
external debugger. So how do you decide what to sanitize?
IHMO, both Xen and Linux should just pass all the characters to the other end. 
This is inline with the semantics of the posix call write().
> > > > > @@ -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.
> > Yes, I added stripping of non-printable characters because escape
> > sequences printed out by some guests (in particular, clear screen
> > sequences printed out by some distro's early boot scripts) interfered
> > with the output of other guests.  It also prevents guests from
> > pretending to be one another or the hypervisor, if the console is being
> > used for some kind of auditing or logging.
> It sounds like it would be useful to add a comment to that effect on the
> non-hwdomain path, to make sure things don't accidentally get removed.
I have a patch to document the behavior.

Cheers,

--
Julien Grall

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