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:14:32AM GMT, Anthony PERARD wrote:
> On Fri, Aug 23, 2024 at 08:31:50AM +0200, Jan Beulich wrote:
> > On 22.08.2024 15:13, 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>
> >
> > Perhaps wants a Fixes: tag as well?
> 
> Seems to be:
> Fixes: 4024bae739cc ("xl: Add subcommand 'xl dmesg'")

Yes, I'll add it.

> > > --- 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;
> >
> > While this looks to be addressing the issue at hand, I still wonder: Why
> > does the "count" field exist at all? It's certainly odd to be set right
> > here (when the buffer actually is empty). It's used solely in
> > libxl_xen_console_read_line(), so could be a local variable there.

True.  I just wanted to fix the actual bug, but I guess while we are
at it we can improve this piece of code.

> It isn't only odd to have "count" field, it is also used the wrong way.
> Once "cr->count" is set to 0, the next call to
> libxl_xen_console_read_line() will simply return an empty NULL, even if
> in the mean time more data is available to read from the string.
> Also, if the last call to libxl_xen_console_read_line() was shorter than
> the buffer size, none of the following call will use the full size of
> the buffer. This isn't really a problem for `xl dmesg`, but could be if
> we want to implement "--follow" option for example.
> 
> So Javi, could you remove that `cr->count` field and use a local
> variable instead?

Sure, I'll do that.

> And a comment in the code might be useful about why there's a "-1".
> 
> But I think a better way to address the issue would be to actually set
> '\0' at the end of the string after the call to xc_readconsolering(),
> and remove the not very useful memset(0). That way, it will be more
> explicite as to why we need "-1".

Right, I will get rid of the memset(0) and just nul-terminate the string.

> > Then, further, I wonder why struct libxl__xen_console_reader lives in
> > libxl_internal.h - it's used solely in libxl_console.c.
> 
> History? It actually use to live in libxl.h, 14 yr ago.
>
> > Finally - why libxl__zalloc() when libxl_xen_console_read_line() clears
> > the buffer anyway?
> 
> Overuse of libxl__zalloc when it was use to replace the open coding that
> was used to allocate "cr". While it doesn't seems necessary, I don't
> think it hurt to keep it there.

It used to be a malloc() that was changed to a zalloc() in
39eaabdf4131 (libxl: don't leak buf in libxl_xen_console_read_start
error handling, 2013-12-03).  The comment in the commit message is
wrong, the malloc+memset was being done for libxl_xen_console_reader
(the struct) not for the buffer.

It doesn't hurt to keep it, but if I'm making changes in this area, I
may as well just make it a libxl__malloc() .

Cheers,
Javi