[Xen-devel] [PATCH v2 0/4] xen/console: Bug fixes and doc improvement

[Xen-devel] [PATCH v2 0/4] xen/console: Bug fixes and doc improvement

[Julien Grall]

Hi all,

This series contains a bunch of bug fixes for the hypercall CONSOLEIO_write
and some documentation.

Cheers,

Julien Grall (4):
  xen/console: Don't treat NUL character as the end of the buffer
  xen/console: Rework HYPERCALL_console_io interface
  xen/public: Document HYPERCALL_console_io()
  xen/console: Simplify domU console handling in guest_console_write

 xen/arch/arm/early_printk.c       |  5 ++-
 xen/common/gdbstub.c              |  6 +--
 xen/drivers/char/console.c        | 83 +++++++++++++++++++++------------------
 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/public/xen.h          | 24 ++++++++++-
 xen/include/xen/console.h         |  2 +-
 xen/include/xen/early_printk.h    |  2 +-
 xen/include/xen/hypercall.h       |  4 +-
 xen/include/xen/pv_console.h      |  4 +-
 xen/include/xen/serial.h          |  4 +-
 xen/include/xen/video.h           |  4 +-
 16 files changed, 111 insertions(+), 83 deletions(-)

-- 
2.11.0


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

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

[Julien Grall]

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.

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
---
 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 2/4] xen/console: Rework HYPERCALL_console_io interface

[Julien Grall]

At the moment, HYPERCALL_console_io is using signed int to describe the
command (@cmd) and the size of the buffer (@count).
    * @cmd does not need to be signed this used as a set of named value.
    None of them are negative. If new one are introduced they can be
    positive.
    * @count is used to know the size of the buffer. It makes little
    sense to have a negative value here.

So both variables are now switched to use unsigned int.

Changing @count to unsigned type will result in a change of behavior for
the existing commands:
    - write: Any buffer bigger than 2GB will now be printed rather than
      been ignored (the command return 0).
    - read: The return value is a signed 32-bit value for 32-bit Xen.
      To keep compatibility between 32-bit and 64-bit ABI, it
      effectively means the return value is 32-bit (despite been long
      on 64-bit). Negative value are used for error and positive value
      for the number of characters read. To avoid clash between the two
      sets, the buffer is still limited to 2GB. The only difference is
      an error is returned rather than claiming there are no characters.

The behavior is only affecting unlikely use of the current interface, so
this is not a big concern regarding backward compatibility.

Signed-off-by: Julien Grall <julien.grall@arm.com>

---
    This will be documented in the public interface in the next patch.

    Changes in v2:
        - Patch added
---
 xen/drivers/char/console.c  | 15 +++++++++++++--
 xen/include/xen/hypercall.h |  4 ++--
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 752a11f6c5..fa8f5cff4b 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -524,7 +524,8 @@ static inline void xen_console_write_debug_port(const char *buf, size_t len)
 }
 #endif
 
-static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count)
+static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
+                                unsigned int count)
 {
     char kbuf[128];
     unsigned int kcount = 0;
@@ -612,7 +613,8 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count)
     return 0;
 }
 
-long do_console_io(int cmd, int count, XEN_GUEST_HANDLE_PARAM(char) buffer)
+long do_console_io(unsigned int cmd, unsigned int count,
+                   XEN_GUEST_HANDLE_PARAM(char) buffer)
 {
     long rc;
     unsigned int idx, len;
@@ -627,6 +629,15 @@ long do_console_io(int cmd, int count, XEN_GUEST_HANDLE_PARAM(char) buffer)
         rc = guest_console_write(buffer, count);
         break;
     case CONSOLEIO_read:
+        /*
+         * The return value is either the number of characters read or
+         * a negative value in case of error. So we need to prevent
+         * overlap between the two sets.
+         */
+        rc = -E2BIG;
+        if ( (int)count < 0 )
+            break;
+
         rc = 0;
         while ( (serial_rx_cons != serial_rx_prod) && (rc < count) )
         {
diff --git a/xen/include/xen/hypercall.h b/xen/include/xen/hypercall.h
index fc00a67448..ad8ad27b23 100644
--- a/xen/include/xen/hypercall.h
+++ b/xen/include/xen/hypercall.h
@@ -83,8 +83,8 @@ do_xen_version(
 
 extern long
 do_console_io(
-    int cmd,
-    int count,
+    unsigned int cmd,
+    unsigned int count,
     XEN_GUEST_HANDLE_PARAM(char) buffer);
 
 extern long
-- 
2.11.0


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

[Xen-devel] [PATCH v2 3/4] xen/public: Document HYPERCALL_console_io()

[Julien Grall]

Currently, OS developpers will have to look at Xen code in order to know
the parameters of an hypercall and how it is meant to work.

This is not a trivial task as you may need to have a deep understanding
of Xen internal.

This patch attempts to document the behavior of HYPERCALL_console_io() to
help OS developer.

Signed-off-by: Julien Grall <julien.grall@arm.com>

---
    Changes in v2:
        - Follow the style of other comments within the file
        - Use @return to make return value
        - Add a sentence regarding the buffer size
---
 xen/include/public/xen.h | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index cb2917e74b..742ab71004 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -486,7 +486,29 @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t);
 /* ` } */
 
 /*
- * Commands to HYPERVISOR_console_io().
+ * ` int
+ * ` HYPERVISOR_console_io(unsigned int cmd,
+ * `                       unsigned int count,
+ * `                       char buffer[]);
+ *
+ * @cmd: Command (see below)
+ * @count: Size of the buffer to read/write
+ * @buffer: Pointer in the guest memory
+ *
+ * List of commands:
+ *
+ *  * CONSOLEIO_write: Write the buffer on Xen console.
+ *      For the hardware domain, all the characters in the buffer will
+ *      be written. Characters will be printed to directly to the
+ *      console.
+ *      For all the other domains, only the printable characters will be
+ *      written. Characters may be buffered until a newline (i.e '\n') is
+ *      found.
+ *      @return 0 on success, otherwise return an error code.
+ *  * CONSOLEIO_read: Attempts to read up @count characters from Xen console.
+ *      The maximum buffer size (i.e @count) supported is 2GB.
+ *      @return the number of character read on success, otherwise return
+ *      an error code.
  */
 #define CONSOLEIO_write         0
 #define CONSOLEIO_read          1
-- 
2.11.0


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

[Xen-devel] [PATCH v2 4/4] xen/console: Simplify domU console handling in guest_console_write

[Julien Grall]

2 paths in the domU console handling are now the same. So they can be
merged to make the code simpler.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Acked-by: Wei Liu <wei.liu2@citrix.com>

---
    Changes in v2:
        - Fix indentation
        - Add Stefano's reviewed-by
        - Add Wei's acked-by
---
 xen/drivers/char/console.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index fa8f5cff4b..4105ca58d8 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -585,13 +585,8 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
             *kout = '\0';
             spin_lock(&cd->pbuf_lock);
             kcount = kin - kbuf;
-            if ( c == '\n' )
-            {
-                cd->pbuf[cd->pbuf_idx] = '\0';
-                guest_printk(cd, XENLOG_G_DEBUG "%s%s\n", cd->pbuf, kbuf);
-                cd->pbuf_idx = 0;
-            }
-            else if ( cd->pbuf_idx + (kout - kbuf) < (DOMAIN_PBUF_SIZE - 1) )
+            if ( c != '\n' &&
+                 (cd->pbuf_idx + (kout - kbuf) < (DOMAIN_PBUF_SIZE - 1)) )
             {
                 /* buffer the output until a newline */
                 memcpy(cd->pbuf + cd->pbuf_idx, kbuf, kout - kbuf);
-- 
2.11.0


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

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

[Jan Beulich]

On 05.08.2019 15:29, Julien Grall wrote:
> @@ -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);

While - given the size of cntbuf - the difference is mostly
benign, you using vscnprintf() above calls for you also
using scnprintf() here.

> --- 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)

While I don't think there's overly much risk of "s" getting an
argument with side effects passed, I think that for "nr" the
risk is there. May I ask that you evaluate both here, just in
case?

Preferably with these adjustments
Reviewed-by: Jan Beulich <jbeulich@suse.com>

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

Re: [Xen-devel] [PATCH v2 2/4] xen/console: Rework HYPERCALL_console_io interface

[Jan Beulich]

On 05.08.2019 15:29, Julien Grall wrote:
> @@ -627,6 +629,15 @@ long do_console_io(int cmd, int count, XEN_GUEST_HANDLE_PARAM(char) buffer)
>           rc = guest_console_write(buffer, count);
>           break;
>       case CONSOLEIO_read:
> +        /*
> +         * The return value is either the number of characters read or
> +         * a negative value in case of error. So we need to prevent
> +         * overlap between the two sets.
> +         */
> +        rc = -E2BIG;
> +        if ( (int)count < 0 )
> +            break;

A more portable (afaict) approach would be to check against INT_MAX.
Either way
Reviewed-by: Jan Beulich <jbeulich@suse.com>

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

Re: [Xen-devel] [PATCH v2 2/4] xen/console: Rework HYPERCALL_console_io interface

[Julien Grall]

On 08/08/2019 14:57, Jan Beulich wrote:
> On 05.08.2019 15:29, Julien Grall wrote:
>> @@ -627,6 +629,15 @@ long do_console_io(int cmd, int count, XEN_GUEST_HANDLE_PARAM(char) buffer)
>>            rc = guest_console_write(buffer, count);
>>            break;
>>        case CONSOLEIO_read:
>> +        /*
>> +         * The return value is either the number of characters read or
>> +         * a negative value in case of error. So we need to prevent
>> +         * overlap between the two sets.
>> +         */
>> +        rc = -E2BIG;
>> +        if ( (int)count < 0 )
>> +            break;
> 
> A more portable (afaict) approach would be to check against INT_MAX.

It would be better than the cast. I will update it.

> Either way
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thank you!

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH v2 3/4] xen/public: Document HYPERCALL_console_io()

[Jan Beulich]

On 05.08.2019 15:29, Julien Grall wrote:
> Currently, OS developpers will have to look at Xen code in order to know
> the parameters of an hypercall and how it is meant to work.
> 
> This is not a trivial task as you may need to have a deep understanding
> of Xen internal.
> 
> This patch attempts to document the behavior of HYPERCALL_console_io() to
> help OS developer.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Acked-by: Jan Beulich <jbeulich@suse.com>
with a couple of nits:

> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -486,7 +486,29 @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t);
>   /* ` } */
>   
>   /*
> - * Commands to HYPERVISOR_console_io().
> + * ` int
> + * ` HYPERVISOR_console_io(unsigned int cmd,
> + * `                       unsigned int count,
> + * `                       char buffer[]);
> + *
> + * @cmd: Command (see below)
> + * @count: Size of the buffer to read/write
> + * @buffer: Pointer in the guest memory
> + *
> + * List of commands:
> + *
> + *  * CONSOLEIO_write: Write the buffer on Xen console.

s/ on / to / ?

> + *      For the hardware domain, all the characters in the buffer will
> + *      be written. Characters will be printed to directly to the

The first "to" looks to be unwanted.

> + *      console.
> + *      For all the other domains, only the printable characters will be
> + *      written. Characters may be buffered until a newline (i.e '\n') is
> + *      found.
> + *      @return 0 on success, otherwise return an error code.
> + *  * CONSOLEIO_read: Attempts to read up @count characters from Xen console.

"... up to @count ..."

> + *      The maximum buffer size (i.e @count) supported is 2GB.

"i.e." or "ie" are the two common forms I'm aware of.

> + *      @return the number of character read on success, otherwise return

"characters"

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

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

[Julien Grall]

Hi Jan,

On 08/08/2019 14:51, Jan Beulich wrote:
> On 05.08.2019 15:29, Julien Grall wrote:
>> @@ -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);
> 
> While - given the size of cntbuf - the difference is mostly
> benign, you using vscnprintf() above calls for you also
> using scnprintf() here.

Good point, it would be safer too. I will update it.

> 
>> --- 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)
> 
> While I don't think there's overly much risk of "s" getting an
> argument with side effects passed, I think that for "nr" the
> risk is there. May I ask that you evaluate both here, just in
> case?

Are you happy with the following code (Not yet compiled!):

#define video_ptus(s, nr) ((void)(s), (void)(nr))

> 
> Preferably with these adjustments
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thank you!

Cheers,

-- 
Julien Grall

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

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

[Jan Beulich]

On 08.08.2019 16:10, Julien Grall wrote:
> On 08/08/2019 14:51, Jan Beulich wrote:
>> On 05.08.2019 15:29, Julien Grall wrote:
>>> --- 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)
>>
>> While I don't think there's overly much risk of "s" getting an
>> argument with side effects passed, I think that for "nr" the
>> risk is there. May I ask that you evaluate both here, just in
>> case?
> 
> Are you happy with the following code (Not yet compiled!):
> 
> #define video_ptus(s, nr) ((void)(s), (void)(nr))

With s/ptus/puts/ - sure. A static inline might be another
(even better) option.

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

Re: [Xen-devel] [PATCH v2 3/4] xen/public: Document HYPERCALL_console_io()

[Julien Grall]

Hi,

On 08/08/2019 15:03, Jan Beulich wrote:
> On 05.08.2019 15:29, Julien Grall wrote:
>> Currently, OS developpers will have to look at Xen code in order to know
>> the parameters of an hypercall and how it is meant to work.
>>
>> This is not a trivial task as you may need to have a deep understanding
>> of Xen internal.
>>
>> This patch attempts to document the behavior of HYPERCALL_console_io() to
>> help OS developer.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> Acked-by: Jan Beulich <jbeulich@suse.com>
> with a couple of nits:
> 
>> --- a/xen/include/public/xen.h
>> +++ b/xen/include/public/xen.h
>> @@ -486,7 +486,29 @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t);
>>    /* ` } */
>>    
>>    /*
>> - * Commands to HYPERVISOR_console_io().
>> + * ` int
>> + * ` HYPERVISOR_console_io(unsigned int cmd,
>> + * `                       unsigned int count,
>> + * `                       char buffer[]);
>> + *
>> + * @cmd: Command (see below)
>> + * @count: Size of the buffer to read/write
>> + * @buffer: Pointer in the guest memory
>> + *
>> + * List of commands:
>> + *
>> + *  * CONSOLEIO_write: Write the buffer on Xen console.
> 
> s/ on / to / ?

I am not entirely sure. Looking online [1] "on" would make sense here.

But I am not a native english speaker. @George, @Ian, @Andrew, any opinions?

> 
>> + *      For the hardware domain, all the characters in the buffer will
>> + *      be written. Characters will be printed to directly to the
> 
> The first "to" looks to be unwanted.

Yes, I have dropped it.

> 
>> + *      console.
>> + *      For all the other domains, only the printable characters will be
>> + *      written. Characters may be buffered until a newline (i.e '\n') is
>> + *      found.
>> + *      @return 0 on success, otherwise return an error code.
>> + *  * CONSOLEIO_read: Attempts to read up @count characters from Xen console.
> 
> "... up to @count ..."
> 
>> + *      The maximum buffer size (i.e @count) supported is 2GB.
> 
> "i.e." or "ie" are the two common forms I'm aware of.

Yes, I must have made up that one. I will use the i.e. form.

> 
>> + *      @return the number of character read on success, otherwise return
> 
> "characters"

Fixed.

Thank you for the ack. I will wait for the on/to discussion to settle before 
committing.

Cheers,

[1] https://idioms.thefreedictionary.com/write+on

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH v2 3/4] xen/public: Document HYPERCALL_console_io()

[Julien Grall]

On 8/16/19 6:42 PM, Julien Grall wrote:
> Hi,
> 
> On 08/08/2019 15:03, Jan Beulich wrote:
>> On 05.08.2019 15:29, Julien Grall wrote:
>>> Currently, OS developpers will have to look at Xen code in order to know
>>> the parameters of an hypercall and how it is meant to work.
>>>
>>> This is not a trivial task as you may need to have a deep understanding
>>> of Xen internal.
>>>
>>> This patch attempts to document the behavior of 
>>> HYPERCALL_console_io() to
>>> help OS developer.
>>>
>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> Acked-by: Jan Beulich <jbeulich@suse.com>
>> with a couple of nits:
>>
>>> --- a/xen/include/public/xen.h
>>> +++ b/xen/include/public/xen.h
>>> @@ -486,7 +486,29 @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t);
>>>    /* ` } */
>>>    /*
>>> - * Commands to HYPERVISOR_console_io().
>>> + * ` int
>>> + * ` HYPERVISOR_console_io(unsigned int cmd,
>>> + * `                       unsigned int count,
>>> + * `                       char buffer[]);
>>> + *
>>> + * @cmd: Command (see below)
>>> + * @count: Size of the buffer to read/write
>>> + * @buffer: Pointer in the guest memory
>>> + *
>>> + * List of commands:
>>> + *
>>> + *  * CONSOLEIO_write: Write the buffer on Xen console.
>>
>> s/ on / to / ?
> 
> I am not entirely sure. Looking online [1] "on" would make sense here.
> 
> But I am not a native english speaker. @George, @Ian, @Andrew, any 
> opinions?

Speaking with my partner today (who is native english), "to" is actually 
the correct version here. So I will use "to" here.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH v2 0/4] xen/console: Bug fixes and doc improvement

[Julien Grall]

Hi,

On 8/5/19 2:29 PM, Julien Grall wrote:
> Hi all,
> 
> This series contains a bunch of bug fixes for the hypercall CONSOLEIO_write
> and some documentation.
> 
> Cheers,
> 
> Julien Grall (4):
>    xen/console: Don't treat NUL character as the end of the buffer
>    xen/console: Rework HYPERCALL_console_io interface
>    xen/public: Document HYPERCALL_console_io()
>    xen/console: Simplify domU console handling in guest_console_write

I have merged the series. Thank you all for the review!

Cheers,

-- 
Julien Grall

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

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

[Stefano Stabellini]

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@arm.com>
> 
> ---
> 
> 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@kernel.org>


> ---
>  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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Julien Grall]

Hi Stefano,

On 10/2/19 2:25 AM, Stefano Stabellini wrote:
> 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.Hi,
>>
>> 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.
>>
>> 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@kernel.org>

Thank you for the acked-by. Although, this was already merged 2 months ago.

Cheers,


-- 
Julien Grall

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