[Xen-changelog] [xen stable-4.6] libxl: Do not trust frontend for channel in getinfo

[patchbot@xxxxxxx]

commit 2805844bf20e1cae824d0ccc864e46dfa8f134da
Author:     Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
AuthorDate: Tue May 3 17:24:32 2016 +0100
Commit:     Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
CommitDate: Mon Jun 6 13:56:34 2016 +0100

    libxl: Do not trust frontend for channel in getinfo
    
    libxl_device_channel_getinfo needs to examine devices without trusting
    frontend-controlled data.  So:
    
    * Use /libxl to find the backend path.
    * Parse the backend path to find the backend domid, rather than
      reading it from the frontend.
    * Tolerate FRONTEND/tty vanishing.
    
    Note that there is a strange off-by-one error in the computation of
    both fe_path and libxl_path in libxl_device_channel_getinfo: the
    incoming channel->devid, which is copied to channelinfo->devid, has +1
    applied to calculate the frontend path (and, after this patch, the
    libxl path).  I.e., the devid passed to libxl_device_channel_getinfo
    must be one less than the actual devid for the device being asked
    about.
    
    This is actually a bug which mirrors a bug in
    libxl__append_channel_list, which fills in the devids of the channel
    devices it finds with sequentially increasing numbers starting at 0.
    
    In the usual case channels have real devids starting at 1 (because
    there is the console, which is devid 0, but not a channel).  So these
    bugs usually cancel out.
    
    We do not address this problem at this time.  This bug does not have
    any security implications.
    
    This patch is part of XSA-175.
    
    Signed-off-by: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
    Reviewed-by: Wei Liu <wei.liu2@xxxxxxxxxx>
---
 tools/libxl/libxl.c | 44 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 36 insertions(+), 8 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 48d491f..db92fae 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3918,23 +3918,28 @@ int libxl_device_channel_getinfo(libxl_ctx *ctx, 
uint32_t domid,
                                  libxl_channelinfo *channelinfo)
 {
     GC_INIT(ctx);
-    char *dompath, *fe_path;
+    char *dompath, *fe_path, *libxl_path;
     char *val;
+    int rc;
 
     dompath = libxl__xs_get_dompath(gc, domid);
     channelinfo->devid = channel->devid;
 
-    fe_path = libxl__sprintf(gc, "%s/device/console/%d", dompath,
-                             channelinfo->devid + 1);
+    fe_path = GCSPRINTF("%s/device/console/%d", dompath,
+                        channelinfo->devid + 1);
+    libxl_path = GCSPRINTF("%s/device/console/%d",
+                           libxl__xs_libxl_path(gc, domid),
+                           channelinfo->devid + 1);
     channelinfo->backend = xs_read(ctx->xsh, XBT_NULL,
-                                   libxl__sprintf(gc, "%s/backend",
-                                   fe_path), NULL);
+                                   GCSPRINTF("%s/backend", libxl_path), NULL);
     if (!channelinfo->backend) {
         GC_FREE;
         return ERROR_FAIL;
     }
-    val = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/backend-id", fe_path));
-    channelinfo->backend_id = val ? strtoul(val, NULL, 10) : -1;
+    rc = libxl__backendpath_parse_domid(gc, channelinfo->backend,
+                                        &channelinfo->backend_id);
+    if (rc) goto out;
+
     val = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/state", fe_path));
     channelinfo->state = val ? strtoul(val, NULL, 10) : -1;
     channelinfo->frontend = xs_read(ctx->xsh, XBT_NULL,
@@ -3952,13 +3957,36 @@ int libxl_device_channel_getinfo(libxl_ctx *ctx, 
uint32_t domid,
     switch (channel->connection) {
          case LIBXL_CHANNEL_CONNECTION_PTY:
              val = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/tty", fe_path));
+             /*
+              * It is obviously very wrong for this value to be in the
+              * frontend.  But in XSA-175 we don't want to re-engineer
+              * this because other xenconsole code elsewhere (some
+              * even out of tree, perhaps) expects this node to be
+              * here.
+              *
+              * FE/pty is readonly for the guest.  It always exists if
+              * FE does because libxl__device_console_add
+              * unconditionally creates it and nothing deletes it.
+              *
+              * The guest can delete the whole FE (which it has write
+              * privilege on) but the containing directories
+              * /local/GUEST[/device[/console]] are also RO for the
+              * guest.  So if the guest deletes FE it cannot recreate
+              * it.
+              *
+              * Therefore the guest cannot cause FE/pty to contain bad
+              * data, although it can cause it to not exist.
+              */
+             if (!val) val = "/NO-SUCH-PATH";
              channelinfo->u.pty.path = strdup(val);
              break;
          default:
              break;
     }
+    rc = 0;
+ out:
     GC_FREE;
-    return 0;
+    return rc;
 }
 
 
/******************************************************************************/
--
generated by git-patchbot for /home/xen/git/xen.git#stable-4.6

_______________________________________________
Xen-changelog mailing list
Xen-changelog@xxxxxxxxxxxxx
http://lists.xensource.com/xen-changelog