Re: [PATCH] libxl: Fix handling XenStore errors in device creation

[Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxxxx>
> >
> > Apart from one nit below:
> >
> > Reviewed-by: Juergen Gross <jgross@xxxxxxxx>
> >
> >> ---
> >>   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

Attachment: signature.asc
Description: PGP signature