Re: [win-pv-devel] [PATCH 2/2] Add support for changing key permissions to the STORE interface

[RafaÅ WojdyÅa <omeg@xxxxxxxxxxxxxxxxxxxxxx>]

On 2015-09-04 11:25, Paul Durrant wrote:
>> -----Original Message-----
>> From: win-pv-devel-bounces@xxxxxxxxxxxxxxxxxxxx [mailto:win-pv-devel-
>> bounces@xxxxxxxxxxxxxxxxxxxx] On Behalf Of Rafal Wojdyla
>> Sent: 25 August 2015 17:16
>> To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
>> Subject: [win-pv-devel] [PATCH 2/2] Add support for changing key
>> permissions to the STORE interface
>>
>> STORE interface now includes a function to change key permissions. This
>> allows granting key access to other, non-privileged domains.
>>
>> Signed-off-by: RafaÅ WojdyÅa <omeg@xxxxxxxxxxxxxxxxxxxxxx>
>> ---
>>  include/store_interface.h |  68 ++++++++++++-
>>  src/xenbus/store.c        | 254
>> +++++++++++++++++++++++++++++++++++++++++++++-
>>  2 files changed, 317 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/store_interface.h b/include/store_interface.h
>> index 5bcbba3..a34b816 100644
>> --- a/include/store_interface.h
>> +++ b/include/store_interface.h
>> @@ -50,6 +50,23 @@ typedef struct _XENBUS_STORE_TRANSACTION
>> XENBUS_STORE_TRANSACTION, *PXENBUS_S
>>  */
>>  typedef struct _XENBUS_STORE_WATCH          XENBUS_STORE_WATCH,
>> *PXENBUS_STORE_WATCH;
>>
>> +/*! \typedef XENBUS_STORE_PERMISSION_MASK
>> +    \brief Bitmask of XenStore key permissions
>> + */
>> +typedef enum _XENBUS_STORE_PERMISSION_MASK {
>> +    XS_PERM_NONE = 0,
>> +    XS_PERM_READ = 1,
>> +    XS_PERM_WRITE = 2,
>> +} XENBUS_STORE_PERMISSION_MASK;
> 
> These values are not defined in xs_wire.h so I think they need a 
> XENBUS_STORE_ prefix rather than XS_.
> 
Alright. I thought I saw them in there but maybe that was in Linux libxc 
implementation.

>> +
>> +/*! \typedef XENBUS_STORE_PERMISSION
>> +    \brief XenStore key permissions entry for a single domain
>> + */
>> +typedef struct _XENBUS_STORE_PERMISSION {
>> +    USHORT Domain;
>> +    XENBUS_STORE_PERMISSION_MASK Mask;
>> +} XENBUS_STORE_PERMISSION, *PXENBUS_STORE_PERMISSION;
>> +
>>  /*! \typedef XENBUS_STORE_ACQUIRE
>>      \brief Acquire a reference to the STORE interface
>>
>> @@ -247,10 +264,36 @@ typedef VOID
>>      IN  PINTERFACE  Interface
>>      );
>>
>> +/*! \typedef XENBUS_STORE_PERMISSIONS_SET
>> +    \brief Set permissions for a XenStore key
>> +
>> +    \param Interface The interface header
>> +    \param Transaction The transaction handle (NULL if this is not
>> +    part of a transaction)
>> +    \param Prefix An optional prefix for the \a Node
>> +    \param Node The concatenation of the \a Prefix and this value specifies
>> +    the XenStore key to set permissions of
>> +    \param Permissions An array of permissions to set
>> +    \param NumberPermissions Number of elements in the \a Permissions
>> array
>> + */
>> +typedef NTSTATUS
>> +(*XENBUS_STORE_PERMISSIONS_SET)(
>> +    IN  PINTERFACE                  Interface,
>> +    IN  PXENBUS_STORE_TRANSACTION   Transaction OPTIONAL,
>> +    IN  PCHAR                       Prefix OPTIONAL,
>> +    IN  PCHAR                       Node,
>> +    IN  PXENBUS_STORE_PERMISSION    Permissions,
>> +    IN  ULONG                       NumberPermissions
>> +    );
>> +
>>  // {86824C3B-D34E-4753-B281-2F1E3AD214D7}
>>  DEFINE_GUID(GUID_XENBUS_STORE_INTERFACE,
>>  0x86824c3b, 0xd34e, 0x4753, 0xb2, 0x81, 0x2f, 0x1e, 0x3a, 0xd2, 0x14, 0xd7);
>>
>> +/*! \struct _XENBUS_STORE_INTERFACE_V1
>> +    \brief STORE interface version 1
>> +    \ingroup interfaces
>> + */
>>  struct _XENBUS_STORE_INTERFACE_V1 {
>>      INTERFACE                       Interface;
>>      XENBUS_STORE_ACQUIRE            StoreAcquire;
>> @@ -267,11 +310,28 @@ struct _XENBUS_STORE_INTERFACE_V1 {
>>      XENBUS_STORE_POLL               StorePoll;
>>  };
>>
>> -/*! \struct _XENBUS_STORE_INTERFACE_V1
>> -    \brief STORE interface version 1
>> +/*! \struct _XENBUS_STORE_INTERFACE_V2
>> +    \brief STORE interface version 2
>>      \ingroup interfaces
>>  */
>> -typedef struct _XENBUS_STORE_INTERFACE_V1
>> XENBUS_STORE_INTERFACE, *PXENBUS_STORE_INTERFACE;
>> +struct _XENBUS_STORE_INTERFACE_V2 {
>> +    INTERFACE                       Interface;
>> +    XENBUS_STORE_ACQUIRE            StoreAcquire;
>> +    XENBUS_STORE_RELEASE            StoreRelease;
>> +    XENBUS_STORE_FREE               StoreFree;
>> +    XENBUS_STORE_READ               StoreRead;
>> +    XENBUS_STORE_PRINTF             StorePrintf;
>> +    XENBUS_STORE_REMOVE             StoreRemove;
>> +    XENBUS_STORE_DIRECTORY          StoreDirectory;
>> +    XENBUS_STORE_TRANSACTION_START  StoreTransactionStart;
>> +    XENBUS_STORE_TRANSACTION_END    StoreTransactionEnd;
>> +    XENBUS_STORE_WATCH_ADD          StoreWatchAdd;
>> +    XENBUS_STORE_WATCH_REMOVE       StoreWatchRemove;
>> +    XENBUS_STORE_POLL               StorePoll;
>> +    XENBUS_STORE_PERMISSIONS_SET    StorePermissionsSet;
>> +};
>> +
>> +typedef struct _XENBUS_STORE_INTERFACE_V2
>> XENBUS_STORE_INTERFACE, *PXENBUS_STORE_INTERFACE;
>>
>>  /*! \def XENBUS_STORE
>>      \brief Macro at assist in method invocation
>> @@ -282,7 +342,7 @@ typedef struct _XENBUS_STORE_INTERFACE_V1
>> XENBUS_STORE_INTERFACE, *PXENBUS_STORE
>>  #endif  // _WINDLL
>>
>>  #define XENBUS_STORE_INTERFACE_VERSION_MIN  1
>> -#define XENBUS_STORE_INTERFACE_VERSION_MAX  1
>> +#define XENBUS_STORE_INTERFACE_VERSION_MAX  2
>>
>>  #endif  // _XENBUS_STORE_INTERFACE_H
>>
>> diff --git a/src/xenbus/store.c b/src/xenbus/store.c
>> index 16ca37b..2642bb3 100644
>> --- a/src/xenbus/store.c
>> +++ b/src/xenbus/store.c
>> @@ -236,6 +236,73 @@ fail1:
>>      return status;
>>  }
>>
>> +// prepare a request with known number of elements
>> +static NTSTATUS
>> +StorePrepareRequestFixed(
>> +    IN  PXENBUS_STORE_CONTEXT       Context,
>> +    OUT PXENBUS_STORE_REQUEST       Request,
>> +    IN  PXENBUS_STORE_TRANSACTION   Transaction OPTIONAL,
>> +    IN  enum xsd_sockmsg_type       Type,
>> +    IN  PXENBUS_STORE_SEGMENT       Segments,
>> +    IN  ULONG                       NumberSegments
>> +    )
>> +{
>> +    ULONG                           Id;
>> +    KIRQL                           Irql;
>> +    NTSTATUS                        status;
>> +    ULONG                           Index;
>> +
>> +    ASSERT(IsZeroMemory(Request, sizeof (XENBUS_STORE_REQUEST)));
>> +
>> +    status = STATUS_INVALID_PARAMETER;
>> +    if (NumberSegments > XENBUS_STORE_REQUEST_SEGMENT_COUNT - 1)
>> // need one for the header
>> +        goto fail1;
>> +
>> +    if (Transaction != NULL) {
>> +        status = STATUS_UNSUCCESSFUL;
>> +        if (!Transaction->Active)
>> +            goto fail2;
>> +
>> +        Id = Transaction->Id;
>> +    } else {
>> +        Id = 0;
>> +    }
>> +
>> +    Request->Header.type = Type;
>> +    Request->Header.tx_id = Id;
>> +    Request->Header.len = 0;
>> +
>> +    KeAcquireSpinLock(&Context->Lock, &Irql);
>> +    Request->Header.req_id = Context->RequestId++;
>> +    KeReleaseSpinLock(&Context->Lock, Irql);
>> +
>> +    // header is the first, then the actual data
>> +    Request->Count = NumberSegments + 1;
>> +
>> +    Request->Segment[0].Data = (PCHAR)&Request->Header;
>> +    Request->Segment[0].Offset = 0;
>> +    Request->Segment[0].Length = sizeof(struct xsd_sockmsg);
>> +
> 
> I don't think you need this new function. See below...
> 
>> +    for (Index = 0; Index < NumberSegments; Index++) {
>> +        Request->Segment[Index+1].Data = Segments[Index].Data;
>> +        Request->Segment[Index+1].Offset = 0;
>> +        Request->Segment[Index+1].Length = Segments[Index].Length;
>> +
>> +        Request->Header.len += Segments[Index].Length;
>> +    }
>> +
>> +    Request->State = XENBUS_STORE_REQUEST_PREPARED;
>> +
>> +    return STATUS_SUCCESS;
>> +
>> +fail2:
>> +    Error("fail2\n");
>> +
>> +fail1:
>> +    Error("fail1 (%08x)\n", status);
>> +    return status;
>> +}
>> +
>>  static ULONG
>>  StoreCopyToRing(
>>      IN  PXENBUS_STORE_CONTEXT           Context,
>> @@ -439,7 +506,6 @@ StoreIgnoreHeaderType(
>>      case XS_RELEASE:
>>      case XS_GET_DOMAIN_PATH:
>>      case XS_MKDIR:
>> -    case XS_SET_PERMS:
>>      case XS_IS_DOMAIN_INTRODUCED:
>>      case XS_RESUME:
>>      case XS_SET_TARGET:
>> @@ -467,6 +533,7 @@ StoreVerifyHeader(
>>          Header->type != XS_TRANSACTION_END &&
>>          Header->type != XS_WRITE &&
>>          Header->type != XS_RM &&
>> +        Header->type != XS_SET_PERMS &&
>>          Header->type != XS_WATCH_EVENT &&
>>          Header->type != XS_ERROR &&
>>          !StoreIgnoreHeaderType(Header->type)) {
>> @@ -1788,6 +1855,157 @@ StorePoll(
>>      KeReleaseSpinLockFromDpcLevel(&Context->Lock);
>>  }
>>
>> +static NTSTATUS
>> +StorePermissionToString(
>> +    IN  PXENBUS_STORE_PERMISSION Permission,
>> +    IN  ULONG BufferSize,
>> +    OUT PCHAR Buffer
>> +    )
>> +{
>> +    NTSTATUS status = STATUS_INVALID_PARAMETER;
>> +
>> +    ASSERT(BufferSize > 1);
>> +
>> +    switch (Permission->Mask) {
>> +    case XS_PERM_WRITE:
>> +        *Buffer = 'w';
>> +        break;
>> +    case XS_PERM_READ:
>> +        *Buffer = 'r';
>> +        break;
>> +    case XS_PERM_READ | XS_PERM_WRITE:
>> +        *Buffer = 'b';
>> +        break;
>> +    case XS_PERM_NONE:
>> +        *Buffer = 'n';
>> +        break;
>> +    default:
>> +        goto fail1;
>> +    }
>> +
>> +    return RtlStringCbPrintfA(Buffer + 1, BufferSize - 1, "%u", Permission-
>>> Domain);
>> +
>> +fail1:
>> +    Error("fail1 (%08x)\n", status);
>> +    return status;
>> +}
>> +
>> +static NTSTATUS
>> +StorePermissionsSet(
>> +    IN  PINTERFACE                  Interface,
>> +    IN  PXENBUS_STORE_TRANSACTION   Transaction OPTIONAL,
>> +    IN  PCHAR                       Prefix OPTIONAL,
>> +    IN  PCHAR                       Node,
>> +    IN  PXENBUS_STORE_PERMISSION    Permissions,
>> +    IN  ULONG                       NumberPermissions
>> +    )
>> +{
>> +    PXENBUS_STORE_CONTEXT           Context = Interface->Context;
>> +    XENBUS_STORE_REQUEST            Request;
>> +    PXENBUS_STORE_RESPONSE          Response;
>> +    NTSTATUS                        status;
>> +    XENBUS_STORE_SEGMENT
>> Segments[XENBUS_STORE_REQUEST_SEGMENT_COUNT];
>> +    ULONG                           Index, BufferSize;
>> +    PCHAR                           Path = NULL;
>> +
>> +    status = STATUS_INVALID_PARAMETER;
>> +    if (NumberPermissions > XENBUS_STORE_REQUEST_SEGMENT_COUNT -
>> 2) // 1 for path, 1 for header in StorePrepareRequestFixed
>> +        goto fail1;
>> +
>> +    if (Prefix != NULL) {
>> +        // we're concatenating it here instead of passing to
>> StorePrepareRequestFixed to reduce the number of segments used
>> +        status = STATUS_NO_MEMORY;
>> +        Path = __StoreAllocate(XENSTORE_ABS_PATH_MAX);
>> +        if (Path == NULL)
>> +            goto fail2;
>> +
>> +        status = RtlStringCbPrintfA(Path, XENSTORE_ABS_PATH_MAX,
>> "%s/%s", Prefix, Node);
>> +        ASSERT(NT_SUCCESS(status));
>> +        Node = Path;
>> +    }
>> +
>> +    RtlZeroMemory(&Request, sizeof(XENBUS_STORE_REQUEST));
>> +    RtlZeroMemory(Segments, sizeof(Segments));
>> +
>> +    Segments[0].Data = Node; // path
>> +    Segments[0].Offset = 0;
>> +    Segments[0].Length = (ULONG)strlen(Node) + 1; // zero terminator
>> required
>> +
>> +    BufferSize = 16;
>> +    for (Index = 0; Index < NumberPermissions; Index++) {
>> +        Segments[Index + 1].Data = __StoreAllocate(BufferSize);
>> +        if (Segments[Index + 1].Data == NULL)
>> +            goto fail3;
>> +
>> +        status = StorePermissionToString(&Permissions[Index], BufferSize,
>> Segments[Index+1].Data);
>> +        if (!NT_SUCCESS(status))
>> +            goto fail4;
>> +
>> +        Segments[Index + 1].Length = (ULONG)strlen(Segments[Index +
>> 1].Data) + 1; // zero terminator required
>> +    }
>> +
>> +    status = StorePrepareRequestFixed(Context,
>> +                                      &Request,
>> +                                      Transaction,
>> +                                      XS_SET_PERMS,
>> +                                      Segments,
>> +                                      NumberPermissions + 1);
>> +
> 
> Segments are only a coding convenience... On the 'wire' they are just 
> concatenated. So I think all you need to is prepare character buffer with a 
> NUL separated list of <perm><domid> strings and then pass that as an arg to 
> the normal ellipsis StorePrepareRequest(). No need for the extra complexity 
> of a new StorePrepareRequestFixed().

That makes sense, when I was writing this I just deferred to how other store 
calls work.
> 
>   Paul
> 
>> +    if (!NT_SUCCESS(status))
>> +        goto fail5;
>> +
>> +    Response = StoreSubmitRequest(Context, &Request);
>> +
>> +    status = STATUS_NO_MEMORY;
>> +    if (Response == NULL)
>> +        goto fail6;
>> +
>> +    status = StoreCheckResponse(Response);
>> +    if (!NT_SUCCESS(status))
>> +        goto fail7;
>> +
>> +    StoreFreeResponse(Response);
>> +    ASSERT(IsZeroMemory(&Request, sizeof(XENBUS_STORE_REQUEST)));
>> +    for (Index = 0; Index < NumberPermissions; Index++)
>> +        __StoreFree(Segments[Index + 1].Data);
>> +
>> +    if (Path != NULL)
>> +        __StoreFree(Path);
>> +
>> +    return STATUS_SUCCESS;
>> +
>> +fail7:
>> +    Error("fail7\n");
>> +    StoreFreeResponse(Response);
>> +
>> +fail6:
>> +    Error("fail6\n");
>> +
>> +fail5:
>> +    Error("fail5\n");
>> +
>> +fail4:
>> +    Error("fail4\n");
>> +
>> +fail3:
>> +    Error("fail3\n");
>> +    for (Index = 0; Index < NumberPermissions; Index++)
>> +        if (Segments[Index + 1].Data != NULL)
>> +            __StoreFree(Segments[Index + 1].Data);
>> +
>> +    if (Path != NULL)
>> +        __StoreFree(Path);
>> +
>> +fail2:
>> +    Error("fail2\n");
>> +
>> +
>> +fail1:
>> +    Error("fail1 (%08x)\n", status);
>> +    ASSERT(IsZeroMemory(&Request, sizeof(XENBUS_STORE_REQUEST)));
>> +    return status;
>> +}
>> +
>>  static
>>  _Function_class_(KSERVICE_ROUTINE)
>>  _IRQL_requires_(HIGH_LEVEL)
>> @@ -2285,6 +2503,23 @@ static struct _XENBUS_STORE_INTERFACE_V1
>> StoreInterfaceVersion1 = {
>>      StorePoll
>>  };
>>
>> +static struct _XENBUS_STORE_INTERFACE_V2 StoreInterfaceVersion2 = {
>> +    { sizeof(struct _XENBUS_STORE_INTERFACE_V2), 2, NULL, NULL, NULL },
>> +    StoreAcquire,
>> +    StoreRelease,
>> +    StoreFree,
>> +    StoreRead,
>> +    StorePrintf,
>> +    StoreRemove,
>> +    StoreDirectory,
>> +    StoreTransactionStart,
>> +    StoreTransactionEnd,
>> +    StoreWatchAdd,
>> +    StoreWatchRemove,
>> +    StorePoll,
>> +    StorePermissionsSet,
>> +};
>> +
>>  NTSTATUS
>>  StoreInitialize(
>>      IN  PXENBUS_FDO             Fdo,
>> @@ -2384,6 +2619,23 @@ StoreGetInterface(
>>          status = STATUS_SUCCESS;
>>          break;
>>      }
>> +    case 2: {
>> +        struct _XENBUS_STORE_INTERFACE_V2  *StoreInterface;
>> +
>> +        StoreInterface = (struct _XENBUS_STORE_INTERFACE_V2 *)Interface;
>> +
>> +        status = STATUS_BUFFER_OVERFLOW;
>> +        if (Size < sizeof(struct _XENBUS_STORE_INTERFACE_V2))
>> +            break;
>> +
>> +        *StoreInterface = StoreInterfaceVersion2;
>> +
>> +        ASSERT3U(Interface->Version, == , Version);
>> +        Interface->Context = Context;
>> +
>> +        status = STATUS_SUCCESS;
>> +        break;
>> +    }
>>      default:
>>          status = STATUS_NOT_SUPPORTED;
>>          break;
>>
>> _______________________________________________
>> win-pv-devel mailing list
>> win-pv-devel@xxxxxxxxxxxxxxxxxxxx
>> http://lists.xenproject.org/cgi-bin/mailman/listinfo/win-pv-devel


-- 
RafaÅ WojdyÅa
Qubes Tools for Windows developer
https://www.qubes-os.org/

_______________________________________________
win-pv-devel mailing list
win-pv-devel@xxxxxxxxxxxxxxxxxxxx
http://lists.xenproject.org/cgi-bin/mailman/listinfo/win-pv-devel