| Message ID | 84a6e6376ab9f5aafac8f33bf772d73fcfd0d818.1713998669.git.demi@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | libxl: Fix handling XenStore errors in device creation | expand |
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 <demi@invisiblethingslab.com> Apart from one nit below: Reviewed-by: Juergen Gross <jgross@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. Juergen
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 <demi@invisiblethingslab.com> > > Apart from one nit below: > > Reviewed-by: Juergen Gross <jgross@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
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 <demi@invisiblethingslab.com> > > > > Apart from one nit below: > > > > Reviewed-by: Juergen Gross <jgross@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 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.
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;
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(-)