[XEN PATCH v3 0/3] libxl: Fixes for libxl_xenconsole_readline()

[XEN PATCH v3 0/3] libxl: Fixes for libxl_xenconsole_readline()

[Javi Merino]

Fix nul-termination of the return value of
libxl_xen_console_read_line().  While at it, remove unneeded memset()s
to the buffer and improve the documentation of the function.

Changes since v2[0]:
  - Fixed comment format as suggested by Anthony
  - Clarified that 16384 is the default size of xen's console buffer.
  - Fixed `libxl_xen_console_read_line()` documentation as suggested by Alejandro and Andrew.
  - line_r -> buff in `tools/include/libxl.h` as well.

Changes since v1[1]:
  - Add Fixes: line to the first patch
  - Remove cr->count from the struct and make it a local variable in
    libxl_xen_console_read_line()
  - Improve the documentation of libxl_xen_console_read_line()

[0] https://lore.kernel.org/xen-devel/cover.1724430173.git.javi.merino@cloud.com/
[1] https://lore.kernel.org/xen-devel/ad7c89bbae34155566ae7c9ca2cb501f21c7d585.1724330921.git.javi.merino@cloud.com/

Javi Merino (3):
  libxl: Fix nul-termination of the return value of
    libxl_xen_console_read_line()
  libxl: Remove unnecessary buffer zeroing and zalloc()
  libxl: Update the documentation of libxl_xen_console_read_line()

 tools/include/libxl.h             |  2 +-
 tools/libs/light/libxl_console.c  | 52 +++++++++++++++++++++----------
 tools/libs/light/libxl_internal.h |  1 -
 3 files changed, 36 insertions(+), 19 deletions(-)

-- 
2.45.2

[XEN PATCH v3 1/3] libxl: Fix nul-termination of the return value of libxl_xen_console_read_line()

[Javi Merino]

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>
Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>
Signed-off-by: Javi Merino <javi.merino@cloud.com>
---
 tools/libs/light/libxl_console.c  | 19 +++++++++++++++----
 tools/libs/light/libxl_internal.h |  1 -
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/tools/libs/light/libxl_console.c b/tools/libs/light/libxl_console.c
index a563c9d3c7f9..9f736b891335 100644
--- a/tools/libs/light/libxl_console.c
+++ b/tools/libs/light/libxl_console.c
@@ -774,12 +774,17 @@ 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.  The default size
+     * of Xen's console buffer is 16384.  Leave one byte at the end
+     * for the null character.
+     */
+    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 +805,16 @@ 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 +822,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 089a2f949c53..cfac8e18b6d3 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

Re: [XEN PATCH v3 1/3] libxl: Fix nul-termination of the return value of libxl_xen_console_read_line()

[Jan Beulich]

On 02.09.2024 18:38, 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.
> 
> Fixes: 4024bae739cc ("xl: Add subcommand 'xl dmesg'")
> Reported-by: Edwin Török <edwin.torok@cloud.com>
> Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>
> Signed-off-by: Javi Merino <javi.merino@cloud.com>

Please keep tags in chronological order. I almost overlooked the R-b,
which makes this eligible for committing.

Jan

[XEN PATCH v3 2/3] libxl: Remove unnecessary buffer zeroing and zalloc()

[Javi Merino]

When reading the console, xen overwrites the contents of the buffer,
so there is no need to zero the buffer before passing it to xen.
Instead, add a NULL at the end of the buffer.

While we are at it, change the zalloc() of the buffer back to
malloc() as it was before bdf4131 (libxl: don't leak buf in
libxl_xen_console_read_start error handling, 2013-12-03).  The comment
in that commit message says that the intent of the commit was to
change malloc+memset to zalloc(), but only for the
libxl_xen_console_reader struct, not for the buffer.

Signed-off-by: Javi Merino <javi.merino@cloud.com>
---
 tools/libs/light/libxl_console.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libs/light/libxl_console.c b/tools/libs/light/libxl_console.c
index 9f736b891335..6c4414fcc1a2 100644
--- a/tools/libs/light/libxl_console.c
+++ b/tools/libs/light/libxl_console.c
@@ -783,7 +783,7 @@ libxl_xen_console_reader *
     unsigned int size = 16384 + 1;
 
     cr = libxl__zalloc(NOGC, sizeof(libxl_xen_console_reader));
-    cr->buffer = libxl__zalloc(NOGC, size);
+    cr->buffer = libxl__malloc(NOGC, size);
     cr->size = size;
     cr->clear = clear;
     cr->incremental = 1;
@@ -813,7 +813,6 @@ int libxl_xen_console_read_line(libxl_ctx *ctx,
     unsigned int nr_chars = cr->size - 1;
     GC_INIT(ctx);
 
-    memset(cr->buffer, 0, cr->size);
     ret = xc_readconsolering(ctx->xch, cr->buffer, &nr_chars,
                              cr->clear, cr->incremental, &cr->index);
     if (ret < 0) {
@@ -823,6 +822,7 @@ int libxl_xen_console_read_line(libxl_ctx *ctx,
     }
     if (!ret) {
         if (nr_chars) {
+            cr->buffer[nr_chars] = '\0';
             *line_r = cr->buffer;
             ret = 1;
         } else {
-- 
2.45.2

Re: [XEN PATCH v3 2/3] libxl: Remove unnecessary buffer zeroing and zalloc()

[Anthony PERARD]

On Mon, Sep 02, 2024 at 05:38:38PM +0100, Javi Merino wrote:
> When reading the console, xen overwrites the contents of the buffer,
> so there is no need to zero the buffer before passing it to xen.
> Instead, add a NULL at the end of the buffer.
> 
> While we are at it, change the zalloc() of the buffer back to
> malloc() as it was before bdf4131 (libxl: don't leak buf in
> libxl_xen_console_read_start error handling, 2013-12-03).  The comment
> in that commit message says that the intent of the commit was to
> change malloc+memset to zalloc(), but only for the
> libxl_xen_console_reader struct, not for the buffer.
> 
> Signed-off-by: Javi Merino <javi.merino@cloud.com>

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

[XEN PATCH v3 3/3] libxl: Update the documentation of libxl_xen_console_read_line()

[Javi Merino]

Despite its name, libxl_xen_console_read_line() does not read a line,
it fills the buffer with as many characters as fit.  Update the
documentation to reflect the real behaviour of the function.  Rename
line_r to avoid confusion since it is a pointer to an array of
characters.

Signed-off-by: Javi Merino <javi.merino@cloud.com>
---
 tools/include/libxl.h            |  2 +-
 tools/libs/light/libxl_console.c | 29 ++++++++++++++++++-----------
 2 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index f5c71677424b..8d32428ea9fe 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -2813,7 +2813,7 @@ libxl_xen_console_reader *
     libxl_xen_console_read_start(libxl_ctx *ctx, int clear);
 int libxl_xen_console_read_line(libxl_ctx *ctx,
                                 libxl_xen_console_reader *cr,
-                                char **line_r);
+                                char **buff);
 void libxl_xen_console_read_finish(libxl_ctx *ctx,
                                    libxl_xen_console_reader *cr);
 
diff --git a/tools/libs/light/libxl_console.c b/tools/libs/light/libxl_console.c
index 6c4414fcc1a2..044ca646765a 100644
--- a/tools/libs/light/libxl_console.c
+++ b/tools/libs/light/libxl_console.c
@@ -792,17 +792,24 @@ libxl_xen_console_reader *
     return cr;
 }
 
-/* return values:                                          *line_r
- *   1          success, whole line obtained from buffer    non-0
- *   0          no more lines available right now           0
- *   negative   error code ERROR_*                          0
- * On success *line_r is updated to point to a nul-terminated
- * string which is valid until the next call on the same console
- * reader.  The libxl caller may overwrite parts of the string
- * if it wishes. */
+/*
+ * Copy part of the console ring into a buffer
+ *
+ * Return values:
+ *   1: Success, *buff points to the string
+ *   0: No more lines available right now
+ *   -ERROR_* on error
+ *
+ * Despite its name, libxl_xen_console_read_line() does not
+ * necessarily read a complete line.  It attempts to fill the buffer
+ * with as many characters as it can accommodate.  The buffer pointed
+ * to by *buff is updated to contain a nul-terminated string.  This
+ * string remains valid until the next call to
+ * libxl_xen_console_read_line() on the same console reader.
+ */
 int libxl_xen_console_read_line(libxl_ctx *ctx,
                                 libxl_xen_console_reader *cr,
-                                char **line_r)
+                                char **buff)
 {
     int ret;
     /*
@@ -823,10 +830,10 @@ int libxl_xen_console_read_line(libxl_ctx *ctx,
     if (!ret) {
         if (nr_chars) {
             cr->buffer[nr_chars] = '\0';
-            *line_r = cr->buffer;
+            *buff = cr->buffer;
             ret = 1;
         } else {
-            *line_r = NULL;
+            *buff = NULL;
             ret = 0;
         }
     }
-- 
2.45.2

Re: [XEN PATCH v3 3/3] libxl: Update the documentation of libxl_xen_console_read_line()

[Anthony PERARD]

On Mon, Sep 02, 2024 at 05:38:39PM +0100, Javi Merino wrote:
> Despite its name, libxl_xen_console_read_line() does not read a line,
> it fills the buffer with as many characters as fit.  Update the
> documentation to reflect the real behaviour of the function.  Rename
> line_r to avoid confusion since it is a pointer to an array of
> characters.
> 
> Signed-off-by: Javi Merino <javi.merino@cloud.com>

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