[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On Mon, 5 Aug 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@xxxxxxx>
> 
> ---
> 
> This patch was originally sent standalone [1]. But the series grows to
> include another bug found in the console code and documentation.
> 
> Cnhanges in v2:
>     - Switch from unsigned int to size_t. So truncation is avoided. We
>     can decide whether we want explicit truncation later on.
>     - Remove unecessary leading NUL added in dump_console_ring_key
>     - Remove unecessary decoration in sercon_puts
>     - Fix loop in lfb_scroll_puts
>     - use while() rather than do {} while()
> 
> 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

Acked-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


> ---
>  xen/arch/arm/early_printk.c       |  5 ++--
>  xen/common/gdbstub.c              |  6 ++--
>  xen/drivers/char/console.c        | 59 
> +++++++++++++++++++--------------------
>  xen/drivers/char/consoled.c       |  7 ++---
>  xen/drivers/char/serial.c         |  7 +++--
>  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, 71 insertions(+), 71 deletions(-)
> 
> diff --git a/xen/arch/arm/early_printk.c b/xen/arch/arm/early_printk.c
> index 97466a12b1..333073d97e 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, size_t 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..6234834a20 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, size_t 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, size_t 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 d728e737d1..752a11f6c5 100644
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -326,9 +326,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 *, size_t nr) = early_puts;
>  
> -int console_steal(int handle, void (*fn)(const char *))
> +int console_steal(int handle, void (*fn)(const char *, size_t nr))
>  {
>      if ( (handle == -1) || (handle != sercon_handle) )
>          return 0;
> @@ -346,15 +346,15 @@ void console_giveback(int id)
>          serial_steal_fn = NULL;
>  }
>  
> -static void sercon_puts(const char *s)
> +static void sercon_puts(const char *s, size_t 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)
> @@ -387,10 +387,9 @@ static void dump_console_ring_key(unsigned char key)
>          sofar += len;
>          c += len;
>      }
> -    buf[sofar] = '\0';
>  
> -    sercon_puts(buf);
> -    video_puts(buf);
> +    sercon_puts(buf, sofar);
> +    video_puts(buf, sofar);
>  
>      free_xenheap_pages(buf, order);
>  }
> @@ -528,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 )
> @@ -541,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
>  
> @@ -576,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 );
> +
>              *kout = '\0';
>              spin_lock(&cd->pbuf_lock);
> +            kcount = kin - kbuf;
>              if ( c == '\n' )
>              {
> -                kcount = kin - kbuf;
>                  cd->pbuf[cd->pbuf_idx] = '\0';
>                  guest_printk(cd, XENLOG_G_DEBUG "%s%s\n", cd->pbuf, kbuf);
>                  cd->pbuf_idx = 0;
> @@ -667,16 +664,16 @@ static bool_t console_locks_busted;
>  
>  static void __putstr(const char *str)
>  {
> +    size_t len = strlen(str);
> +
>      ASSERT(spin_is_locked(&console_lock));
>  
> -    sercon_puts(str);
> -    video_puts(str);
> +    sercon_puts(str, len);
> +    video_puts(str, len);
>  
>  #ifdef CONFIG_X86
>      if ( opt_console_xen )
>      {
> -        size_t len = strlen(str);
> -
>          if ( xen_guest )
>              xen_hypercall_console_write(str, len);
>          else
> @@ -1250,6 +1247,7 @@ void debugtrace_printk(const char *fmt, ...)
>      char          cntbuf[24];
>      va_list       args;
>      unsigned long flags;
> +    unsigned int nr;
>  
>      if ( debugtrace_bytes == 0 )
>          return;
> @@ -1261,14 +1259,15 @@ void debugtrace_printk(const char *fmt, ...)
>      ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0);
>  
>      va_start(args, fmt);
> -    vsnprintf(buf, sizeof(buf), fmt, args);
> +    nr = vscnprintf(buf, sizeof(buf), fmt, args);
>      va_end(args);
>  
>      if ( debugtrace_send_to_console )
>      {
> -        snprintf(cntbuf, sizeof(cntbuf), "%u ", ++count);
> -        serial_puts(sercon_handle, cntbuf);
> -        serial_puts(sercon_handle, buf);
> +        unsigned int n = snprintf(cntbuf, sizeof(cntbuf), "%u ", ++count);
> +
> +        serial_puts(sercon_handle, cntbuf, n);
> +        serial_puts(sercon_handle, buf, nr);
>      }
>      else
>      {
> @@ -1381,7 +1380,7 @@ void panic(const char *fmt, ...)
>   * **************************************************************
>   */
>  
> -static void suspend_steal_fn(const char *str) { }
> +static void suspend_steal_fn(const char *str, size_t nr) { }
>  static int suspend_steal_id;
>  
>  int console_suspend(void)
> diff --git a/xen/drivers/char/consoled.c b/xen/drivers/char/consoled.c
> index 552abf5766..222e018442 100644
> --- a/xen/drivers/char/consoled.c
> +++ b/xen/drivers/char/consoled.c
> @@ -77,16 +77,13 @@ size_t consoled_guest_rx(void)
>  
>          if ( idx >= BUF_SZ )
>          {
> -            pv_console_puts(buf);
> +            pv_console_puts(buf, BUF_SZ);
>              idx = 0;
>          }
>      }
>  
>      if ( idx )
> -    {
> -        buf[idx] = '\0';
> -        pv_console_puts(buf);
> -    }
> +        pv_console_puts(buf, idx);
>  
>      /* No need for a mem barrier because every character was already 
> consumed */
>      barrier();
> diff --git a/xen/drivers/char/serial.c b/xen/drivers/char/serial.c
> index 221a14c092..88cd876790 100644
> --- a/xen/drivers/char/serial.c
> +++ b/xen/drivers/char/serial.c
> @@ -223,11 +223,10 @@ void serial_putc(int handle, char c)
>      spin_unlock_irqrestore(&port->tx_lock, flags);
>  }
>  
> -void serial_puts(int handle, const char *s)
> +void serial_puts(int handle, const char *s, size_t nr)
>  {
>      struct serial_port *port;
>      unsigned long flags;
> -    char c;
>  
>      if ( handle == -1 )
>          return;
> @@ -238,8 +237,10 @@ void serial_puts(int handle, const char *s)
>  
>      spin_lock_irqsave(&port->tx_lock, flags);
>  
> -    while ( (c = *s++) != '\0' )
> +    for ( ; nr > 0; nr--, s++ )
>      {
> +        char c = *s;
> +
>          if ( (c == '\n') && (handle & SERHND_COOKED) )
>              __serial_putc(port, '\r' | ((handle & SERHND_HI) ? 0x80 : 0x00));
>  
> diff --git a/xen/drivers/char/xen_pv_console.c 
> b/xen/drivers/char/xen_pv_console.c
> index cc1c1d743f..612784b074 100644
> --- a/xen/drivers/char/xen_pv_console.c
> +++ b/xen/drivers/char/xen_pv_console.c
> @@ -129,13 +129,13 @@ size_t pv_console_rx(struct cpu_user_regs *regs)
>      return recv;
>  }
>  
> -static size_t pv_ring_puts(const char *buf)
> +static size_t pv_ring_puts(const char *buf, size_t nr)
>  {
>      XENCONS_RING_IDX cons, prod;
>      size_t sent = 0, avail;
>      bool put_r = false;
>  
> -    while ( buf[sent] != '\0' || put_r )
> +    while ( sent < nr || put_r )
>      {
>          cons = ACCESS_ONCE(cons_ring->out_cons);
>          prod = cons_ring->out_prod;
> @@ -156,7 +156,7 @@ static size_t pv_ring_puts(const char *buf)
>              continue;
>          }
>  
> -        while ( avail && (buf[sent] != '\0' || put_r) )
> +        while ( avail && (sent < nr || put_r) )
>          {
>              if ( put_r )
>              {
> @@ -185,7 +185,7 @@ static size_t pv_ring_puts(const char *buf)
>      return sent;
>  }
>  
> -void pv_console_puts(const char *buf)
> +void pv_console_puts(const char *buf, size_t nr)
>  {
>      unsigned long flags;
>  
> @@ -193,7 +193,7 @@ void pv_console_puts(const char *buf)
>          return;
>  
>      spin_lock_irqsave(&tx_lock, flags);
> -    pv_ring_puts(buf);
> +    pv_ring_puts(buf, nr);
>      spin_unlock_irqrestore(&tx_lock, flags);
>  }
>  
> diff --git a/xen/drivers/video/lfb.c b/xen/drivers/video/lfb.c
> index 5022195ae5..75b749b330 100644
> --- a/xen/drivers/video/lfb.c
> +++ b/xen/drivers/video/lfb.c
> @@ -53,14 +53,15 @@ static void lfb_show_line(
>  }
>  
>  /* Fast mode which redraws all modified parts of a 2D text buffer. */
> -void lfb_redraw_puts(const char *s)
> +void lfb_redraw_puts(const char *s, size_t nr)
>  {
>      unsigned int i, min_redraw_y = lfb.ypos;
> -    char c;
>  
>      /* Paste characters into text buffer. */
> -    while ( (c = *s++) != '\0' )
> +    for ( ; nr > 0; nr--, s++ )
>      {
> +        char c = *s;
> +
>          if ( (c == '\n') || (lfb.xpos >= lfb.lfbp.text_columns) )
>          {
>              if ( ++lfb.ypos >= lfb.lfbp.text_rows )
> @@ -97,13 +98,14 @@ void lfb_redraw_puts(const char *s)
>  }
>  
>  /* Slower line-based scroll mode which interacts better with dom0. */
> -void lfb_scroll_puts(const char *s)
> +void lfb_scroll_puts(const char *s, size_t nr)
>  {
>      unsigned int i;
> -    char c;
>  
> -    while ( (c = *s++) != '\0' )
> +    for ( ; nr > 0; nr--, s++ )
>      {
> +        char c = *s;
> +
>          if ( (c == '\n') || (lfb.xpos >= lfb.lfbp.text_columns) )
>          {
>              unsigned int bytes = (lfb.lfbp.width *
> diff --git a/xen/drivers/video/lfb.h b/xen/drivers/video/lfb.h
> index ac40a66379..e743ccdd6b 100644
> --- a/xen/drivers/video/lfb.h
> +++ b/xen/drivers/video/lfb.h
> @@ -35,8 +35,8 @@ struct lfb_prop {
>      unsigned int text_rows;
>  };
>  
> -void lfb_redraw_puts(const char *s);
> -void lfb_scroll_puts(const char *s);
> +void lfb_redraw_puts(const char *s, size_t nr);
> +void lfb_scroll_puts(const char *s, size_t nr);
>  void lfb_carriage_return(void);
>  void lfb_free(void);
>  
> diff --git a/xen/drivers/video/vga.c b/xen/drivers/video/vga.c
> index 454457ade8..666f2e2509 100644
> --- a/xen/drivers/video/vga.c
> +++ b/xen/drivers/video/vga.c
> @@ -18,9 +18,9 @@ static int vgacon_keep;
>  static unsigned int xpos, ypos;
>  static unsigned char *video;
>  
> -static void vga_text_puts(const char *s);
> -static void vga_noop_puts(const char *s) {}
> -void (*video_puts)(const char *) = vga_noop_puts;
> +static void vga_text_puts(const char *s, size_t nr);
> +static void vga_noop_puts(const char *s, size_t nr) {}
> +void (*video_puts)(const char *, size_t nr) = vga_noop_puts;
>  
>  /*
>   * 'vga=<mode-specifier>[,keep]' where <mode-specifier> is one of:
> @@ -174,12 +174,12 @@ void __init video_endboot(void)
>      }
>  }
>  
> -static void vga_text_puts(const char *s)
> +static void vga_text_puts(const char *s, size_t nr)
>  {
> -    char c;
> -
> -    while ( (c = *s++) != '\0' )
> +    for ( ; nr > 0; nr--, s++ )
>      {
> +        char c = *s;
> +
>          if ( (c == '\n') || (xpos >= columns) )
>          {
>              if ( ++ypos >= lines )
> diff --git a/xen/include/xen/console.h b/xen/include/xen/console.h
> index b4f9463936..ba914f9e5b 100644
> --- a/xen/include/xen/console.h
> +++ b/xen/include/xen/console.h
> @@ -38,7 +38,7 @@ struct domain *console_input_domain(void);
>   * Steal output from the console. Returns +ve identifier, else -ve error.
>   * Takes the handle of the serial line to steal, and steal callback function.
>   */
> -int console_steal(int handle, void (*fn)(const char *));
> +int console_steal(int handle, void (*fn)(const char *, size_t nr));
>  
>  /* Give back stolen console. Takes the identifier returned by console_steal. 
> */
>  void console_giveback(int id);
> diff --git a/xen/include/xen/early_printk.h b/xen/include/xen/early_printk.h
> index 2c3e1b3519..0f76c3a74f 100644
> --- a/xen/include/xen/early_printk.h
> +++ b/xen/include/xen/early_printk.h
> @@ -5,7 +5,7 @@
>  #define __XEN_EARLY_PRINTK_H__
>  
>  #ifdef CONFIG_EARLY_PRINTK
> -void early_puts(const char *s);
> +void early_puts(const char *s, size_t nr);
>  #else
>  #define early_puts NULL
>  #endif
> diff --git a/xen/include/xen/pv_console.h b/xen/include/xen/pv_console.h
> index cb92539666..4745f46f2d 100644
> --- a/xen/include/xen/pv_console.h
> +++ b/xen/include/xen/pv_console.h
> @@ -8,7 +8,7 @@
>  void pv_console_init(void);
>  void pv_console_set_rx_handler(serial_rx_fn fn);
>  void pv_console_init_postirq(void);
> -void pv_console_puts(const char *buf);
> +void pv_console_puts(const char *buf, size_t nr);
>  size_t pv_console_rx(struct cpu_user_regs *regs);
>  evtchn_port_t pv_console_evtchn(void);
>  
> @@ -17,7 +17,7 @@ evtchn_port_t pv_console_evtchn(void);
>  static inline void pv_console_init(void) {}
>  static inline void pv_console_set_rx_handler(serial_rx_fn fn) { }
>  static inline void pv_console_init_postirq(void) { }
> -static inline void pv_console_puts(const char *buf) { }
> +static inline void pv_console_puts(const char *buf, size_t nr) { }
>  static inline size_t pv_console_rx(struct cpu_user_regs *regs) { return 0; }
>  evtchn_port_t pv_console_evtchn(void)
>  {
> diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h
> index f2994d4093..6548f0b0a9 100644
> --- a/xen/include/xen/serial.h
> +++ b/xen/include/xen/serial.h
> @@ -114,8 +114,8 @@ int serial_parse_handle(char *conf);
>  /* Transmit a single character via the specified COM port. */
>  void serial_putc(int handle, char c);
>  
> -/* Transmit a NULL-terminated string via the specified COM port. */
> -void serial_puts(int handle, const char *s);
> +/* Transmit a string via the specified COM port. */
> +void serial_puts(int handle, const char *s, size_t nr);
>  
>  /*
>   * An alternative to registering a character-receive hook. This function
> diff --git a/xen/include/xen/video.h b/xen/include/xen/video.h
> index 2e897f9df5..905f331112 100644
> --- a/xen/include/xen/video.h
> +++ b/xen/include/xen/video.h
> @@ -13,11 +13,11 @@
>  
>  #ifdef CONFIG_VIDEO
>  void video_init(void);
> -extern void (*video_puts)(const char *);
> +extern void (*video_puts)(const char *, size_t nr);
>  void video_endboot(void);
>  #else
>  #define video_init()    ((void)0)
> -#define video_puts(s)   ((void)0)
> +#define video_puts(s, nr)   ((void)0)
>  #define video_endboot() ((void)0)
>  #endif
>  
> -- 
> 2.11.0
> 

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.