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

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


  • To: Juergen Gross <jgross@xxxxxxxx>
  • From: David Scott <dave@xxxxxxxxxx>
  • Date: Tue, 7 Feb 2017 09:06:55 +0000
  • Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx, wei.liu2@xxxxxxxxxx, ian.jackson@xxxxxxxxxxxxx
  • Delivery-date: Tue, 07 Feb 2017 09:07:15 +0000
  • Domainkey-signature: a=rsa-sha1; c=nofws; d=recoil.org; h=content-type :mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; q=dns; s= selector1; b=I/tWAtA9We7uAVwXmSwjPdOxshaYYuoVjOELHlIPeg5N5gc7K7l e6XvtrFtcXqFBAGMbKaVJ1rOTqTZW1L7rGMpWRwap11qidNm9aDDq5NLwps/AsHI 0x3EiU8CeYHqXiMWcfF8vpIsgoynR+rVRwI1uhUTQyJ79mZ4Rvjj4IPs=
  • List-id: Xen developer discussion <xen-devel.lists.xen.org>

> On 1 Feb 2017, at 14:49, Juergen Gross <jgross@xxxxxxxx> wrote:
> 
> 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.

Looks fine to me:

Acked-by: David Scott <dave@xxxxxxxxxx>

Cheers,
Dave
> 
> 
> 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®.