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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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?

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.

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

> One thing I didn't consider that I probably should have is that
> isprint() in the hypervisor is not a UTF-8 aware check, so it will end
> up corrupting characters if your guests treat the console as having that
> encoding.
> 
>> 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?
> 
> This really depends on the purpose of the console in the system.  Since
> there's usually possible hypervisor messages in the output, it makes
> sense to me to treat it as a line-based log containing readable text. 
> However, the ability for the hardware domain to use it interactively is
> also important for debugging, and limiting or buffering that domain's
> output would interfere with that.  The current handling of the output is
> a compromise between these two uses.

Right; the system console is a shared resource, and the hypervisor has
to  make sure that unprivileged guests don't do anything to muck it up.
 As you say, in the general case they shouldn't be able to insert lines
or clear the screen or things like that.

If for specific setups it's required for one particular guest to have
that kind of access, we should add in an interface to grant that ability
to specific guests.

 -George

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