| Message ID | b1c7eb226ce62f50d6a50baa3977830a31fa5c9b.1724430173.git.javi.merino@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libxl: Fixes for libxl_xenconsole_readline() | expand |
On Fri, Aug 23, 2024 at 06:05:03PM +0100, Javi Merino wrote: > diff --git a/tools/libs/light/libxl_console.c b/tools/libs/light/libxl_console.c > index a563c9d3c7f9..012fd996fba9 100644 > --- a/tools/libs/light/libxl_console.c > +++ b/tools/libs/light/libxl_console.c > @@ -774,12 +774,14 @@ 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. Leave one byte at > + * the end for the null */ > + unsigned int size = 16384 + 1; This comment doesn't really explain why the size choosen is 16k+1, it kind of explain the +1 but that's about it. 16k seems to be the initial size https://elixir.bootlin.com/xen/v4.19.0/source/xen/drivers/char/console.c#L110 But then, it is changed to depend on $(nproc) and loglevel https://elixir.bootlin.com/xen/v4.19.0/source/xen/drivers/char/console.c#L1095 with 16k been the minimum it seems: https://elixir.bootlin.com/xen/v4.19.0/source/xen/drivers/char/console.c#L1061 So, I think a useful comment here would reflect that 16k is default size of Xen's console buffer. Also, multi-line comments are normally expected to be with begin and end markers on separated lines: /* * Comments. */ It be nice to fix both comments, but otherwise the patch looks good: Reviewed-by: Anthony PERARD <anthony.perard@vates.tech> Thanks,
On Fri, Aug 23, 2024 at 06:05:03PM +0100, 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. Instead of playing games with the buffer size in order to add an extra NUL character, we could possibly use libxl_write_exactly(ctx, STDOUT_FILENO,...) in main_dmesg(), using cr->count as the buffer length? Regards, Roger.
On 29/08/2024 4:53 pm, Roger Pau Monné wrote: > On Fri, Aug 23, 2024 at 06:05:03PM +0100, 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. > Instead of playing games with the buffer size in order to add an extra > NUL character, we could possibly use libxl_write_exactly(ctx, > STDOUT_FILENO,...) in main_dmesg(), using cr->count as the buffer > length? Sadly no. struct libxl__xen_console_reader (which has the count field) is a libxl private (opaque) type which `xl` can't access. Otherwise this would be a oneline fix already... ~Andrew
On Fri, Aug 30, 2024 at 08:22:29PM +0100, Andrew Cooper wrote: > On 29/08/2024 4:53 pm, Roger Pau Monné wrote: > > On Fri, Aug 23, 2024 at 06:05:03PM +0100, 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. > > Instead of playing games with the buffer size in order to add an extra > > NUL character, we could possibly use libxl_write_exactly(ctx, > > STDOUT_FILENO,...) in main_dmesg(), using cr->count as the buffer > > length? > > Sadly no. > > struct libxl__xen_console_reader (which has the count field) is a libxl > private (opaque) type which `xl` can't access. I was fooled by the libxl_xen_console_reader typedef. > Otherwise this would be a oneline fix already... Hm, maybe the easiest way is to introduce a new function that returns the buffer and the number of characters? (libxl_xen_console_dump_buffer() or similar?) Thanks, Roger.
On Tue, Aug 27, 2024 at 03:20:10PM GMT, Anthony PERARD wrote: > On Fri, Aug 23, 2024 at 06:05:03PM +0100, Javi Merino wrote: > > diff --git a/tools/libs/light/libxl_console.c b/tools/libs/light/libxl_console.c > > index a563c9d3c7f9..012fd996fba9 100644 > > --- a/tools/libs/light/libxl_console.c > > +++ b/tools/libs/light/libxl_console.c > > @@ -774,12 +774,14 @@ 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. Leave one byte at > > + * the end for the null */ > > + unsigned int size = 16384 + 1; > > This comment doesn't really explain why the size choosen is 16k+1, it > kind of explain the +1 but that's about it. > > 16k seems to be the initial size > https://elixir.bootlin.com/xen/v4.19.0/source/xen/drivers/char/console.c#L110 > But then, it is changed to depend on $(nproc) and loglevel > https://elixir.bootlin.com/xen/v4.19.0/source/xen/drivers/char/console.c#L1095 > with 16k been the minimum it seems: > https://elixir.bootlin.com/xen/v4.19.0/source/xen/drivers/char/console.c#L1061 > > So, I think a useful comment here would reflect that 16k is default size > of Xen's console buffer. Ok, I'll add a line that explains this. > Also, multi-line comments are normally expected to be with begin and end > markers on separated lines: > /* > * Comments. > */ In this file, there were more comments that didn't have the end marker in a separate line, so I was just trying to be consistent. For example: - https://elixir.bootlin.com/xen/v4.19.0/source/tools/libs/light/libxl_console.c#L454 - https://elixir.bootlin.com/xen/v4.19.0/source/tools/libs/light/libxl_console.c#L752 (this one is mixed actually) - https://elixir.bootlin.com/xen/v4.19.0/source/tools/libs/light/libxl_console.c#L790 Sure, I will make all comments I introduce have an end marker on a separate line > It be nice to fix both comments, but otherwise the patch looks good: > Reviewed-by: Anthony PERARD <anthony.perard@vates.tech> Thanks for the review! Javi
On Mon, Sep 02, 2024 at 09:50:28AM GMT, Roger Pau Monné wrote: > On Fri, Aug 30, 2024 at 08:22:29PM +0100, Andrew Cooper wrote: > > On 29/08/2024 4:53 pm, Roger Pau Monné wrote: > > > On Fri, Aug 23, 2024 at 06:05:03PM +0100, 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. > > > Instead of playing games with the buffer size in order to add an extra > > > NUL character, we could possibly use libxl_write_exactly(ctx, > > > STDOUT_FILENO,...) in main_dmesg(), using cr->count as the buffer > > > length? > > > > Sadly no. > > > > struct libxl__xen_console_reader (which has the count field) is a libxl > > private (opaque) type which `xl` can't access. > > I was fooled by the libxl_xen_console_reader typedef. > > > Otherwise this would be a oneline fix already... > > Hm, maybe the easiest way is to introduce a new function that returns > the buffer and the number of characters? > (libxl_xen_console_dump_buffer() or similar?) Even if we did that, this function needs to be fixed as it are part of the library and doesn't do what its documentation say: return a nul-terminated string. Personally, I would introduce a function as you suggest and call this interface deprecated. But I think it is overkill in this case, as this is just `xl dmesg`. Cheers, Javi
diff --git a/tools/libs/light/libxl_console.c b/tools/libs/light/libxl_console.c index a563c9d3c7f9..012fd996fba9 100644 --- a/tools/libs/light/libxl_console.c +++ b/tools/libs/light/libxl_console.c @@ -774,12 +774,14 @@ 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. Leave one byte at + * the end for the null */ + 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 +802,14 @@ 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 +817,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 3b58bb2d7f43..96d14f5746e1 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> Signed-off-by: Javi Merino <javi.merino@cloud.com> --- tools/libs/light/libxl_console.c | 14 ++++++++++---- tools/libs/light/libxl_internal.h | 1 - 2 files changed, 10 insertions(+), 5 deletions(-)