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(-)
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;
--
2.45.2
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 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
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,
--
Anthony Perard | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
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
© 2016 - 2024 Red Hat, Inc.