[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v3] xenstore: remove XS_RESTRICT support



On 27/01/17 12:47, Juergen Gross wrote:
> XS_RESTRICT and the xenstore library function xs_restrict() have never
> been usable in all configurations and there are no known users.
> 
> This functionality was thought to limit access rights of device models
> to xenstore in order to avoid affecting other domains in case of a
> security breech. Unfortunately XS_RESTRICT won't help as current
> qemu is requiring access to dom0 only accessible xenstore paths to
> work correctly. So this command is useless and should be removed.
> 
> In order to avoid problems in the future remove all support for
> XS_RESTRICT from xenstore.
> 
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>

Adding Dave Scott to Cc: list.


Juergen

> ---
> V3: remove restrict functions in ocaml as requested by Wei Liu
> 
> V2: don't replace XS_RESTRICT enum member with a dummy one as suggested
>     by Andrew Cooper
> ---
>  tools/ocaml/libs/xb/op.ml           |  5 ++---
>  tools/ocaml/libs/xb/xb.mli          |  1 -
>  tools/ocaml/xenstored/connection.ml |  3 ---
>  tools/ocaml/xenstored/logging.ml    |  1 -
>  tools/ocaml/xenstored/perms.ml      |  5 -----
>  tools/ocaml/xenstored/process.ml    | 16 ----------------
>  tools/xenstore/include/xenstore.h   |  9 ---------
>  tools/xenstore/xenstored_core.c     |  1 -
>  tools/xenstore/xs.c                 |  8 --------
>  xen/include/public/io/xs_wire.h     |  4 ++--
>  10 files changed, 4 insertions(+), 49 deletions(-)
> 
> diff --git a/tools/ocaml/libs/xb/op.ml b/tools/ocaml/libs/xb/op.ml
> index 69346d8..d4f1f08 100644
> --- a/tools/ocaml/libs/xb/op.ml
> +++ b/tools/ocaml/libs/xb/op.ml
> @@ -19,7 +19,7 @@ type operation = Debug | Directory | Read | Getperms |
>                   Transaction_end | Introduce | Release |
>                   Getdomainpath | Write | Mkdir | Rm |
>                   Setperms | Watchevent | Error | Isintroduced |
> -                 Resume | Set_target | Restrict | Reset_watches |
> +                 Resume | Set_target | Reset_watches |
>                   Invalid
>  
>  let operation_c_mapping =
> @@ -28,7 +28,7 @@ let operation_c_mapping =
>             Transaction_end; Introduce; Release;
>             Getdomainpath; Write; Mkdir; Rm;
>             Setperms; Watchevent; Error; Isintroduced;
> -           Resume; Set_target; Restrict; Reset_watches |]
> +           Resume; Set_target; Reset_watches |]
>  let size = Array.length operation_c_mapping
>  
>  let array_search el a =
> @@ -68,6 +68,5 @@ let to_string ty =
>       | Isintroduced          -> "IS_INTRODUCED"
>       | Resume                -> "RESUME"
>       | Set_target            -> "SET_TARGET"
> -     | Restrict              -> "RESTRICT"
>       | Reset_watches         -> "RESET_WATCHES"
>       | Invalid               -> "INVALID"
> diff --git a/tools/ocaml/libs/xb/xb.mli b/tools/ocaml/libs/xb/xb.mli
> index 6c242da..b4d7052 100644
> --- a/tools/ocaml/libs/xb/xb.mli
> +++ b/tools/ocaml/libs/xb/xb.mli
> @@ -22,7 +22,6 @@ module Op :
>        | Isintroduced
>        | Resume
>        | Set_target
> -      | Restrict
>        | Reset_watches
>        | Invalid
>      val operation_c_mapping : operation array
> diff --git a/tools/ocaml/xenstored/connection.ml 
> b/tools/ocaml/xenstored/connection.ml
> index 3ffd35b..27fa778 100644
> --- a/tools/ocaml/xenstored/connection.ml
> +++ b/tools/ocaml/xenstored/connection.ml
> @@ -122,9 +122,6 @@ let close con =
>  let get_perm con =
>       con.perm
>  
> -let restrict con domid =
> -     con.perm <- Perms.Connection.restrict con.perm domid
> -
>  let set_target con target_domid =
>       con.perm <- Perms.Connection.set_target (get_perm con) 
> ~perms:[Perms.READ; Perms.WRITE] target_domid
>  
> diff --git a/tools/ocaml/xenstored/logging.ml 
> b/tools/ocaml/xenstored/logging.ml
> index c52f03d..0c0d03d 100644
> --- a/tools/ocaml/xenstored/logging.ml
> +++ b/tools/ocaml/xenstored/logging.ml
> @@ -241,7 +241,6 @@ let string_of_access_type = function
>       | Xenbus.Xb.Op.Mkdir             -> "mkdir    "
>       | Xenbus.Xb.Op.Rm                -> "rm       "
>       | Xenbus.Xb.Op.Setperms          -> "setperms "
> -     | Xenbus.Xb.Op.Restrict          -> "restrict "
>       | Xenbus.Xb.Op.Reset_watches     -> "reset watches"
>       | Xenbus.Xb.Op.Set_target        -> "settarget"
>  
> diff --git a/tools/ocaml/xenstored/perms.ml b/tools/ocaml/xenstored/perms.ml
> index 19bf44c..3ea193e 100644
> --- a/tools/ocaml/xenstored/perms.ml
> +++ b/tools/ocaml/xenstored/perms.ml
> @@ -119,11 +119,6 @@ let is_owner (connection:t) id =
>  let is_dom0 (connection:t) =
>       is_owner connection 0
>  
> -let restrict (connection:t) domid =
> -     match connection.target, connection.main with
> -     | None, (0, perms) -> { connection with main = (domid, perms) }
> -     | _                -> raise Define.Permission_denied
> -
>  let elt_to_string (i,p) =
>       Printf.sprintf "%i%S" i (String.concat "" (List.map String.of_char 
> (List.map char_of_permty p)))
>  
> diff --git a/tools/ocaml/xenstored/process.ml 
> b/tools/ocaml/xenstored/process.ml
> index 7b60376..963549d 100644
> --- a/tools/ocaml/xenstored/process.ml
> +++ b/tools/ocaml/xenstored/process.ml
> @@ -172,30 +172,16 @@ let do_isintroduced con t domains cons data =
>               in
>       if domid = Define.domid_self || Domains.exist domains domid then 
> "T\000" else "F\000"
>  
> -(* [restrict] is in the patch queue since xen3.2 *)
> -let do_restrict con t domains cons data =
> -     if not (Connection.is_dom0 con)
> -     then raise Define.Permission_denied;
> -     let domid =
> -             match (split None '\000' data) with
> -             | [ domid; "" ] -> c_int_of_string domid
> -             | _          -> raise Invalid_Cmd_Args
> -     in
> -     Connection.restrict con domid
> -
>  (* only in xen >= 4.2 *)
>  let do_reset_watches con t domains cons data =
>    Connection.del_watches con;
>    Connection.del_transactions con
>  
>  (* only in >= xen3.3                                                         
>                            *)
> -(* we ensure backward compatibility with restrict by counting the number of 
> argument of set_target ...  *)
> -(* This is not very elegant, but it is safe as 'restrict' only restricts 
> permission of dom0 connections *)
>  let do_set_target con t domains cons data =
>       if not (Connection.is_dom0 con)
>       then raise Define.Permission_denied;
>       match split None '\000' data with
> -             | [ domid; "" ]               -> do_restrict con t domains con 
> data (* backward compatibility with xen3.2-pq *)
>               | [ domid; target_domid; "" ] -> Connections.set_target cons 
> (c_int_of_string domid) (c_int_of_string target_domid)
>               | _                           -> raise Invalid_Cmd_Args
>  
> @@ -244,7 +230,6 @@ let function_of_type_simple_op ty =
>       | Xenbus.Xb.Op.Isintroduced
>       | Xenbus.Xb.Op.Resume
>       | Xenbus.Xb.Op.Set_target
> -     | Xenbus.Xb.Op.Restrict
>       | Xenbus.Xb.Op.Reset_watches
>       | Xenbus.Xb.Op.Invalid           -> error "called 
> function_of_type_simple_op on operation %s" (Xenbus.Xb.Op.to_string ty);
>                                           raise (Invalid_argument 
> (Xenbus.Xb.Op.to_string ty))
> @@ -428,7 +413,6 @@ let function_of_type ty =
>       | Xenbus.Xb.Op.Isintroduced      -> reply_data do_isintroduced
>       | Xenbus.Xb.Op.Resume            -> reply_ack do_resume
>       | Xenbus.Xb.Op.Set_target        -> reply_ack do_set_target
> -     | Xenbus.Xb.Op.Restrict          -> reply_ack do_restrict
>       | Xenbus.Xb.Op.Reset_watches     -> reply_ack do_reset_watches
>       | Xenbus.Xb.Op.Invalid           -> reply_ack do_error
>       | _                              -> function_of_type_simple_op ty
> diff --git a/tools/xenstore/include/xenstore.h 
> b/tools/xenstore/include/xenstore.h
> index 42c0dc7..0d12c39 100644
> --- a/tools/xenstore/include/xenstore.h
> +++ b/tools/xenstore/include/xenstore.h
> @@ -132,15 +132,6 @@ bool xs_mkdir(struct xs_handle *h, xs_transaction_t t,
>  bool xs_rm(struct xs_handle *h, xs_transaction_t t,
>          const char *path);
>  
> -/* Restrict a xenstore handle so that it acts as if it had the
> - * permissions of domain @domid.  The handle must currently be
> - * using domain 0's credentials.
> - *
> - * Returns false on failure, in which case the handle continues
> - * to use the old credentials, or true on success.
> - */
> -bool xs_restrict(struct xs_handle *h, unsigned domid);
> -
>  /* Get permissions of node (first element is owner, first perms is "other").
>   * Returns malloced array, or NULL: call free() after use.
>   */
> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> index fdd4242..1e9b622 100644
> --- a/tools/xenstore/xenstored_core.c
> +++ b/tools/xenstore/xenstored_core.c
> @@ -1309,7 +1309,6 @@ static struct {
>                       { "IS_DOMAIN_INTRODUCED", do_is_domain_introduced },
>       [XS_RESUME]            = { "RESUME",            do_resume },
>       [XS_SET_TARGET]        = { "SET_TARGET",        do_set_target },
> -     [XS_RESTRICT]          = { "RESTRICT",          NULL },
>       [XS_RESET_WATCHES]     = { "RESET_WATCHES",     do_reset_watches },
>       [XS_DIRECTORY_PART]    = { "DIRECTORY_PART",    send_directory_part },
>  };
> diff --git a/tools/xenstore/xs.c b/tools/xenstore/xs.c
> index 3ce7157..6fa1261 100644
> --- a/tools/xenstore/xs.c
> +++ b/tools/xenstore/xs.c
> @@ -769,14 +769,6 @@ unwind:
>       return false;
>  }
>  
> -bool xs_restrict(struct xs_handle *h, unsigned domid)
> -{
> -     char buf[16];
> -
> -     sprintf(buf, "%d", domid);
> -     return xs_bool(xs_single(h, XBT_NULL, XS_RESTRICT, buf, NULL));
> -}
> -
>  /* Watch a node for changes (poll on fd to detect, or call read_watch()).
>   * When the node (or any child) changes, fd will become readable.
>   * Token is returned when watch is read, to allow matching.
> diff --git a/xen/include/public/io/xs_wire.h b/xen/include/public/io/xs_wire.h
> index 54c1d71..f9f94f1 100644
> --- a/xen/include/public/io/xs_wire.h
> +++ b/xen/include/public/io/xs_wire.h
> @@ -48,8 +48,8 @@ enum xsd_sockmsg_type
>      XS_IS_DOMAIN_INTRODUCED,
>      XS_RESUME,
>      XS_SET_TARGET,
> -    XS_RESTRICT,
> -    XS_RESET_WATCHES,
> +    /* XS_RESTRICT has been removed */
> +    XS_RESET_WATCHES = XS_SET_TARGET + 2,
>      XS_DIRECTORY_PART,
>  
>      XS_TYPE_COUNT,      /* Number of valid types. */
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.