On 10/05/2024 9:05 am, Jürgen Groß wrote: > On 27.04.24 04:17, Demi Marie Obenour wrote: >> If xenstored runs out of memory it is possible for it to fail operations >> that should succeed. libxl wasn't robust against this, and could fail >> to ensure that the TTY path of a non-initial console was created and >> read-only for guests. This doesn't qualify for an XSA because guests >> should not be able to run xenstored out of memory, but it still needs to >> be fixed. >> >> Add the missing error checks to ensure that all errors are properly >> handled and that at no point can a guest make the TTY path of its >> frontend directory writable. >> >> Signed-off-by: Demi Marie Obenour <d...@invisiblethingslab.com> > > Apart from one nit below: > > Reviewed-by: Juergen Gross <jgr...@suse.com> > >> --- >> tools/libs/light/libxl_console.c | 10 ++--- >> tools/libs/light/libxl_device.c | 72 ++++++++++++++++++++------------ >> tools/libs/light/libxl_xshelp.c | 13 ++++-- >> 3 files changed, 59 insertions(+), 36 deletions(-) >> >> diff --git a/tools/libs/light/libxl_console.c >> b/tools/libs/light/libxl_console.c >> index >> cd7412a3272a2faf4b9dab0ef4dd077e55472546..adf82aa844a4f4989111bfc8a94af18ad8e114f1 >> 100644 >> --- a/tools/libs/light/libxl_console.c >> +++ b/tools/libs/light/libxl_console.c >> @@ -351,11 +351,10 @@ int libxl__device_console_add(libxl__gc *gc, >> uint32_t domid, >> flexarray_append(front, "protocol"); >> flexarray_append(front, LIBXL_XENCONSOLE_PROTOCOL); >> } >> - libxl__device_generic_add(gc, XBT_NULL, device, >> - libxl__xs_kvs_of_flexarray(gc, back), >> - libxl__xs_kvs_of_flexarray(gc, front), >> - libxl__xs_kvs_of_flexarray(gc, >> ro_front)); >> - rc = 0; >> + rc = libxl__device_generic_add(gc, XBT_NULL, device, >> + libxl__xs_kvs_of_flexarray(gc, >> back), >> + libxl__xs_kvs_of_flexarray(gc, >> front), >> + libxl__xs_kvs_of_flexarray(gc, >> ro_front)); >> out: >> return rc; >> } >> @@ -665,6 +664,7 @@ int libxl_device_channel_getinfo(libxl_ctx *ctx, >> uint32_t domid, >> */ >> if (!val) val = "/NO-SUCH-PATH"; >> channelinfo->u.pty.path = strdup(val); >> + if (channelinfo->u.pty.path == NULL) abort(); > > Even with the bad example 2 lines up, please put the "abort();" into a > line of its own.
I've fixed this on commit. ~Andrew
