| Message ID | 20190226230351.12882-1-julien.grall@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [Xen-devel,for-4.12,RFC] xen/console: Handle NUL character in buffer sent via CONSOLEIO_write | expand |
>>> On 27.02.19 at 00:03, <julien.grall@arm.com> wrote: > After upgrading Debian to Buster, I started noticing console mangling > when using zsh. This is happenning because output sent by zsh to the > console may contain NUL character in the middle of the buffer. > > Linux is sending the buffer as it is to Xen console via CONSOLEIO_write. > However, the implementation in Xen considers NUL character is used to > terminate the buffer and therefore will ignore anything after it. > > The actual documentation of CONSOLEIO_write is pretty limited. 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. We don't need the behavior for internal producers, so I think the change touches way too much code. I think all you need to do is make the hypercall handler sense null characters, and perhaps simply invoke lower level handlers multiple times. Or replace them by something else (e.g. a blank). Jan
Hi, On 2/27/19 10:25 AM, Jan Beulich wrote: >>>> On 27.02.19 at 00:03, <julien.grall@arm.com> wrote: >> After upgrading Debian to Buster, I started noticing console mangling >> when using zsh. This is happenning because output sent by zsh to the >> console may contain NUL character in the middle of the buffer. >> >> Linux is sending the buffer as it is to Xen console via CONSOLEIO_write. >> However, the implementation in Xen considers NUL character is used to >> terminate the buffer and therefore will ignore anything after it. >> >> The actual documentation of CONSOLEIO_write is pretty limited. 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. > > We don't need the behavior for internal producers, so I think the change > touches way too much code. I think all you need to do is make the > hypercall handler sense null characters, and perhaps simply invoke lower > level handlers multiple times. Or replace them by something else (e.g. a > blank). I have to disagree here. If the OS decides to pass a buffer containing NUL character, then we should honor it and send the NUL character to the serial. Otherwise you may have a different behavior when running on baremetal and on Xen. One case I have in mind is debugging over HVC console. So we need to modify the console API for handling this purpose. Yes, it will allow the internal producers to put NUL character in it. But that's not a real issue by itself. Cheers,
(+ Juergen Gross as RM) I forgot to CC Juergen for this. On 2/26/19 11:03 PM, Julien Grall wrote: > After upgrading Debian to Buster, I started noticing console mangling > when using zsh. This is happenning because output sent by zsh to the > console may contain NUL character in the middle of the buffer. > > Linux is sending the buffer as it is to Xen console via CONSOLEIO_write. > However, the implementation in Xen considers NUL character is used to > terminate the buffer and therefore will ignore anything after it. > > The actual documentation of CONSOLEIO_write is pretty limited. 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 is an early RFC to start getting feedback on the issue and raise > awareness on the problem. > > This patch is candidate for Xen 4.12. Without it zsh output gets mangled > when using the upcoming Debian Buster. A workaround is to add in your > .zshrc: > > setopt single_line_zle > > For the longer bits, it looks like zsh is now adding NUL characters in > the middle of the output sent onto the console. Below an easy way to > repro it 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 > whether the video and PV console is correct. I would appreciate help for > testing here. > > TODO: Actually document CONSOLEIO_write in the public header. > > --- > xen/arch/arm/early_printk.c | 14 +++++++++----- > xen/drivers/char/console.c | 37 +++++++++++++++++++------------------ > xen/drivers/char/consoled.c | 4 ++-- > 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 ++-- > 13 files changed, 66 insertions(+), 55 deletions(-) > > diff --git a/xen/arch/arm/early_printk.c b/xen/arch/arm/early_printk.c > index 97466a12b1..dd2e9fb46f 100644 > --- a/xen/arch/arm/early_printk.c > +++ b/xen/arch/arm/early_printk.c > @@ -17,13 +17,17 @@ > 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') { > - if (*s == '\n') > + unsigned int i; > + > + for ( i = 0; i < nr; i++ ) > + { > + char c = *s; > + > + if (c == '\n') > early_putch('\r'); > - early_putch(*s); > - s++; > + early_putch(c); > } > > /* > diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c > index 4315588f05..cce1211a0c 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 ) > @@ -547,8 +547,8 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count) > /* Use direct console output as it could be interactive */ > spin_lock_irq(&console_lock); > > - sercon_puts(kbuf); > - video_puts(kbuf); > + sercon_puts(kbuf, kcount); > + video_puts(kbuf, kcount); > > #ifdef CONFIG_X86 > if ( opt_console_xen ) > @@ -666,16 +666,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 > @@ -1233,6 +1233,7 @@ void debugtrace_printk(const char *fmt, ...) > va_list args; > char *p; > unsigned long flags; > + unsigned int nr; > > if ( debugtrace_bytes == 0 ) > return; > @@ -1246,12 +1247,12 @@ void debugtrace_printk(const char *fmt, ...) > snprintf(buf, sizeof(buf), "%u ", ++count); > > va_start(args, fmt); > - (void)vsnprintf(buf + strlen(buf), sizeof(buf) - strlen(buf), fmt, args); > + nr = vscnprintf(buf + strlen(buf), sizeof(buf) - strlen(buf), fmt, args); > va_end(args); > > if ( debugtrace_send_to_console ) > { > - serial_puts(sercon_handle, buf); > + serial_puts(sercon_handle, buf, nr); > } > else > { > @@ -1357,7 +1358,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..3e849a2557 100644 > --- a/xen/drivers/char/consoled.c > +++ b/xen/drivers/char/consoled.c > @@ -77,7 +77,7 @@ size_t consoled_guest_rx(void) > > if ( idx >= BUF_SZ ) > { > - pv_console_puts(buf); > + pv_console_puts(buf, BUF_SZ); > idx = 0; > } > } > @@ -85,7 +85,7 @@ size_t consoled_guest_rx(void) > 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 */ > 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 > >
On Tue, Feb 26, 2019 at 11:03:51PM +0000, Julien Grall wrote: > After upgrading Debian to Buster, I started noticing console mangling > when using zsh. This is happenning because output sent by zsh to the > console may contain NUL character in the middle of the buffer. > > Linux is sending the buffer as it is to Xen console via CONSOLEIO_write. > However, the implementation in Xen considers NUL character is used to > terminate the buffer and therefore will ignore anything after it. > > The actual documentation of CONSOLEIO_write is pretty limited. 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> > > --- > [...] > @@ -527,7 +527,7 @@ static inline void xen_console_write_debug_port(const char *buf, size_t len) > static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count) > { > char kbuf[128]; > - int kcount = 0; > + unsigned int kcount = 0; > struct domain *cd = current->domain; > > while ( count > 0 ) > @@ -547,8 +547,8 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count) > /* Use direct console output as it could be interactive */ > spin_lock_irq(&console_lock); > > - sercon_puts(kbuf); > - video_puts(kbuf); > + sercon_puts(kbuf, kcount); > + video_puts(kbuf, kcount); > I think you missed the non-hwdom branch in the same function. It still strips non-printable characters. > int console_suspend(void) [...] > diff --git a/xen/drivers/char/consoled.c b/xen/drivers/char/consoled.c > index 552abf5766..3e849a2557 100644 > --- a/xen/drivers/char/consoled.c > +++ b/xen/drivers/char/consoled.c > @@ -77,7 +77,7 @@ size_t consoled_guest_rx(void) > > if ( idx >= BUF_SZ ) > { > - pv_console_puts(buf); > + pv_console_puts(buf, BUF_SZ); > idx = 0; > } > } > @@ -85,7 +85,7 @@ size_t consoled_guest_rx(void) > if ( idx ) > { > buf[idx] = '\0'; Can this be deleted? Now that you've explicitly sized the buffer. Wei.
On Wed, Feb 27, 2019 at 03:25:22AM -0700, Jan Beulich wrote: > >>> On 27.02.19 at 00:03, <julien.grall@arm.com> wrote: > > After upgrading Debian to Buster, I started noticing console mangling > > when using zsh. This is happenning because output sent by zsh to the > > console may contain NUL character in the middle of the buffer. > > > > Linux is sending the buffer as it is to Xen console via CONSOLEIO_write. > > However, the implementation in Xen considers NUL character is used to > > terminate the buffer and therefore will ignore anything after it. > > > > The actual documentation of CONSOLEIO_write is pretty limited. 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. > > We don't need the behavior for internal producers, so I think the change > touches way too much code. I think all you need to do is make the > hypercall handler sense null characters, and perhaps simply invoke lower > level handlers multiple times. Or replace them by something else (e.g. a > blank). I don't think replacing NULs with blank is the right thing to do. It's not only about how human perceives the output, but also about scripting. Wei.
Hi Wei, On 2/27/19 12:55 PM, Wei Liu wrote: > On Tue, Feb 26, 2019 at 11:03:51PM +0000, Julien Grall wrote: >> After upgrading Debian to Buster, I started noticing console mangling >> when using zsh. This is happenning because output sent by zsh to the >> console may contain NUL character in the middle of the buffer. >> >> Linux is sending the buffer as it is to Xen console via CONSOLEIO_write. >> However, the implementation in Xen considers NUL character is used to >> terminate the buffer and therefore will ignore anything after it. >> >> The actual documentation of CONSOLEIO_write is pretty limited. 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> >> >> --- >> > [...] >> @@ -527,7 +527,7 @@ static inline void xen_console_write_debug_port(const char *buf, size_t len) >> static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count) >> { >> char kbuf[128]; >> - int kcount = 0; >> + unsigned int kcount = 0; >> struct domain *cd = current->domain; >> >> while ( count > 0 ) >> @@ -547,8 +547,8 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count) >> /* Use direct console output as it could be interactive */ >> spin_lock_irq(&console_lock); >> >> - sercon_puts(kbuf); >> - video_puts(kbuf); >> + sercon_puts(kbuf, kcount); >> + video_puts(kbuf, kcount); >> > > I think you missed the non-hwdom branch in the same function. It still > strips non-printable characters. Good point. The non-printable characters was added by Daniel in commit 48d50de8e0 " console: buffer and show origin of guest PV writes" without much explanation. The only reason I can see is, as we buffer the guest writes, the console would be screwed if we split an escape sequence. Furthermore, for guest output, we will always append "(dX)" to the output. So I am not entirely sure what to do in the non-hwdom case. Any opinions? > >> int console_suspend(void) > [...] >> diff --git a/xen/drivers/char/consoled.c b/xen/drivers/char/consoled.c >> index 552abf5766..3e849a2557 100644 >> --- a/xen/drivers/char/consoled.c >> +++ b/xen/drivers/char/consoled.c >> @@ -77,7 +77,7 @@ size_t consoled_guest_rx(void) >> >> if ( idx >= BUF_SZ ) >> { >> - pv_console_puts(buf); >> + pv_console_puts(buf, BUF_SZ); >> idx = 0; >> } >> } >> @@ -85,7 +85,7 @@ size_t consoled_guest_rx(void) >> if ( idx ) >> { >> buf[idx] = '\0'; > > Can this be deleted? Now that you've explicitly sized the buffer. We probably can delete a few of the '\0' over the console code. I will have a look at it. Cheers,
On 2/27/19 1:45 PM, Julien Grall wrote: > Hi Wei, > > On 2/27/19 12:55 PM, Wei Liu wrote: >> On Tue, Feb 26, 2019 at 11:03:51PM +0000, Julien Grall wrote: >>> After upgrading Debian to Buster, I started noticing console mangling >>> when using zsh. This is happenning because output sent by zsh to the >>> console may contain NUL character in the middle of the buffer. >>> >>> Linux is sending the buffer as it is to Xen console via CONSOLEIO_write. >>> However, the implementation in Xen considers NUL character is used to >>> terminate the buffer and therefore will ignore anything after it. >>> >>> The actual documentation of CONSOLEIO_write is pretty limited. 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> >>> >>> --- >>> >> [...] >>> @@ -527,7 +527,7 @@ static inline void xen_console_write_debug_port(const char *buf, size_t len) >>> static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count) >>> { >>> char kbuf[128]; >>> - int kcount = 0; >>> + unsigned int kcount = 0; >>> struct domain *cd = current->domain; >>> while ( count > 0 ) >>> @@ -547,8 +547,8 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count) >>> /* Use direct console output as it could be interactive */ >>> spin_lock_irq(&console_lock); >>> - sercon_puts(kbuf); >>> - video_puts(kbuf); >>> + sercon_puts(kbuf, kcount); >>> + video_puts(kbuf, kcount); >> >> I think you missed the non-hwdom branch in the same function. It still >> strips non-printable characters. > > Good point. The non-printable characters was added by Daniel in commit 48d50de8e0 " console: buffer and show origin of guest PV writes" without much explanation. Yes, I added stripping of non-printable characters because escape sequences printed out by some guests (in particular, clear screen sequences printed out by some distro's early boot scripts) interfered with the output of other guests. It also prevents guests from pretending to be one another or the hypervisor, if the console is being used for some kind of auditing or logging. One thing I didn't consider that I probably should have is that isprint() in the hypervisor is not a UTF-8 aware check, so it will end up corrupting characters if your guests treat the console as having that encoding. > The only reason I can see is, as we buffer the guest writes, the console would be screwed if we split an escape sequence. Furthermore, for guest output, we will always append "(dX)" to the output. So I am not entirely sure what to do in the non-hwdom case. > > Any opinions? This really depends on the purpose of the console in the system. Since there's usually possible hypervisor messages in the output, it makes sense to me to treat it as a line-based log containing readable text. However, the ability for the hardware domain to use it interactively is also important for debugging, and limiting or buffering that domain's output would interfere with that. The current handling of the output is a compromise between these two uses.
On 3/1/19 10:59 PM, Daniel De Graaf wrote: > On 2/27/19 1:45 PM, Julien Grall wrote: >> Hi Wei, >> >> On 2/27/19 12:55 PM, Wei Liu wrote: >>> On Tue, Feb 26, 2019 at 11:03:51PM +0000, Julien Grall wrote: >>>> After upgrading Debian to Buster, I started noticing console mangling >>>> when using zsh. This is happenning because output sent by zsh to the >>>> console may contain NUL character in the middle of the buffer. >>>> >>>> Linux is sending the buffer as it is to Xen console via >>>> CONSOLEIO_write. >>>> However, the implementation in Xen considers NUL character is used to >>>> terminate the buffer and therefore will ignore anything after it. zsh is sending a NUL character to the console in the middle of the buffer? Why would it do that? Is that defined behavior, or does it just happen to work because Xen is the first console device to act strange as a result? There's an argument to be made that 1) zsh shouldn't be sending NUL characters, and/or that 2) Linux should be the one 'sanitizing' input it sends to the console. >>>> @@ -527,7 +527,7 @@ static inline void >>>> xen_console_write_debug_port(const char *buf, size_t len) >>>> static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) >>>> buffer, int count) >>>> { >>>> char kbuf[128]; >>>> - int kcount = 0; >>>> + unsigned int kcount = 0; >>>> struct domain *cd = current->domain; >>>> while ( count > 0 ) >>>> @@ -547,8 +547,8 @@ static long >>>> guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count) >>>> /* Use direct console output as it could be >>>> interactive */ >>>> spin_lock_irq(&console_lock); >>>> - sercon_puts(kbuf); >>>> - video_puts(kbuf); >>>> + sercon_puts(kbuf, kcount); >>>> + video_puts(kbuf, kcount); >>> >>> I think you missed the non-hwdom branch in the same function. It still >>> strips non-printable characters. >> >> Good point. The non-printable characters was added by Daniel in commit >> 48d50de8e0 " console: buffer and show origin of guest PV writes" >> without much explanation. > > Yes, I added stripping of non-printable characters because escape > sequences printed out by some guests (in particular, clear screen > sequences printed out by some distro's early boot scripts) interfered > with the output of other guests. It also prevents guests from > pretending to be one another or the hypervisor, if the console is being > used for some kind of auditing or logging. It sounds like it would be useful to add a comment to that effect on the non-hwdomain path, to make sure things don't accidentally get removed. > One thing I didn't consider that I probably should have is that > isprint() in the hypervisor is not a UTF-8 aware check, so it will end > up corrupting characters if your guests treat the console as having that > encoding. > >> The only reason I can see is, as we buffer the guest writes, the >> console would be screwed if we split an escape sequence. Furthermore, >> for guest output, we will always append "(dX)" to the output. So I am >> not entirely sure what to do in the non-hwdom case. >> >> Any opinions? > > This really depends on the purpose of the console in the system. Since > there's usually possible hypervisor messages in the output, it makes > sense to me to treat it as a line-based log containing readable text. > However, the ability for the hardware domain to use it interactively is > also important for debugging, and limiting or buffering that domain's > output would interfere with that. The current handling of the output is > a compromise between these two uses. Right; the system console is a shared resource, and the hypervisor has to make sure that unprivileged guests don't do anything to muck it up. As you say, in the general case they shouldn't be able to insert lines or clear the screen or things like that. If for specific setups it's required for one particular guest to have that kind of access, we should add in an interface to grant that ability to specific guests. -George
Hi, On 04/03/2019 11:21, George Dunlap wrote: > On 3/1/19 10:59 PM, Daniel De Graaf wrote: >> On 2/27/19 1:45 PM, Julien Grall wrote: >>> Hi Wei, >>> >>> On 2/27/19 12:55 PM, Wei Liu wrote: >>>> On Tue, Feb 26, 2019 at 11:03:51PM +0000, Julien Grall wrote: >>>>> After upgrading Debian to Buster, I started noticing console mangling >>>>> when using zsh. This is happenning because output sent by zsh to the >>>>> console may contain NUL character in the middle of the buffer. >>>>> >>>>> Linux is sending the buffer as it is to Xen console via >>>>> CONSOLEIO_write. >>>>> However, the implementation in Xen considers NUL character is used to >>>>> terminate the buffer and therefore will ignore anything after it. > > zsh is sending a NUL character to the console in the middle of the > buffer? Why would it do that? Is that defined behavior, or does it > just happen to work because Xen is the first console device to act > strange as a result? I have no idea why new version of ZSH is adding '\0' in the buffer. But, to be honest, it does not matter to know it. What matters is an application using the POSIX call write() is free to put a '\0' in the middle of the stream. It is up to the reader (i.e screen/gdb...) to decide how to interpret the characters. > > There's an argument to be made that 1) zsh shouldn't be sending NUL > characters, and/or that 2) Linux should be the one 'sanitizing' input it > sends to the console. Your arguments seems to be based on the assumption the console is only used by a human. Console can be used for other purpose, such as communication with an external debugger. So how do you decide what to sanitize? IHMO, both Xen and Linux should just pass all the characters to the other end. This is inline with the semantics of the posix call write(). > >>>>> @@ -527,7 +527,7 @@ static inline void >>>>> xen_console_write_debug_port(const char *buf, size_t len) >>>>> static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) >>>>> buffer, int count) >>>>> { >>>>> char kbuf[128]; >>>>> - int kcount = 0; >>>>> + unsigned int kcount = 0; >>>>> struct domain *cd = current->domain; >>>>> while ( count > 0 ) >>>>> @@ -547,8 +547,8 @@ static long >>>>> guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count) >>>>> /* Use direct console output as it could be >>>>> interactive */ >>>>> spin_lock_irq(&console_lock); >>>>> - sercon_puts(kbuf); >>>>> - video_puts(kbuf); >>>>> + sercon_puts(kbuf, kcount); >>>>> + video_puts(kbuf, kcount); >>>> >>>> I think you missed the non-hwdom branch in the same function. It still >>>> strips non-printable characters. >>> >>> Good point. The non-printable characters was added by Daniel in commit >>> 48d50de8e0 " console: buffer and show origin of guest PV writes" >>> without much explanation. >> >> Yes, I added stripping of non-printable characters because escape >> sequences printed out by some guests (in particular, clear screen >> sequences printed out by some distro's early boot scripts) interfered >> with the output of other guests. It also prevents guests from >> pretending to be one another or the hypervisor, if the console is being >> used for some kind of auditing or logging. > > It sounds like it would be useful to add a comment to that effect on the > non-hwdomain path, to make sure things don't accidentally get removed. I have a patch to document the behavior. Cheers,
Julien Grall writes ("Re: [PATCH for-4.12 RFC] xen/console: Handle NUL character in buffer sent via CONSOLEIO_write"):
> On 04/03/2019 11:21, George Dunlap wrote:
> > zsh is sending a NUL character to the console in the middle of the
> > buffer? Why would it do that? Is that defined behavior, or does it
> > just happen to work because Xen is the first console device to act
> > strange as a result?
>
> I have no idea why new version of ZSH is adding '\0' in the
> buffer. But, to be honest, it does not matter to know it.
>
> What matters is an application using the POSIX call write() is free
> to put a '\0' in the middle of the stream. It is up to the reader
> (i.e screen/gdb...) to decide how to interpret the characters.
Nuls are perfectly legitimate in terminal streams. They used to be
used for padding for slow terminals, for example. See terminfo(5)
section `Delays and Padding', or search for the pcre `\bpad'.
NB that Xen does not control the ultimate kind of terminal. The
terminal is whatever is ultimately connected (via a series of physical
and logical links) to the dom0's console interface.
There is no bug in zsh here.
Ian.
On 27/02/2019 11:45, Julien Grall wrote: > (+ Juergen Gross as RM) > > I forgot to CC Juergen for this. > > On 2/26/19 11:03 PM, Julien Grall wrote: >> After upgrading Debian to Buster, I started noticing console mangling >> when using zsh. This is happenning because output sent by zsh to the >> console may contain NUL character in the middle of the buffer. >> >> Linux is sending the buffer as it is to Xen console via CONSOLEIO_write. >> However, the implementation in Xen considers NUL character is used to >> terminate the buffer and therefore will ignore anything after it. >> >> The actual documentation of CONSOLEIO_write is pretty limited. 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> The risk for a regression is too high this late in the 4.12 release process IMO. My plan is to have only one further RC before branching off 4.12, so please let us shift this patch to 4.13. Juergen
Hi Juergen, On 3/5/19 12:57 PM, Juergen Gross wrote: > On 27/02/2019 11:45, Julien Grall wrote: >> (+ Juergen Gross as RM) >> >> I forgot to CC Juergen for this. >> >> On 2/26/19 11:03 PM, Julien Grall wrote: >>> After upgrading Debian to Buster, I started noticing console mangling >>> when using zsh. This is happenning because output sent by zsh to the >>> console may contain NUL character in the middle of the buffer. >>> >>> Linux is sending the buffer as it is to Xen console via CONSOLEIO_write. >>> However, the implementation in Xen considers NUL character is used to >>> terminate the buffer and therefore will ignore anything after it. >>> >>> The actual documentation of CONSOLEIO_write is pretty limited. 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> > > The risk for a regression is too high this late in the 4.12 release > process IMO. This code path is fairly well tested (console are used pretty much all the times). So a regression would be quickly noticed. This patch has the advantage to allow upgrade to a newer Debian without loosing part of your prompt on zsh. I am not sure whether the problem is the same with other Distros. > > My plan is to have only one further RC before branching off 4.12, > so please let us shift this patch to 4.13. I understand. It is possible to workaround the problem at least with zsh. So a release note in Xen (and maybe Debian) should do the job. Cheers,
On 05/03/2019 15:08, Julien Grall wrote: > Hi Juergen, > > On 3/5/19 12:57 PM, Juergen Gross wrote: >> On 27/02/2019 11:45, Julien Grall wrote: >>> (+ Juergen Gross as RM) >>> >>> I forgot to CC Juergen for this. >>> >>> On 2/26/19 11:03 PM, Julien Grall wrote: >>>> After upgrading Debian to Buster, I started noticing console mangling >>>> when using zsh. This is happenning because output sent by zsh to the >>>> console may contain NUL character in the middle of the buffer. >>>> >>>> Linux is sending the buffer as it is to Xen console via >>>> CONSOLEIO_write. >>>> However, the implementation in Xen considers NUL character is used to >>>> terminate the buffer and therefore will ignore anything after it. >>>> >>>> The actual documentation of CONSOLEIO_write is pretty limited. 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> >> >> The risk for a regression is too high this late in the 4.12 release >> process IMO. > > This code path is fairly well tested (console are used pretty much all > the times). So a regression would be quickly noticed. Only if you test all the guests out in the wild. > This patch has the advantage to allow upgrade to a newer Debian without > loosing part of your prompt on zsh. I am not sure whether the problem is > the same with other Distros. > >> >> My plan is to have only one further RC before branching off 4.12, >> so please let us shift this patch to 4.13. > > I understand. It is possible to workaround the problem at least with > zsh. So a release note in Xen (and maybe Debian) should do the job. Could you please post the needed information for the 4.12 RN? Juergen
Hi Juergen, On 3/5/19 2:12 PM, Juergen Gross wrote: > On 05/03/2019 15:08, Julien Grall wrote: >> Hi Juergen, >> >> On 3/5/19 12:57 PM, Juergen Gross wrote: >>> On 27/02/2019 11:45, Julien Grall wrote: >>>> (+ Juergen Gross as RM) >>>> >>>> I forgot to CC Juergen for this. >>>> >>>> On 2/26/19 11:03 PM, Julien Grall wrote: >>>>> After upgrading Debian to Buster, I started noticing console mangling >>>>> when using zsh. This is happenning because output sent by zsh to the >>>>> console may contain NUL character in the middle of the buffer. >>>>> >>>>> Linux is sending the buffer as it is to Xen console via >>>>> CONSOLEIO_write. >>>>> However, the implementation in Xen considers NUL character is used to >>>>> terminate the buffer and therefore will ignore anything after it. >>>>> >>>>> The actual documentation of CONSOLEIO_write is pretty limited. 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> >>> >>> The risk for a regression is too high this late in the 4.12 release >>> process IMO. >> >> This code path is fairly well tested (console are used pretty much all >> the times). So a regression would be quickly noticed. > > Only if you test all the guests out in the wild. The buffer is bounded to 'nr'. So the worst things that can happen is you print more characters than you wanted. CONSOLEIO_write is using a semantics very similar to write() and we didn't document what happen when encountering a NUL character. So, I highly doubt anyone relies on the current behavior. Thinking a bit more, from what Ian wrote [1], the issue maybe wider than zsh. So maybe we want to write a band-aid patch at least helping the most common case (i.e losing all characters after the first NUL character). The band-aid patch should be contained to just the hypercall. Would that be more suitable for you? > >> This patch has the advantage to allow upgrade to a newer Debian without >> loosing part of your prompt on zsh. I am not sure whether the problem is >> the same with other Distros. >> >>> >>> My plan is to have only one further RC before branching off 4.12, >>> so please let us shift this patch to 4.13. >> >> I understand. It is possible to workaround the problem at least with >> zsh. So a release note in Xen (and maybe Debian) should do the job. > > Could you please post the needed information for the 4.12 RN? How about: " While the hypercall CONSOLEIO_write looks analogous to the POSIX call write, it will only print character up to the first NUL character if any in the buffer. This may result to loss of characters for any application using directly the POSIX call write. The issue has been confirmed some zsh version (such as in Debian Buster). Where the prompt is mangled, this could be avoided by adding 'setopt single_line_zle' in your .zshrc. " Cheers, [1] https://lists.xen.org/archives/html/xen-devel/2019-03/msg00082.html
On 05/03/2019 16:28, Julien Grall wrote: > Hi Juergen, > > On 3/5/19 2:12 PM, Juergen Gross wrote: >> On 05/03/2019 15:08, Julien Grall wrote: >>> Hi Juergen, >>> >>> On 3/5/19 12:57 PM, Juergen Gross wrote: >>>> On 27/02/2019 11:45, Julien Grall wrote: >>>>> (+ Juergen Gross as RM) >>>>> >>>>> I forgot to CC Juergen for this. >>>>> >>>>> On 2/26/19 11:03 PM, Julien Grall wrote: >>>>>> After upgrading Debian to Buster, I started noticing console mangling >>>>>> when using zsh. This is happenning because output sent by zsh to the >>>>>> console may contain NUL character in the middle of the buffer. >>>>>> >>>>>> Linux is sending the buffer as it is to Xen console via >>>>>> CONSOLEIO_write. >>>>>> However, the implementation in Xen considers NUL character is used to >>>>>> terminate the buffer and therefore will ignore anything after it. >>>>>> >>>>>> The actual documentation of CONSOLEIO_write is pretty limited. >>>>>> 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> >>>> >>>> The risk for a regression is too high this late in the 4.12 release >>>> process IMO. >>> >>> This code path is fairly well tested (console are used pretty much all >>> the times). So a regression would be quickly noticed. >> >> Only if you test all the guests out in the wild. > > The buffer is bounded to 'nr'. So the worst things that can happen is > you print more characters than you wanted. CONSOLEIO_write is using a > semantics very similar to write() and we didn't document what happen > when encountering a NUL character. So, I highly doubt anyone relies on > the current behavior. > > Thinking a bit more, from what Ian wrote [1], the issue maybe wider than > zsh. So maybe we want to write a band-aid patch at least helping the > most common case (i.e losing all characters after the first NUL character). > > The band-aid patch should be contained to just the hypercall. Would that > be more suitable for you? My main concern is the reasoning of Daniel: "Yes, I added stripping of non-printable characters because escape sequences printed out by some guests (in particular, clear screen sequences printed out by some distro's early boot scripts) interfered with the output of other guests. It also prevents guests from pretending to be one another or the hypervisor, if the console is being used for some kind of auditing or logging." With keeping in mind that the behavior is the same since more than 5 years I'd prefer to just add the text below to the release note. > >> >>> This patch has the advantage to allow upgrade to a newer Debian without >>> loosing part of your prompt on zsh. I am not sure whether the problem is >>> the same with other Distros. >>> >>>> >>>> My plan is to have only one further RC before branching off 4.12, >>>> so please let us shift this patch to 4.13. >>> >>> I understand. It is possible to workaround the problem at least with >>> zsh. So a release note in Xen (and maybe Debian) should do the job. >> >> Could you please post the needed information for the 4.12 RN? > > How about: > > " > While the hypercall CONSOLEIO_write looks analogous to the POSIX call > write, it will only print character up to the first NUL character if any > in the buffer. This may result to loss of characters for any application > using directly the POSIX call write. > > The issue has been confirmed some zsh version (such as in Debian > Buster). Where the prompt is mangled, this could be avoided by adding > 'setopt single_line_zle' in your .zshrc. > " Sounds okay for me. With keeping in mind that the behavior is the same since more than 5 years I'm in favor of just adding above text to the release note. Juergen
Hi Juergen, On 3/5/19 4:44 PM, Juergen Gross wrote: > On 05/03/2019 16:28, Julien Grall wrote: >> Hi Juergen, >> >> On 3/5/19 2:12 PM, Juergen Gross wrote: >>> On 05/03/2019 15:08, Julien Grall wrote: >>>> Hi Juergen, >>>> >>>> On 3/5/19 12:57 PM, Juergen Gross wrote: >>>>> On 27/02/2019 11:45, Julien Grall wrote: >>>>>> (+ Juergen Gross as RM) >>>>>> >>>>>> I forgot to CC Juergen for this. >>>>>> >>>>>> On 2/26/19 11:03 PM, Julien Grall wrote: >>>>>>> After upgrading Debian to Buster, I started noticing console mangling >>>>>>> when using zsh. This is happenning because output sent by zsh to the >>>>>>> console may contain NUL character in the middle of the buffer. >>>>>>> >>>>>>> Linux is sending the buffer as it is to Xen console via >>>>>>> CONSOLEIO_write. >>>>>>> However, the implementation in Xen considers NUL character is used to >>>>>>> terminate the buffer and therefore will ignore anything after it. >>>>>>> >>>>>>> The actual documentation of CONSOLEIO_write is pretty limited. >>>>>>> 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> >>>>> >>>>> The risk for a regression is too high this late in the 4.12 release >>>>> process IMO. >>>> >>>> This code path is fairly well tested (console are used pretty much all >>>> the times). So a regression would be quickly noticed. >>> >>> Only if you test all the guests out in the wild. >> >> The buffer is bounded to 'nr'. So the worst things that can happen is >> you print more characters than you wanted. CONSOLEIO_write is using a >> semantics very similar to write() and we didn't document what happen >> when encountering a NUL character. So, I highly doubt anyone relies on >> the current behavior. >> >> Thinking a bit more, from what Ian wrote [1], the issue maybe wider than >> zsh. So maybe we want to write a band-aid patch at least helping the >> most common case (i.e losing all characters after the first NUL character). >> >> The band-aid patch should be contained to just the hypercall. Would that >> be more suitable for you? > > My main concern is the reasoning of Daniel: > > "Yes, I added stripping of non-printable characters because escape > sequences printed out by some guests (in particular, clear screen > sequences printed out by some distro's early boot scripts) interfered > with the output of other guests. It also prevents guests from > pretending to be one another or the hypervisor, if the console is being > used for some kind of auditing or logging." > > With keeping in mind that the behavior is the same since more than 5 > years I'd prefer to just add the text below to the release note. I am afraid you have taken Daniel's comment out of the context. His comment was in discussion about what should be the guest behavior with NUL character. I have no plan for Xen 4.12 (or any later release) to modify this behavior. In the case of Dom0, we already print all the non-printable characters until the first NUL character (or end of the buffer). What this patch is doing is to avoid using NUL character as a special case. Anyway, I am not going to insist. I will defer this to Xen 4.13. Cheers,
diff --git a/xen/arch/arm/early_printk.c b/xen/arch/arm/early_printk.c index 97466a12b1..dd2e9fb46f 100644 --- a/xen/arch/arm/early_printk.c +++ b/xen/arch/arm/early_printk.c @@ -17,13 +17,17 @@ 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') { - if (*s == '\n') + unsigned int i; + + for ( i = 0; i < nr; i++ ) + { + char c = *s; + + if (c == '\n') early_putch('\r'); - early_putch(*s); - s++; + early_putch(c); } /* diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c index 4315588f05..cce1211a0c 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 ) @@ -547,8 +547,8 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count) /* Use direct console output as it could be interactive */ spin_lock_irq(&console_lock); - sercon_puts(kbuf); - video_puts(kbuf); + sercon_puts(kbuf, kcount); + video_puts(kbuf, kcount); #ifdef CONFIG_X86 if ( opt_console_xen ) @@ -666,16 +666,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 @@ -1233,6 +1233,7 @@ void debugtrace_printk(const char *fmt, ...) va_list args; char *p; unsigned long flags; + unsigned int nr; if ( debugtrace_bytes == 0 ) return; @@ -1246,12 +1247,12 @@ void debugtrace_printk(const char *fmt, ...) snprintf(buf, sizeof(buf), "%u ", ++count); va_start(args, fmt); - (void)vsnprintf(buf + strlen(buf), sizeof(buf) - strlen(buf), fmt, args); + nr = vscnprintf(buf + strlen(buf), sizeof(buf) - strlen(buf), fmt, args); va_end(args); if ( debugtrace_send_to_console ) { - serial_puts(sercon_handle, buf); + serial_puts(sercon_handle, buf, nr); } else { @@ -1357,7 +1358,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..3e849a2557 100644 --- a/xen/drivers/char/consoled.c +++ b/xen/drivers/char/consoled.c @@ -77,7 +77,7 @@ size_t consoled_guest_rx(void) if ( idx >= BUF_SZ ) { - pv_console_puts(buf); + pv_console_puts(buf, BUF_SZ); idx = 0; } } @@ -85,7 +85,7 @@ size_t consoled_guest_rx(void) 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 */ 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
After upgrading Debian to Buster, I started noticing console mangling when using zsh. This is happenning because output sent by zsh to the console may contain NUL character in the middle of the buffer. Linux is sending the buffer as it is to Xen console via CONSOLEIO_write. However, the implementation in Xen considers NUL character is used to terminate the buffer and therefore will ignore anything after it. The actual documentation of CONSOLEIO_write is pretty limited. 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 is an early RFC to start getting feedback on the issue and raise awareness on the problem. This patch is candidate for Xen 4.12. Without it zsh output gets mangled when using the upcoming Debian Buster. A workaround is to add in your .zshrc: setopt single_line_zle For the longer bits, it looks like zsh is now adding NUL characters in the middle of the output sent onto the console. Below an easy way to repro it 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 whether the video and PV console is correct. I would appreciate help for testing here. TODO: Actually document CONSOLEIO_write in the public header. --- xen/arch/arm/early_printk.c | 14 +++++++++----- xen/drivers/char/console.c | 37 +++++++++++++++++++------------------ xen/drivers/char/consoled.c | 4 ++-- 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 ++-- 13 files changed, 66 insertions(+), 55 deletions(-)