Re: [Xen-devel] [PATCH v2 1/4] xen/console: Don't treat NUL character as the end of the buffer
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
---
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
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
Re: [Xen-devel] [PATCH v2 1/4] xen/console: Don't treat NUL character as the end of the buffer
On Mon, 5 Aug 2019, Julien Grall wrote:
> After upgrading Debian to Buster, I have began to notice console
> mangling when using zsh in Dom0. This is happenning because output sent by
> zsh to the console may contain NULs in the middle of the buffer.
>
> The actual implementation of CONSOLEIO_write considers that a buffer
> always terminate with a NUL and therefore will ignore anything after it.
>
> In general, NULs are perfectly legitimate in terminal streams. For
> instance, this could be used for padding slow terminals. See terminfo(5)
> section `Delays and Padding`, or search for the pcre '\bpad'.
>
> Other use cases includes using the console for dumping non-human
> readable information (e.g debugger, file if no network...). With the
> current behavior, the resulting stream will end up to be corrupted.
>
> The documentation for CONSOLEIO_write is pretty limited (to not say
> inexistent). From the declaration, the hypercall takes a buffer and size.
> So this could lead to think the NUL character is allowed in the middle of
> the buffer.
>
> This patch updates the console API to pass the size along the buffer
> down so we can remove the reliance on buffer terminating by a NUL
> character.
>
> Signed-off-by: Julien Grall
>
> ---
>
> 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
> ---
> 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 );
>
Re: [Xen-devel] [PATCH v2 1/4] xen/console: Don't treat NUL character as the end of the buffer
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 1/4] xen/console: Don't treat NUL character as the end of the buffer
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
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
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
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
