On 22/08/2024 2:13 pm, 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 making count one less that the size of the allocated
> buffer so that the last byte is always null.
>
> Reported-by: Edwin Török <[email protected]>
> Signed-off-by: Javi Merino <[email protected]>
> ---
> tools/libs/light/libxl_console.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/libs/light/libxl_console.c
> b/tools/libs/light/libxl_console.c
> index a563c9d3c7f9..fa28e2139453 100644
> --- a/tools/libs/light/libxl_console.c
> +++ b/tools/libs/light/libxl_console.c
> @@ -779,7 +779,7 @@ libxl_xen_console_reader *
> cr = libxl__zalloc(NOGC, sizeof(libxl_xen_console_reader));
> cr->buffer = libxl__zalloc(NOGC, size);
> cr->size = size;
> - cr->count = size;
> + cr->count = size - 1;
> cr->clear = clear;
> cr->incremental = 1;
>
This looks like it will fix the ASAN issue, but I think a better fix
would be:
- printf("%s", line);
+ printf("%.*s", cr->count, line);
because otherwise there's a world of sharp corners once Xen has wrapped
the buffer for the first time.
Which brings us a lot of other WTFs in this code...
First, libxl_console.c describes it's functionality in terms of lines,
and line_reader() in the API. Yet it's not lines, it's a 16k buffer
with generally multi-line content. It's too late to fix the naming, but
we could at least rewrite the comments not to be blatant lies.
Just out of context above the hunk is:
unsigned int size = 16384;
which isn't accurate. The size of Xen's console ring can be changed at
compile time (like XenServer does), and/or by command line variable.
Because the dmesg ring is never full (it just keeps producing and
overwriting tail data), it's impossible to get a clean copy except in a
single hypercall; the incremental/offset parameters are an illusion, and
do not function correctly if a new printk() has occurred between
adjacent hypercalls.
And that is why setting count to size - 1 probably isn't wise. It means
that, even in the ideal case where Xen's ringbuffer is 16k, you've still
got to make 2 hypercalls to get the content.
~Andrew
