|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 2/4] xen/console: Don't treat NUL character as the end of the buffer
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@xxxxxxx>
>
> ---
>
> 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.
I read the patch and I couldn't find anything beyond what Jan already
mentioned.
> *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;
> @@ -666,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
> @@ -1246,6 +1244,7 @@ void debugtrace_printk(const char *fmt, ...)
> char cntbuf[24];
> va_list args;
> unsigned long flags;
> + unsigned int nr;
>
> if ( debugtrace_bytes == 0 )
> return;
> @@ -1257,14 +1256,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
> {
> @@ -1377,7 +1377,7 @@ void panic(const char *fmt, ...)
> * **************************************************************
> */
>
> -static void suspend_steal_fn(const char *str) { }
> +static void suspend_steal_fn(const char *str, unsigned int 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..7498299807 100644
> --- a/xen/drivers/char/serial.c
> +++ b/xen/drivers/char/serial.c
> @@ -223,11 +223,11 @@ 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, unsigned int nr)
> {
> struct serial_port *port;
> unsigned long flags;
> - char c;
> + unsigned int i;
>
> if ( handle == -1 )
> return;
> @@ -238,8 +238,10 @@ void serial_puts(int handle, const char *s)
>
> spin_lock_irqsave(&port->tx_lock, flags);
>
> - while ( (c = *s++) != '\0' )
> + for ( i = 0; i < nr; i++ )
> {
> + char c = s[i];
> +
> 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..5bb303d4c8 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, unsigned int 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, unsigned int 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 d0c8c492b0..93b6a33a41 100644
> --- a/xen/drivers/video/lfb.c
> +++ b/xen/drivers/video/lfb.c
> @@ -59,14 +59,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, unsigned int nr)
> {
> unsigned int i, min_redraw_y = lfb.ypos;
> - char c;
>
> /* Paste characters into text buffer. */
> - while ( (c = *s++) != '\0' )
> + for ( i = 0; i < nr; i++ )
> {
> + char c = s[i];
> +
> if ( (c == '\n') || (lfb.xpos >= lfb.lfbp.text_columns) )
> {
> if ( ++lfb.ypos >= lfb.lfbp.text_rows )
> @@ -103,13 +104,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, unsigned int nr)
> {
> unsigned int i;
> - char c;
>
> - while ( (c = *s++) != '\0' )
> + for ( i = 0; i < nr; i++ )
> {
> + char c = s[i];
> +
> 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..15599e22ef 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, unsigned int nr);
> +void lfb_scroll_puts(const char *s, unsigned int 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 6a64fd9013..01f12aae42 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, unsigned int nr);
> +static void vga_noop_puts(const char *s, unsigned int nr) {}
> +void (*video_puts)(const char *, unsigned int nr) = vga_noop_puts;
>
> /*
> * 'vga=<mode-specifier>[,keep]' where <mode-specifier> is one of:
> @@ -177,12 +177,14 @@ void __init video_endboot(void)
> }
> }
>
> -static void vga_text_puts(const char *s)
> +static void vga_text_puts(const char *s, unsigned int nr)
> {
> - char c;
> + unsigned int i;
>
> - while ( (c = *s++) != '\0' )
> + for ( i = 0; i < nr; i++ )
> {
> + char c = s[i];
> +
> if ( (c == '\n') || (xpos >= columns) )
> {
> if ( ++ypos >= lines )
> diff --git a/xen/include/xen/console.h b/xen/include/xen/console.h
> index b4f9463936..dafa53ba6b 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 *, unsigned int 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..22f8009a5f 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, unsigned int 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..41144890e4 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, unsigned int 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, unsigned int 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..e11d6d3ebc 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, unsigned int 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..ddd21f374d 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 *, unsigned int 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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |