From: Demi Marie Obenour <demi@invisiblethingslab.com>
To: "Andrew Cooper" <andrew.cooper3@citrix.com>,
"Jürgen Groß" <jgross@suse.com>,
xen-devel@lists.xenproject.org
Cc: "Anthony PERARD" <anthony.perard@citrix.com>,
"Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>
Subject: Re: [PATCH] libxl: Fix handling XenStore errors in device creation
Date: Fri, 10 May 2024 18:14:20 -0400 [thread overview]
Message-ID: <Zj6cSdiyu31BoFkE@itl-email> (raw)
In-Reply-To: <7384a44d-0eb8-4033-98b6-ddb7fd9a8131@citrix.com>
[-- Attachment #1: Type: text/plain, Size: 3571 bytes --]
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.
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
prev parent reply other threads:[~2024-05-10 22:15 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-27 2:17 [PATCH] libxl: Fix handling XenStore errors in device creation Demi Marie Obenour
2024-05-10 8:05 ` Jürgen Groß
2024-05-10 18:00 ` Andrew Cooper
2024-05-10 22:14 ` Demi Marie Obenour [this message]
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=Zj6cSdiyu31BoFkE@itl-email \
--to=demi@invisiblethingslab.com \
--cc=andrew.cooper3@citrix.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).