Re: [Xen-devel] [PATCH 2/4] xen/console: Don't treat NUL character as the end of the buffer

From: Julien Grall <julien.grall@arm.com>
To: Stefano Stabellini <sstabellini@kernel.org>
Cc: Wei Liu <wei.liu2@citrix.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>,
	Jan Beulich <jbeulich@suse.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [Xen-devel] [PATCH 2/4] xen/console: Don't treat NUL character as the end of the buffer
Date: Mon, 5 Aug 2019 12:40:04 +0100	[thread overview]
Message-ID: <6b9f3353-61a0-cfa0-657b-400451966ed5@arm.com> (raw)
In-Reply-To: <alpine.DEB.2.10.1904161245000.1370@sstabellini-ThinkPad-X260>

Hi Stefano,

On 16/04/2019 21:33, Stefano Stabellini wrote:
> On Tue, 2 Apr 2019, Julien Grall wrote:
>> After upgrading Debian to Buster, I have began to notice console
>> mangling when using zsh in Dom0. This is happenning because output sent by
>> zsh to the console may contain NULs in the middle of the buffer.
>>
>> The actual implementation of CONSOLEIO_write considers that a buffer
>> always terminate with a NUL and therefore will ignore anything after it.
>>
>> In general, NULs are perfectly legitimate in terminal streams. For
>> instance, this could be used for padding slow terminals. See terminfo(5)
>> section `Delays and Padding`, or search for the pcre '\bpad'.
>>
>> Other use cases includes using the console for dumping non-human
>> readable information (e.g debugger, file if no network...). With the
>> current behavior, the resulting stream will end up to be corrupted.
>>
>> The documentation for CONSOLEIO_write is pretty limited (to not say
>> inexistent). 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@arm.com>
>>
>> ---
>>
>> This patch was originally sent standalone [1]. But the series grows to
>> include another bug found in the console code and documentation.
>>
>> Change since the standalone version:
>>      - Fix early printk on Arm
>>      - Fix gdbstub
>>      - Remove unecessary leading NUL character added by Xen
>>      - Handle DomU console
>>      - Rework the commit message
>>
>> Below a small C program to repro the bug on Xen:
>>
>> int main(void)
>> {
>>      write(1,
>>            "\r\33[0m\0\0\0\0\0\0\0\0\33[27m\33[24m\33[j\33[32mjulien\33[31m@\33[00m\33[36mjuno2-julieng:~\33[37m>",
>>            75);
>>      write(1,
>>            "\33[K\33[32C\33[01;33m--juno2-julieng-13:44--\33[00m\33[37m\33[55D",
>>            54);
>>      write(1, "\33[?2004h", 8);
>>
>>      return 0;
>> }
>>
>> Without this patch, the only --juno2-julieng-13:44-- will be printed in
>> yellow.
>>
>> This patch was tested on Arm using serial console. I am not entirely
>> sure whether the video and PV console is correct. I would appreciate help
>> for testing here.
>>
>> [1] https://lists.xenproject.org/archives/html/xen-devel/2019-02/msg01932.html
>> ---
>>   xen/arch/arm/early_printk.c       |  5 ++--
>>   xen/common/gdbstub.c              |  6 ++--
>>   xen/drivers/char/console.c        | 58 +++++++++++++++++++--------------------
>>   xen/drivers/char/consoled.c       |  7 ++---
>>   xen/drivers/char/serial.c         |  8 ++++--
>>   xen/drivers/char/xen_pv_console.c | 10 +++----
>>   xen/drivers/video/lfb.c           | 14 ++++++----
>>   xen/drivers/video/lfb.h           |  4 +--
>>   xen/drivers/video/vga.c           | 14 ++++++----
>>   xen/include/xen/console.h         |  2 +-
>>   xen/include/xen/early_printk.h    |  2 +-
>>   xen/include/xen/pv_console.h      |  4 +--
>>   xen/include/xen/serial.h          |  4 +--
>>   xen/include/xen/video.h           |  4 +--
>>   14 files changed, 73 insertions(+), 69 deletions(-)
>>
>> diff --git a/xen/arch/arm/early_printk.c b/xen/arch/arm/early_printk.c
>> index 97466a12b1..35a47c7229 100644
>> --- a/xen/arch/arm/early_printk.c
>> +++ b/xen/arch/arm/early_printk.c
>> @@ -17,9 +17,10 @@
>>   void early_putch(char c);
>>   void early_flush(void);
>>   
>> -void early_puts(const char *s)
>> +void early_puts(const char *s, unsigned int nr)
>>   {
>> -    while (*s != '\0') {
>> +    while ( nr-- > 0 )
>> +    {
>>           if (*s == '\n')
>>               early_putch('\r');
>>           early_putch(*s);
>> diff --git a/xen/common/gdbstub.c b/xen/common/gdbstub.c
>> index 07095e1ec7..08a4dda9f3 100644
>> --- a/xen/common/gdbstub.c
>> +++ b/xen/common/gdbstub.c
>> @@ -68,7 +68,7 @@ static void gdb_smp_resume(void);
>>   static char __initdata opt_gdb[30];
>>   string_param("gdb", opt_gdb);
>>   
>> -static void gdbstub_console_puts(const char *str);
>> +static void gdbstub_console_puts(const char *str, unsigned int nr);
>>   
>>   /* value <-> char (de)serialzers */
>>   static char
>> @@ -546,14 +546,14 @@ __gdb_ctx = {
>>   static struct gdb_context *gdb_ctx = &__gdb_ctx;
>>   
>>   static void
>> -gdbstub_console_puts(const char *str)
>> +gdbstub_console_puts(const char *str, unsigned int nr)
>>   {
>>       const char *p;
>>   
>>       gdb_start_packet(gdb_ctx);
>>       gdb_write_to_packet_char('O', gdb_ctx);
>>   
>> -    for ( p = str; *p != '\0'; p++ )
>> +    for ( p = str; nr > 0; p++, nr-- )
>>       {
>>           gdb_write_to_packet_char(hex2char((*p>>4) & 0x0f), gdb_ctx );
>>           gdb_write_to_packet_char(hex2char((*p) & 0x0f), gdb_ctx );
>> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
>> index 9bbcb0f57a..b119bf980b 100644
>> --- a/xen/drivers/char/console.c
>> +++ b/xen/drivers/char/console.c
>> @@ -325,9 +325,9 @@ long read_console_ring(struct xen_sysctl_readconsole *op)
>>   static char serial_rx_ring[SERIAL_RX_SIZE];
>>   static unsigned int serial_rx_cons, serial_rx_prod;
>>   
>> -static void (*serial_steal_fn)(const char *) = early_puts;
>> +static void (*serial_steal_fn)(const char *, unsigned int nr) = early_puts;
>>   
>> -int console_steal(int handle, void (*fn)(const char *))
>> +int console_steal(int handle, void (*fn)(const char *, unsigned int nr))
>>   {
>>       if ( (handle == -1) || (handle != sercon_handle) )
>>           return 0;
>> @@ -345,15 +345,15 @@ void console_giveback(int id)
>>           serial_steal_fn = NULL;
>>   }
>>   
>> -static void sercon_puts(const char *s)
>> +static void sercon_puts(const char *s, unsigned int nr)
>>   {
>>       if ( serial_steal_fn != NULL )
>> -        (*serial_steal_fn)(s);
>> +        (*serial_steal_fn)(s, nr);
>>       else
>> -        serial_puts(sercon_handle, s);
>> +        serial_puts(sercon_handle, s, nr);
>>   
>>       /* Copy all serial output into PV console */
>> -    pv_console_puts(s);
>> +    pv_console_puts(s, nr);
>>   }
>>   
>>   static void dump_console_ring_key(unsigned char key)
>> @@ -388,8 +388,8 @@ static void dump_console_ring_key(unsigned char key)
>>       }
>>       buf[sofar] = '\0';
>>   
>> -    sercon_puts(buf);
>> -    video_puts(buf);
>> +    sercon_puts(buf, sofar);
>> +    video_puts(buf, sofar);
>>   
>>       free_xenheap_pages(buf, order);
>>   }
>> @@ -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 )
>> @@ -540,25 +540,22 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count)
>>           kcount = min_t(int, count, sizeof(kbuf)-1);
>>           if ( copy_from_guest(kbuf, buffer, kcount) )
>>               return -EFAULT;
>> -        kbuf[kcount] = '\0';
>>   
>>           if ( is_hardware_domain(cd) )
>>           {
>>               /* 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);
>>   
>>   #ifdef CONFIG_X86
>>               if ( opt_console_xen )
>>               {
>> -                size_t len = strlen(kbuf);
>> -
>>                   if ( xen_guest )
>> -                    xen_hypercall_console_write(kbuf, len);
>> +                    xen_hypercall_console_write(kbuf, kcount);
>>                   else
>> -                    xen_console_write_debug_port(kbuf, len);
>> +                    xen_console_write_debug_port(kbuf, kcount);
>>               }
>>   #endif
>>   
>> @@ -575,19 +572,20 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count)
>>               char *kin = kbuf, *kout = kbuf, c;
>>   
>>               /* Strip non-printable characters */
>> -            for ( ; ; )
>> +            do
>>               {
>>                   c = *kin++;
>> -                if ( c == '\0' || c == '\n' )
>> +                if ( c == '\n' )
>>                       break;
>>                   if ( isprint(c) || c == '\t' )
>>                       *kout++ = c;
>> -            }
>> +            } while ( --kcount > 0 );
> 
> Why not
> 
>      while ( kcount-- > 0 )
>      {
>         XXX
>      }
>    
> like you did in early_puts? Anyway this should be just style, so up to
> you.

This is not a style issue but a compilation problem. The variable c is used 
after the loop to check if it a newline (i.e \n). The variable is only set 
within the loop. With a while () the compiler thinks the variable may never be 
initialized.

The do {} while fixes the problem by always executing one iteration. This is 
fine  because count is kcount will always be > 0.

Alternatively, the variable can be initialized to a constant. But I feels this 
is may hide other problem in the code in the future.

Cheers,

-- 
Julien Grall

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