| Message ID | 37e935be214083f1b16b5da4d3be09e7cbafc971.1725294334.git.javi.merino@cloud.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | libxl: Fixes for libxl_xenconsole_readline() | expand |
On 02.09.2024 18:38, Javi Merino wrote: > When built with ASAN, "xl dmesg" crashes in the "printf("%s", line)" > call in main_dmesg(). ASAN reports a heap buffer overflow: an > off-by-one access to cr->buffer. > > The readconsole sysctl copies up to count characters into the buffer, > but it does not add a null character at the end. Despite the > documentation of libxl_xen_console_read_line(), line_r is not > nul-terminated if 16384 characters were copied to the buffer. > > Fix this by asking xc_readconsolering() to fill the buffer up to size > - 1. As the number of characters in the buffer is only needed in > libxl_xen_console_read_line(), make it a local variable there instead > of part of the libxl__xen_console_reader struct. > > Fixes: 4024bae739cc ("xl: Add subcommand 'xl dmesg'") > Reported-by: Edwin Török <edwin.torok@cloud.com> > Reviewed-by: Anthony PERARD <anthony.perard@vates.tech> > Signed-off-by: Javi Merino <javi.merino@cloud.com> Please keep tags in chronological order. I almost overlooked the R-b, which makes this eligible for committing. Jan
diff --git a/tools/libs/light/libxl_console.c b/tools/libs/light/libxl_console.c index a563c9d3c7f9..9f736b891335 100644 --- a/tools/libs/light/libxl_console.c +++ b/tools/libs/light/libxl_console.c @@ -774,12 +774,17 @@ libxl_xen_console_reader * { GC_INIT(ctx); libxl_xen_console_reader *cr; - unsigned int size = 16384; + /* + * We want xen to fill the buffer in as few hypercalls as + * possible, but xen will not nul-terminate it. The default size + * of Xen's console buffer is 16384. Leave one byte at the end + * for the null character. + */ + unsigned int size = 16384 + 1; cr = libxl__zalloc(NOGC, sizeof(libxl_xen_console_reader)); cr->buffer = libxl__zalloc(NOGC, size); cr->size = size; - cr->count = size; cr->clear = clear; cr->incremental = 1; @@ -800,10 +805,16 @@ int libxl_xen_console_read_line(libxl_ctx *ctx, char **line_r) { int ret; + /* + * Number of chars to copy into the buffer. xc_readconsolering() + * does not add a null character at the end, so leave a space for + * us to add it. + */ + unsigned int nr_chars = cr->size - 1; GC_INIT(ctx); memset(cr->buffer, 0, cr->size); - ret = xc_readconsolering(ctx->xch, cr->buffer, &cr->count, + ret = xc_readconsolering(ctx->xch, cr->buffer, &nr_chars, cr->clear, cr->incremental, &cr->index); if (ret < 0) { LOGE(ERROR, "reading console ring buffer"); @@ -811,7 +822,7 @@ int libxl_xen_console_read_line(libxl_ctx *ctx, return ERROR_FAIL; } if (!ret) { - if (cr->count) { + if (nr_chars) { *line_r = cr->buffer; ret = 1; } else { diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h index 089a2f949c53..cfac8e18b6d3 100644 --- a/tools/libs/light/libxl_internal.h +++ b/tools/libs/light/libxl_internal.h @@ -2077,7 +2077,6 @@ _hidden char *libxl__uuid2string(libxl__gc *gc, const libxl_uuid uuid); struct libxl__xen_console_reader { char *buffer; unsigned int size; - unsigned int count; unsigned int clear; unsigned int incremental; unsigned int index;
When built with ASAN, "xl dmesg" crashes in the "printf("%s", line)" call in main_dmesg(). ASAN reports a heap buffer overflow: an off-by-one access to cr->buffer. The readconsole sysctl copies up to count characters into the buffer, but it does not add a null character at the end. Despite the documentation of libxl_xen_console_read_line(), line_r is not nul-terminated if 16384 characters were copied to the buffer. Fix this by asking xc_readconsolering() to fill the buffer up to size - 1. As the number of characters in the buffer is only needed in libxl_xen_console_read_line(), make it a local variable there instead of part of the libxl__xen_console_reader struct. Fixes: 4024bae739cc ("xl: Add subcommand 'xl dmesg'") Reported-by: Edwin Török <edwin.torok@cloud.com> Reviewed-by: Anthony PERARD <anthony.perard@vates.tech> Signed-off-by: Javi Merino <javi.merino@cloud.com> --- tools/libs/light/libxl_console.c | 19 +++++++++++++++---- tools/libs/light/libxl_internal.h | 1 - 2 files changed, 15 insertions(+), 5 deletions(-)