Re: [XTF PATCH v2] Add a Live Patch privilege check test

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Fri, Nov 18, 2016 at 09:36:06AM +0000, Ross Lagerwall wrote:
> Add a test to check that Live Patch operations cannot be called from an
> unprivileged domain.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
> 
> In v2:
> * Fixed review issues.
> * Rebased and added a test_title string.
> 
>  common/lib.c                        |  15 +++
>  docs/all-tests.dox                  |   2 +
>  include/xen/sysctl.h                | 218 ++++++++++++++++++++++++++++++++++++
>  include/xtf/hypercall.h             |   6 +
>  include/xtf/lib.h                   |   2 +
>  tests/livepatch-priv-check/Makefile |   9 ++
>  tests/livepatch-priv-check/main.c   | 153 +++++++++++++++++++++++++
>  7 files changed, 405 insertions(+)
>  create mode 100755 include/xen/sysctl.h
>  create mode 100644 tests/livepatch-priv-check/Makefile
>  create mode 100644 tests/livepatch-priv-check/main.c
> 
> diff --git a/common/lib.c b/common/lib.c
> index 9dca3e3..cc847ab 100644
> --- a/common/lib.c
> +++ b/common/lib.c
> @@ -19,6 +19,21 @@ void __noreturn panic(const char *fmt, ...)
>      arch_crash_hard();
>  }
>  
> +int probe_sysctl_interface_version(void)

/me chuckles.
> +{
> +    int i;
> +    xen_sysctl_t op = {0};
> +
> +    for ( i = 0; i < 128; i++ )
> +    {
> +        op.interface_version = i;
> +        if ( hypercall_sysctl(&op) != -EACCES )
> +            return i;
> +    }
> +
> +    return -1;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/docs/all-tests.dox b/docs/all-tests.dox
> index 4f86182..2909b85 100644
> --- a/docs/all-tests.dox
> +++ b/docs/all-tests.dox
> @@ -22,6 +22,8 @@ and functionality.
>  
>  @subpage test-invlpg - `invlpg` instruction behaviour.
>  
> +@subpage test-livepatch-priv-check - Live Patch Privilege Check
> +
>  @subpage test-pv-iopl - IOPL emulation for PV guests.
>  
>  @subpage test-swint-emulation - Software interrupt emulation for HVM guests.
> diff --git a/include/xen/sysctl.h b/include/xen/sysctl.h
> new file mode 100755
> index 0000000..f4006fb
> --- /dev/null
> +++ b/include/xen/sysctl.h
> @@ -0,0 +1,218 @@
> +/******************************************************************************
> + * sysctl.h
> + *
> + * System management operations. For use by node control stack.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to
> + * deal in the Software without restriction, including without limitation the
> + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
> + * sell copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + *
> + * Copyright (c) 2002-2006, K Fraser
> + */
> +
> +#ifndef __XEN_PUBLIC_SYSCTL_H__
> +#define __XEN_PUBLIC_SYSCTL_H__
> +
> +#include "xen.h"
> +#include "physdev.h"
> +
> +typedef struct {
> +    union {
> +        void *p;
> +        uint64_t __attribute__((aligned(8))) q;
> +    };
> +} guest_handle_64;
> +
> +/*
> + * XEN_SYSCTL_LIVEPATCH_op
> + *
> + * Refer to the docs/unstable/misc/livepatch.markdown
> + * for the design details of this hypercall.
> + *
> + * There are four sub-ops:
> + *  XEN_SYSCTL_LIVEPATCH_UPLOAD (0)
> + *  XEN_SYSCTL_LIVEPATCH_GET (1)
> + *  XEN_SYSCTL_LIVEPATCH_LIST (2)
> + *  XEN_SYSCTL_LIVEPATCH_ACTION (3)
> + *
> + * The normal sequence of sub-ops is to:
> + *  1) XEN_SYSCTL_LIVEPATCH_UPLOAD to upload the payload. If errors STOP.
> + *  2) XEN_SYSCTL_LIVEPATCH_GET to check the `->rc`. If -XEN_EAGAIN spin.
> + *     If zero go to next step.
> + *  3) XEN_SYSCTL_LIVEPATCH_ACTION with LIVEPATCH_ACTION_APPLY to apply the patch.
> + *  4) XEN_SYSCTL_LIVEPATCH_GET to check the `->rc`. If in -XEN_EAGAIN spin.
> + *     If zero exit with success.
> + */
> +
> +#define LIVEPATCH_PAYLOAD_VERSION 1
> +/*
> + * Structure describing an ELF payload. Uniquely identifies the
> + * payload. Should be human readable.
> + * Recommended length is upto XEN_LIVEPATCH_NAME_SIZE.
> + * Includes the NUL terminator.
> + */
> +#define XEN_LIVEPATCH_NAME_SIZE 128
> +struct xen_livepatch_name {
> +    guest_handle_64 name;                   /* IN: pointer to name. */
> +    uint16_t size;                          /* IN: size of name. May be upto
> +                                               XEN_LIVEPATCH_NAME_SIZE. */
> +    uint16_t pad[3];                        /* IN: MUST be zero. */
> +};
> +typedef struct xen_livepatch_name xen_livepatch_name_t;
> +
> +/*
> + * Upload a payload to the hypervisor. The payload is verified
> + * against basic checks and if there are any issues the proper return code
> + * will be returned. The payload is not applied at this time - that is
> + * controlled by XEN_SYSCTL_LIVEPATCH_ACTION.
> + *
> + * The return value is zero if the payload was succesfully uploaded.
> + * Otherwise an EXX return value is provided. Duplicate `name` are not
> + * supported.
> + *
> + * The payload at this point is verified against basic checks.
> + *
> + * The `payload` is the ELF payload as mentioned in the `Payload format`
> + * section in the Live Patch design document.
> + */
> +#define XEN_SYSCTL_LIVEPATCH_UPLOAD 0
> +struct xen_sysctl_livepatch_upload {
> +    xen_livepatch_name_t name;              /* IN, name of the patch. */
> +    uint64_t size;                          /* IN, size of the ELF file. */
> +    guest_handle_64 payload;                /* IN, the ELF file. */
> +};
> +typedef struct xen_sysctl_livepatch_upload xen_sysctl_livepatch_upload_t;
> +
> +/*
> + * Retrieve an status of an specific payload.
> + *
> + * Upon completion the `struct xen_livepatch_status` is updated.
> + *
> + * The return value is zero on success and XEN_EXX on failure. This operation
> + * is synchronous and does not require preemption.
> + */
> +#define XEN_SYSCTL_LIVEPATCH_GET 1
> +
> +struct xen_livepatch_status {
> +#define LIVEPATCH_STATE_CHECKED      1
> +#define LIVEPATCH_STATE_APPLIED      2
> +    uint32_t state;                /* OUT: LIVEPATCH_STATE_*. */
> +    int32_t rc;                    /* OUT: 0 if no error, otherwise -XEN_EXX. */
> +};
> +typedef struct xen_livepatch_status xen_livepatch_status_t;
> +
> +struct xen_sysctl_livepatch_get {
> +    xen_livepatch_name_t name;              /* IN, name of the payload. */
> +    xen_livepatch_status_t status;          /* IN/OUT, state of it. */
> +};
> +typedef struct xen_sysctl_livepatch_get xen_sysctl_livepatch_get_t;
> +
> +/*
> + * Retrieve an array of abbreviated status and names of payloads that are
> + * loaded in the hypervisor.
> + *
> + * If the hypercall returns an positive number, it is the number (up to `nr`)
> + * of the payloads returned, along with `nr` updated with the number of remaining
> + * payloads, `version` updated (it may be the same across hypercalls. If it
> + * varies the data is stale and further calls could fail). The `status`,
> + * `name`, and `len`' are updated at their designed index value (`idx`) with
> + * the returned value of data.
> + *
> + * If the hypercall returns E2BIG the `nr` is too big and should be
> + * lowered. The upper limit of `nr` is left to the implemention.
> + *
> + * Note that due to the asynchronous nature of hypercalls the domain might have
> + * added or removed the number of payloads making this information stale. It is
> + * the responsibility of the toolstack to use the `version` field to check
> + * between each invocation. if the version differs it should discard the stale
> + * data and start from scratch. It is OK for the toolstack to use the new
> + * `version` field.
> + */
> +#define XEN_SYSCTL_LIVEPATCH_LIST 2
> +struct xen_sysctl_livepatch_list {
> +    uint32_t version;                       /* OUT: Hypervisor stamps value.
> +                                               If varies between calls, we are
> +                                             * getting stale data. */
> +    uint32_t idx;                           /* IN: Index into hypervisor list. */
> +    uint32_t nr;                            /* IN: How many status, name, and len
> +                                               should fill out. Can be zero to get
> +                                               amount of payloads and version.
> +                                               OUT: How many payloads left. */
> +    uint32_t pad;                           /* IN: Must be zero. */
> +    guest_handle_64                status;  /* OUT. Must have enough
> +                                               space allocate for nr of them. */
> +    guest_handle_64 name;                   /* OUT: Array of names. Each member
> +                                               MUST XEN_LIVEPATCH_NAME_SIZE in size.
> +                                               Must have nr of them. */
> +    guest_handle_64 len;                    /* OUT: Array of lengths of name's.
> +                                               Must have nr of them. */
> +};
> +typedef struct xen_sysctl_livepatch_list xen_sysctl_livepatch_list_t;
> +
> +/*
> + * Perform an operation on the payload structure referenced by the `name` field.
> + * The operation request is asynchronous and the status should be retrieved
> + * by using either XEN_SYSCTL_LIVEPATCH_GET or XEN_SYSCTL_LIVEPATCH_LIST hypercall.
> + */
> +#define XEN_SYSCTL_LIVEPATCH_ACTION 3
> +struct xen_sysctl_livepatch_action {
> +    xen_livepatch_name_t name;              /* IN, name of the patch. */
> +#define LIVEPATCH_ACTION_UNLOAD       1
> +#define LIVEPATCH_ACTION_REVERT       2
> +#define LIVEPATCH_ACTION_APPLY        3
> +#define LIVEPATCH_ACTION_REPLACE      4
> +    uint32_t cmd;                           /* IN: LIVEPATCH_ACTION_*. */
> +    uint32_t timeout;                       /* IN: Zero if no timeout. */
> +                                            /* Or upper bound of time (ms) */
> +                                            /* for operation to take. */
> +};
> +typedef struct xen_sysctl_livepatch_action xen_sysctl_livepatch_action_t;
> +
> +struct xen_sysctl_livepatch_op {
> +    uint32_t cmd;                           /* IN: XEN_SYSCTL_LIVEPATCH_*. */
> +    uint32_t pad;                           /* IN: Always zero. */
> +    union {
> +        xen_sysctl_livepatch_upload_t upload;
> +        xen_sysctl_livepatch_list_t list;
> +        xen_sysctl_livepatch_get_t get;
> +        xen_sysctl_livepatch_action_t action;
> +    } u;
> +};
> +typedef struct xen_sysctl_livepatch_op xen_sysctl_livepatch_op_t;
> +
> +struct xen_sysctl {
> +    uint32_t cmd;
> +#define XEN_SYSCTL_livepatch_op                  27
> +    uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
> +    union {
> +        struct xen_sysctl_livepatch_op      livepatch;
> +        uint8_t                             pad[128];
> +    } u;
> +};
> +typedef struct xen_sysctl xen_sysctl_t;
> +
> +#endif /* __XEN_PUBLIC_SYSCTL_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/include/xtf/hypercall.h b/include/xtf/hypercall.h
> index a37ebc2..ab4986f 100644
> --- a/include/xtf/hypercall.h
> +++ b/include/xtf/hypercall.h
> @@ -34,6 +34,7 @@ extern uint8_t hypercall_page[PAGE_SIZE];
>  #include <xen/physdev.h>
>  #include <xen/memory.h>
>  #include <xen/version.h>
> +#include <xen/sysctl.h>
>  #include <xen/hvm/hvm_op.h>
>  #include <xen/hvm/params.h>
>  
> @@ -114,6 +115,11 @@ static inline long hypercall_hvm_op(unsigned int cmd, void *arg)
>      return HYPERCALL2(long, __HYPERVISOR_hvm_op, cmd, arg);
>  }
>  
> +static inline long hypercall_sysctl(xen_sysctl_t *arg)
> +{
> +    return HYPERCALL1(long, __HYPERVISOR_sysctl, arg);
> +}
> +
>  /*
>   * Higher level hypercall helpers
>   */
> diff --git a/include/xtf/lib.h b/include/xtf/lib.h
> index b0ae39d..8fb490a 100644
> --- a/include/xtf/lib.h
> +++ b/include/xtf/lib.h
> @@ -65,6 +65,8 @@ static inline void exec_user_void(void (*fn)(void))
>      exec_user((void *)fn);
>  }
>  
> +int probe_sysctl_interface_version(void);
> +
>  #endif /* XTF_LIB_H */
>  
>  /*
> diff --git a/tests/livepatch-priv-check/Makefile b/tests/livepatch-priv-check/Makefile
> new file mode 100644
> index 0000000..e27b9da
> --- /dev/null
> +++ b/tests/livepatch-priv-check/Makefile
> @@ -0,0 +1,9 @@
> +include $(ROOT)/build/common.mk
> +
> +NAME      := livepatch-priv-check
> +CATEGORY  := functional
> +TEST-ENVS := $(ALL_ENVIRONMENTS)
> +
> +obj-perenv += main.o
> +
> +include $(ROOT)/build/gen.mk
> diff --git a/tests/livepatch-priv-check/main.c b/tests/livepatch-priv-check/main.c
> new file mode 100644
> index 0000000..4d64d58
> --- /dev/null
> +++ b/tests/livepatch-priv-check/main.c
> @@ -0,0 +1,153 @@
> +/**
> + * @file tests/livepatch-priv-check/main.c
> + * @ref test-livepatch-priv-check
> + *
> + * @page test-livepatch-priv-check Live Patch Privilege Check
> + *
> + * Checks that Xen prevents using all live patching operations and
> + * sub-operations from an unprivileged guest.
> + *
> + * @see tests/livepatch-check-priv/main.c
> + */
> +#include <xtf.h>
> +
> +const char test_title[] = "Live Patch Privilege Check";
> +
> +static int interface_version;
> +
> +static void check_ret(int rc)

Would it make it easier (to troubleshoot if somebody did enable
the XSM checks for specifc sub-obs - if that was ever done), to
extend the funtion to have 'const char *msg'
> +{
> +    switch ( rc )
> +    {
> +    case -EPERM:
> +        printk("Xen correctly denied Live Patch calls\n");

And in all of those printk and xtf_* use the '%s: ... ", msg.
> +        break;
> +
> +    case -ENOSYS:
> +        xtf_skip("Live Patch support not detected\n");
> +        break;
> +
> +    default:
> +        xtf_failure("Fail: Unexpected return code %d\n", rc);
> +        break;
> +    }
> +}
> +
> +#define TEST_NAME "foo"
> +
> +static void test_upload(void)
> +{
> +    uint8_t payload[PAGE_SIZE];
> +    xen_sysctl_t op =
> +    {
> +        .cmd = XEN_SYSCTL_livepatch_op,
> +        .interface_version = interface_version,
> +        .u.livepatch = {
> +            .cmd = XEN_SYSCTL_LIVEPATCH_UPLOAD,
> +            .u.upload = {
> +                .name = {
> +                    .name.p = TEST_NAME,
> +                    .size = sizeof(TEST_NAME),
> +                },
> +                .size = PAGE_SIZE,
> +                .payload.p = payload,
> +            },
> +        },
> +    };
> +
> +    check_ret(hypercall_sysctl(&op));

And here in you could just do check_ret(__func__, hypercall_sysctl?)

Thanks for making a nice test-case for this!!


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