Re: [PATCH] libxl: Fix handling XenStore errors in device creation
On Fri, May 10, 2024 at 07:00:49PM +0100, Andrew Cooper wrote: > 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 > > > > Apart from one nit below: > > > > Reviewed-by: Juergen Gross > > > >> --- > >> 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 Thank you. Should this be backported to stable braches? It's not a security vulnerability from a Xen upstream PoV, but "running Xenstore out of memory" should be denial of service only, not a potential privilege escalation. This is especially true if Xenstore is in dom0, where there might be other processes that could eat up lots of memory. -- Sincerely, Demi Marie Obenour (she/her/hers) Invisible Things Lab signature.asc Description: PGP signature
Re: [PATCH] libxl: Fix handling XenStore errors in device creation
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 > > Apart from one nit below: > > Reviewed-by: Juergen Gross > >> --- >> 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
Re: [PATCH] libxl: Fix handling XenStore errors in device creation
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 Apart from one nit below: Reviewed-by: Juergen Gross --- 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. Juergen
[PATCH] libxl: Fix handling XenStore errors in device creation
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
---
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();
break;
default:
break;
diff --git a/tools/libs/light/libxl_device.c b/tools/libs/light/libxl_device.c
index
a3d9f6f7df24b6ce1241c9cf0394a01a42c31b41..4faa5fa3bd115354af0a7ff9785acccd848a51bf
100644
--- a/tools/libs/light/libxl_device.c
+++ b/tools/libs/light/libxl_device.c
@@ -177,8 +177,13 @@ int libxl__device_generic_add(libxl__gc *gc,
xs_transaction_t t,
ro_frontend_perms[1].perms = backend_perms[1].perms = XS_PERM_READ;
retry_transaction:
-if (create_transaction)
+if (create_transaction) {
t = xs_transaction_start(ctx->xsh);
+if (t == XBT_NULL) {
+LOGED(ERROR, device->domid, "xs_transaction_start failed");
+return ERROR_FAIL;
+}
+}
/* FIXME: read frontend_path and check state before removing stuff */
@@ -195,42 +200,55 @@ retry_transaction:
if (rc) goto out;
}
-/* xxx much of this function lacks error checks! */
-
if (fents || ro_fents) {
-xs_rm(ctx->xsh, t, frontend_path);
-xs_mkdir(ctx->xsh, t, frontend_path);
+if (!xs_rm(ctx->xsh, t, frontend_path) && errno != ENOENT)
+goto out;
+if (!xs_mkdir(ctx->xsh, t, frontend_path))
+goto out;
/* Console 0 is a special case. It doesn't use the regular PV
* state machine but also the frontend directory has
* historically contained other information, such as the
* vnc-port, which we don't want the guest fiddling with.
*/
if ((device->kind == LIBXL__DEVICE_KIND_CONSOLE && device->devid == 0)
||
-(device->kind == LIBXL__DEVICE_KIND_VUART))
-xs_set_permissions(ctx->xsh, t, frontend_path,
- ro_frontend_perms,
ARRAY_SIZE(ro_frontend_perms));
-else
-xs_set_permissions(ctx->xsh, t, frontend_path,
- frontend_perms, ARRAY_SIZE(frontend_perms));
-xs_write(ctx->xsh, t, GCSPRINTF("%s/backend", frontend_path),
- backend_path, strlen(backend_path));
-if (fents)
-libxl__xs_writev_perms(gc, t, frontend_path, fents,
- frontend_perms, ARRAY_SIZE(frontend_perms));
-if (ro_fents)
-libxl__xs_writev_perms(gc, t, frontend_path, ro_fents,
- ro_frontend_perms,
ARRAY_SIZE(ro_frontend_perms));
+(device->kind == LIBXL__DEVICE_KIND_VUART)) {
+if (!xs_set_permissions(ctx->xsh, t, frontend_path,
+ro_frontend_perms,
ARRAY_SIZE(ro_frontend_perms)))
+goto out;
+} else {
+if (!xs_set_permissions(ctx->xsh, t, frontend_path,
+frontend_perms,
ARRAY_SIZE(frontend_perms)))
+goto out;
+}
+if
