Re: [XEN PATCH v1] libxl: Fix nul-termination of the return value of libxl_xen_console_read_line()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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 <edwin.torok@xxxxxxxxx>
> Signed-off-by: Javi Merino <javi.merino@xxxxxxxxx>
> ---
>  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