Re: [Xen-devel] [XTF PATCH] Add a Live Patch privilege check test

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 17/11/16 15:40, Ross Lagerwall wrote:
> +typedef struct {
> +    union {
> +        void *p;
> +        uint64_t __attribute__((aligned(8))) q;
> +    };
> +} guest_handle_64;
> +
> +#define XEN_SYSCTL_INTERFACE_VERSION 0x0000000E

/sigh

This is going to require some rethinking.  We cannot have the unstable
ABI versions baked into the header files, as the tests wont function on
older versions of Xen.

The sysctl ABI is actually fixed.  The struct is always 136 bytes long,
and has two uint32_t's at the top.  XTF needs a library function to
probe which interface version is accepted by Xen.

The livepatch layout is also fixed, so the sysctl version isn't actually
relevant.

> diff --git a/include/xtf/hypercall.h b/include/xtf/hypercall.h
> index a37ebc2..75c0e9d 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(void *arg)

xen_sysctl_t * please.

> +{
> +    return HYPERCALL1(long, __HYPERVISOR_sysctl, arg);
> +}
> +
>  /*
>   * Higher level hypercall helpers
>   */
> 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..357e4da
> --- /dev/null
> +++ b/tests/livepatch-priv-check/main.c
> @@ -0,0 +1,144 @@
> +/**
> + * @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>
> +
> +static void check_ret(int rc)
> +{
> +    switch ( rc )
> +    {
> +    case -EPERM:
> +        xtf_success("Xen correctly denied Live Patch calls\n");
> +        break;

Newlines after breaks please.

Also, a quirk of multiple sub-tests like this is that you shouldn't call
xtf_success multiple times.  It can end up incorrectly declaring success
if a logical bug caused you to skip part of the test logic.

Use printk here, and have a single xtf_success() as the final action in
test_main().

> +    case -ENOSYS:
> +        xtf_skip("Live Patch support not detected\n");
> +        break;
> +    case 0:
> +        xtf_failure("Live Patch call succeeded\n");
> +        break;

I find it is helpful to manually prefix the strings with "Fail: ".  It
helps visually distinguish the logging if there is a mix of steps with
different results.

> +    default:
> +        xtf_failure("Unexpected error %d\n", rc);
> +        break;

This should be xtf_error().

~Andrew

> +    }
> +}
> +
>


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