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 <demi@invisiblethingslab.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
master commit: 531d3bea5e9357357eaf6d40f5784a1b4c29b910
master date: 2024-05-11 00:13:43 +0100

diff --git a/tools/libs/light/libxl_console.c b/tools/libs/light/libxl_console.c
index d8b2bc5465826d5fa9896aa4b063bdd73e797388..7087a0486d148a6859707a8121010602212202a0 100644 (file)
--- a/tools/libs/light/libxl_console.c
+++ b/tools/libs/light/libxl_console.c
@@ -337,11 +337,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;
 }
@@ -628,6 +627,8 @@ 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 a75c21d4337c46094fdbfaaa86a59877cb8d114a..99758524ff20fb5bf0b755d443054be98269e898 100644 (file)
--- 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 (file)
--- 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;