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

[Javi Merino <javi.merino@xxxxxxxxx>]

On Fri, Aug 23, 2024 at 10:34:05AM GMT, Andrew Cooper wrote:
> 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.

You are right, having to do 2 hypercalls in the ideal case is not
good.  I'm going to get rid of cr->count, and make
libxl_xen_console_read_line() honour its documentation of returning a
nul-terminated string.  I'll just make the allocation one char bigger,
which was the fix I originally had.

Cheers,
Javi