From: Demi Marie Obenour <demi@invisiblethingslab.com>
To: xen-devel@lists.xenproject.org
Cc: "Demi Marie Obenour" <demi@invisiblethingslab.com>,
"Anthony PERARD" <anthony.perard@citrix.com>,
"Juergen Gross" <jgross@suse.com>,
"Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>
Subject: [PATCH] libxl: Fix handling XenStore errors in device creation
Date: Fri, 26 Apr 2024 22:17:03 -0400 [thread overview]
Message-ID: <84a6e6376ab9f5aafac8f33bf772d73fcfd0d818.1713998669.git.demi@invisiblethingslab.com> (raw)
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 <demi@invisiblethingslab.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();
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 (!xs_write(ctx->xsh, t, GCSPRINTF("%s/backend", frontend_path),
+ backend_path, strlen(backend_path)))
+ goto out;
+ if (fents) {
+ rc = libxl__xs_writev_perms(gc, t, frontend_path, fents,
+ frontend_perms, ARRAY_SIZE(frontend_perms));
+ if (rc) goto out;
+ }
+ if (ro_fents) {
+ rc = libxl__xs_writev_perms(gc, t, frontend_path, ro_fents,
+ ro_frontend_perms, ARRAY_SIZE(ro_frontend_perms));
+ if (rc) goto out;
+ }
}
if (bents) {
if (!libxl_only) {
- xs_rm(ctx->xsh, t, backend_path);
- xs_mkdir(ctx->xsh, t, backend_path);
- xs_set_permissions(ctx->xsh, t, backend_path, backend_perms,
- ARRAY_SIZE(backend_perms));
- xs_write(ctx->xsh, t, GCSPRINTF("%s/frontend", backend_path),
- frontend_path, strlen(frontend_path));
- libxl__xs_writev(gc, t, backend_path, bents);
+ if (!xs_rm(ctx->xsh, t, backend_path) && errno != ENOENT)
+ goto out;
+ if (!xs_mkdir(ctx->xsh, t, backend_path))
+ goto out;
+ if (!xs_set_permissions(ctx->xsh, t, backend_path, backend_perms,
+ ARRAY_SIZE(backend_perms)))
+ goto out;
+ if (!xs_write(ctx->xsh, t, GCSPRINTF("%s/frontend", backend_path),
+ frontend_path, strlen(frontend_path)))
+ goto out;
+ rc = libxl__xs_writev(gc, t, backend_path, bents);
+ if (rc) goto out;
}
/*
@@ -276,7 +294,7 @@ retry_transaction:
out:
if (create_transaction && t)
libxl__xs_transaction_abort(gc, &t);
- return rc;
+ return rc != 0 ? rc : ERROR_FAIL;
}
typedef struct {
diff --git a/tools/libs/light/libxl_xshelp.c b/tools/libs/light/libxl_xshelp.c
index 751cd942d95334191885ba3e8e45b77f7de82e34..a6e34ab10f23e674529d81419ec478dbad456deb 100644
--- a/tools/libs/light/libxl_xshelp.c
+++ b/tools/libs/light/libxl_xshelp.c
@@ -60,10 +60,15 @@ int libxl__xs_writev_perms(libxl__gc *gc, xs_transaction_t t,
for (i = 0; kvs[i] != NULL; i += 2) {
path = GCSPRINTF("%s/%s", dir, kvs[i]);
if (path && kvs[i + 1]) {
- int length = strlen(kvs[i + 1]);
- xs_write(ctx->xsh, t, path, kvs[i + 1], length);
- if (perms)
- xs_set_permissions(ctx->xsh, t, path, perms, num_perms);
+ size_t length = strlen(kvs[i + 1]);
+ if (length > UINT_MAX)
+ return ERROR_FAIL;
+ if (!xs_write(ctx->xsh, t, path, kvs[i + 1], length))
+ return ERROR_FAIL;
+ if (perms) {
+ if (!xs_set_permissions(ctx->xsh, t, path, perms, num_perms))
+ return ERROR_FAIL;
+ }
}
}
return 0;
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
reply other threads:[~2024-04-27 2:18 UTC|newest]
Thread overview: [no followups] expand[flat|nested] mbox.gz Atom feed
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=84a6e6376ab9f5aafac8f33bf772d73fcfd0d818.1713998669.git.demi@invisiblethingslab.com \
--to=demi@invisiblethingslab.com \
--cc=anthony.perard@citrix.com \
--cc=jgross@suse.com \
--cc=marmarek@invisiblethingslab.com \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.