[kvm-unit-tests PATCH v2 0/4] s390x: Attestation tests

[kvm-unit-tests PATCH v2 0/4] s390x: Attestation tests

[Steffen Eiden]

This series adds some test in s390x/uv-guest.c verifying error paths of the
Request Attestation Measurement UVC.
Also adds a test in s390x/uv-host.c to verify that the
Request Attestation Measurement UVC cannot be called in guest1.

v1->v2:
  * renamed 'uv_get_info(void)' to 'uv_get_query_data(void)'
  * renamed various fields in 'struct uv_arcb_v1'
  * added a test for invalid additional size
  * added r-b from Janosch in PATCH 1/4
  * added r-b from Janosch in PATCH 3/4

Steffen Eiden (4):
  s390x: uv-host: Add attestation test
  s390x: lib: Add QUI getter
  s390x: uv-guest: remove duplicated checks
  s390x: uv-guest: Add attestation tests

 lib/s390x/asm/uv.h |  28 ++++++-
 lib/s390x/uv.c     |   8 ++
 lib/s390x/uv.h     |   1 +
 s390x/uv-guest.c   | 196 +++++++++++++++++++++++++++++++++++++++++----
 s390x/uv-host.c    |   1 +
 5 files changed, 216 insertions(+), 18 deletions(-)

-- 
2.30.2

[kvm-unit-tests PATCH v2 1/4] s390x: uv-host: Add attestation test

[Steffen Eiden]

Adds an invalid command test for attestation in the uv-host.

Signed-off-by: Steffen Eiden <seiden@linux.ibm.com>
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/asm/uv.h | 23 ++++++++++++++++++++++-
 s390x/uv-host.c    |  1 +
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
index 97c90e81..7afbcffd 100644
--- a/lib/s390x/asm/uv.h
+++ b/lib/s390x/asm/uv.h
@@ -1,7 +1,7 @@
 /*
  * s390x Ultravisor related definitions
  *
- * Copyright (c) 2020 IBM Corp
+ * Copyright IBM Corp. 2020, 2022
  *
  * Authors:
  *  Janosch Frank <frankja@linux.ibm.com>
@@ -47,6 +47,7 @@
 #define UVC_CMD_UNPIN_PAGE_SHARED	0x0342
 #define UVC_CMD_SET_SHARED_ACCESS	0x1000
 #define UVC_CMD_REMOVE_SHARED_ACCESS	0x1001
+#define UVC_CMD_ATTESTATION		0x1020
 
 /* Bits in installed uv calls */
 enum uv_cmds_inst {
@@ -71,6 +72,7 @@ enum uv_cmds_inst {
 	BIT_UVC_CMD_UNSHARE_ALL = 20,
 	BIT_UVC_CMD_PIN_PAGE_SHARED = 21,
 	BIT_UVC_CMD_UNPIN_PAGE_SHARED = 22,
+	BIT_UVC_CMD_ATTESTATION = 28,
 };
 
 struct uv_cb_header {
@@ -178,6 +180,25 @@ struct uv_cb_cfs {
 	u64 paddr;
 }  __attribute__((packed))  __attribute__((aligned(8)));
 
+/* Retrieve Attestation Measurement */
+struct uv_cb_attest {
+	struct uv_cb_header header;	/* 0x0000 */
+	u64 reserved08[2];		/* 0x0008 */
+	u64 arcb_addr;			/* 0x0018 */
+	u64 continuation_token;		/* 0x0020 */
+	u8  reserved28[6];		/* 0x0028 */
+	u16 user_data_length;		/* 0x002e */
+	u8  user_data[256];		/* 0x0030 */
+	u32 reserved130[3];		/* 0x0130 */
+	u32 measurement_length;		/* 0x013c */
+	u64 measurement_address;	/* 0x0140 */
+	u8 config_uid[16];		/* 0x0148 */
+	u32 reserved158;		/* 0x0158 */
+	u32 add_data_length;		/* 0x015c */
+	u64 add_data_address;		/* 0x0160 */
+	u64 reserved168[4];		/* 0x0168 */
+}  __attribute__((packed))  __attribute__((aligned(8)));
+
 /* Set Secure Config Parameter */
 struct uv_cb_ssc {
 	struct uv_cb_header header;
diff --git a/s390x/uv-host.c b/s390x/uv-host.c
index 92a41069..946f031e 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -418,6 +418,7 @@ static struct cmd_list invalid_cmds[] = {
 	{ "bogus", 0x4242, sizeof(struct uv_cb_header), -1},
 	{ "share", UVC_CMD_SET_SHARED_ACCESS, sizeof(struct uv_cb_share), BIT_UVC_CMD_SET_SHARED_ACCESS },
 	{ "unshare", UVC_CMD_REMOVE_SHARED_ACCESS, sizeof(struct uv_cb_share), BIT_UVC_CMD_REMOVE_SHARED_ACCESS },
+	{ "attest", UVC_CMD_ATTESTATION, sizeof(struct uv_cb_attest), BIT_UVC_CMD_ATTESTATION },
 	{ NULL, 0, 0 },
 };
 
-- 
2.30.2

[kvm-unit-tests PATCH v2 2/4] s390x: lib: Add QUI getter

[Steffen Eiden]

Some tests need the information provided by the QUI UVC and lib/s390x/uv.c
already has cached the qui result. Let's add a function to avoid
unnecessary QUI UVCs.

Signed-off-by: Steffen Eiden <seiden@linux.ibm.com>
---
 lib/s390x/uv.c | 8 ++++++++
 lib/s390x/uv.h | 1 +
 2 files changed, 9 insertions(+)

diff --git a/lib/s390x/uv.c b/lib/s390x/uv.c
index 6fe11dff..602cbbfc 100644
--- a/lib/s390x/uv.c
+++ b/lib/s390x/uv.c
@@ -47,6 +47,14 @@ bool uv_query_test_call(unsigned int nr)
 	return test_bit_inv(nr, uvcb_qui.inst_calls_list);
 }
 
+const struct uv_cb_qui *uv_get_query_data(void)
+{
+	/* Query needs to be called first */
+	assert(uvcb_qui.header.rc);
+
+	return &uvcb_qui;
+}
+
 int uv_setup(void)
 {
 	if (!test_facility(158))
diff --git a/lib/s390x/uv.h b/lib/s390x/uv.h
index 8175d9c6..44264861 100644
--- a/lib/s390x/uv.h
+++ b/lib/s390x/uv.h
@@ -8,6 +8,7 @@
 bool uv_os_is_guest(void);
 bool uv_os_is_host(void);
 bool uv_query_test_call(unsigned int nr);
+const struct uv_cb_qui *uv_get_query_data(void);
 void uv_init(void);
 int uv_setup(void);
 void uv_create_guest(struct vm *vm);
-- 
2.30.2

[kvm-unit-tests PATCH v2 3/4] s390x: uv-guest: remove duplicated checks

[Steffen Eiden]

Removing some tests which are done at other points in the code
implicitly.

In lib/s390x/uc.c#setup_uv(void) the rc of the qui result is verified
using asserts.
The whole test is fenced by lib/s390x/uc.c#os_is_guest(void) that
checks if SET and REMOVE SHARED is present.

Signed-off-by: Steffen Eiden <seiden@linux.ibm.com>
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/uv-guest.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/s390x/uv-guest.c b/s390x/uv-guest.c
index 44ad2154..97ae4687 100644
--- a/s390x/uv-guest.c
+++ b/s390x/uv-guest.c
@@ -69,23 +69,15 @@ static void test_query(void)
 	cc = uv_call(0, (u64)&uvcb);
 	report(cc == 1 && uvcb.header.rc == UVC_RC_INV_LEN, "length");
 
-	uvcb.header.len = sizeof(uvcb);
-	cc = uv_call(0, (u64)&uvcb);
-	report((!cc && uvcb.header.rc == UVC_RC_EXECUTED) ||
-	       (cc == 1 && uvcb.header.rc == 0x100),
-		"successful query");
-
 	/*
-	 * These bits have been introduced with the very first
-	 * Ultravisor version and are expected to always be available
-	 * because they are basic building blocks.
+	 * BIT_UVC_CMD_QUI, BIT_UVC_CMD_SET_SHARED_ACCESS and
+	 * BIT_UVC_CMD_SET_SHARED_ACCESS are always present as they
+	 * have been introduced with the first Ultravisor version.
+	 * However, we only need to check for QUI as
+	 * SET/REMOVE SHARED are used to fence this test to be only
+	 * executed by protected guests.
 	 */
-	report(test_bit_inv(BIT_UVC_CMD_QUI, &uvcb.inst_calls_list[0]),
-	       "query indicated");
-	report(test_bit_inv(BIT_UVC_CMD_SET_SHARED_ACCESS, &uvcb.inst_calls_list[0]),
-	       "share indicated");
-	report(test_bit_inv(BIT_UVC_CMD_REMOVE_SHARED_ACCESS, &uvcb.inst_calls_list[0]),
-	       "unshare indicated");
+	report(uv_query_test_call(BIT_UVC_CMD_QUI), "query indicated");
 	report_prefix_pop();
 }
 
-- 
2.30.2

[kvm-unit-tests PATCH v2 4/4] s390x: uv-guest: Add attestation tests

[Steffen Eiden]

Adds several tests to verify correct error paths of attestation.

Signed-off-by: Steffen Eiden <seiden@linux.ibm.com>
---
 lib/s390x/asm/uv.h |   5 +-
 s390x/uv-guest.c   | 174 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 177 insertions(+), 2 deletions(-)

diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
index 7afbcffd..7fe55052 100644
--- a/lib/s390x/asm/uv.h
+++ b/lib/s390x/asm/uv.h
@@ -108,7 +108,10 @@ struct uv_cb_qui {
 	u8  reserved88[158 - 136];	/* 0x0088 */
 	uint16_t max_guest_cpus;	/* 0x009e */
 	u64 uv_feature_indications;	/* 0x00a0 */
-	u8  reserveda8[200 - 168];	/* 0x00a8 */
+	u8  reserveda8[224 - 168];	/* 0x00a8 */
+	u64 supported_att_hdr_versions;	/* 0x00e0 */
+	u64 supported_paf;		/* 0x00e8 */
+	u8  reservedf0[256 - 240];	/* 0x00f0 */
 }  __attribute__((packed))  __attribute__((aligned(8)));
 
 struct uv_cb_cgc {
diff --git a/s390x/uv-guest.c b/s390x/uv-guest.c
index 97ae4687..3fca9d21 100644
--- a/s390x/uv-guest.c
+++ b/s390x/uv-guest.c
@@ -2,10 +2,11 @@
 /*
  * Guest Ultravisor Call tests
  *
- * Copyright (c) 2020 IBM Corp
+ * Copyright IBM Corp. 2020, 2022
  *
  * Authors:
  *  Janosch Frank <frankja@linux.ibm.com>
+ *  Steffen Eiden <seiden@linux.ibm.com>
  */
 
 #include <libcflat.h>
@@ -53,6 +54,15 @@ static void test_priv(void)
 	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
 	report_prefix_pop();
 
+	report_prefix_push("attest");
+	uvcb.cmd = UVC_CMD_ATTESTATION;
+	uvcb.len = sizeof(struct uv_cb_attest);
+	expect_pgm_int();
+	enter_pstate();
+	uv_call_once(0, (u64)&uvcb);
+	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
+	report_prefix_pop();
+
 	report_prefix_pop();
 }
 
@@ -111,7 +121,168 @@ static void test_sharing(void)
 	cc = uv_call(0, (u64)&uvcb);
 	report(cc == 0 && uvcb.header.rc == UVC_RC_EXECUTED, "unshare");
 	report_prefix_pop();
+}
+
+#define ARCB_VERSION_1 0x0100
+#define ARCB_HMAC_SHA512 1
+/* arcb with one key slot and no nonce */
+struct uv_arcb_v1 {
+	uint64_t	reserved0;		/* 0x0000 */
+	uint32_t	req_ver;		/* 0x0008 */
+	uint32_t	req_len;		/* 0x000c */
+	uint8_t		iv[12];			/* 0x0010 */
+	uint32_t	reserved1c;		/* 0x001c */
+	uint8_t		reserved20[7];		/* 0x0020 */
+	uint8_t		nks;			/* 0x0027 */
+	uint32_t	reserved28;		/* 0x0028 */
+	uint32_t	sea;			/* 0x002c */
+	uint64_t	plaint_att_flags;	/* 0x0030 */
+	uint32_t	meas_alg_id;		/* 0x0038 */
+	uint32_t	reserved3c;		/* 0x003c */
+	uint8_t		cpk[160];		/* 0x0040 */
+	uint8_t		key_slot[80];		/* 0x00e0 */
+	uint8_t		meas_key[64];		/* 0x0130 */
+	uint8_t		tag[16];		/* 0x0170 */
+} __attribute__((packed));
+
+static void test_attest_v1(u64 supported_paf)
+{
+	struct uv_cb_attest uvcb = {
+		.header.cmd = UVC_CMD_ATTESTATION,
+		.header.len = sizeof(uvcb),
+	};
+	struct uv_arcb_v1 *arcb = (void *)page;
+	uint64_t measurement = page + sizeof(*arcb);
+	size_t measurement_size = 64;
+	uint64_t additional = measurement + measurement_size;
+	size_t additional_size = 32;
+	uint64_t plaint_att_flags = 1ULL << 61;
+	int cc;
+
+	memset((void *) page, 0, PAGE_SIZE);
+
+	/* create a minimal arcb/uvcb such that FW has everything to start unsealing the request. */
+	arcb->req_ver = ARCB_VERSION_1;
+	arcb->req_len = sizeof(*arcb);
+	arcb->nks = 1;
+	arcb->sea = sizeof(arcb->meas_key);
+	arcb->plaint_att_flags = plaint_att_flags;
+	arcb->meas_alg_id = ARCB_HMAC_SHA512;
+	uvcb.arcb_addr = page;
+	uvcb.measurement_address = measurement;
+	uvcb.measurement_length = measurement_size;
+	uvcb.add_data_address = additional;
+	uvcb.add_data_length = additional_size;
+
+	uvcb.continuation_token = 0xff;
+	cc = uv_call(0, (u64)&uvcb);
+	report(cc == 1 && uvcb.header.rc == 0x0101, "invalid continuation token");
+	uvcb.continuation_token = 0;
+
+	uvcb.user_data_length = sizeof(uvcb.user_data) + 1;
+	cc = uv_call(0, (u64)&uvcb);
+	report(cc == 1 && uvcb.header.rc == 0x0102, "invalid user data size");
+	uvcb.user_data_length = 0;
+
+	uvcb.arcb_addr = 0;
+	cc = uv_call(0, (u64)&uvcb);
+	report(cc == 1 && uvcb.header.rc == 0x0103, "invalid address arcb");
+	uvcb.arcb_addr = page;
+
+	/* 0104 - 0105 need an unseal-able request */
+
+	/* version 0000 is an illegal version number */
+	arcb->req_ver = 0x0000;
+	cc = uv_call(0, (u64)&uvcb);
+	report(cc == 1 && uvcb.header.rc == 0x0106, "unsupported version");
+	arcb->req_ver = ARCB_VERSION_1;
+
+	arcb->req_len += 1;
+	cc = uv_call(0, (u64)&uvcb);
+	report(cc == 1 && uvcb.header.rc == 0x0107, "invalid arcb size 1");
+	arcb->req_len -= 1;
+	arcb->nks = 2;
+	cc = uv_call(0, (u64)&uvcb);
+	report(cc == 1 && uvcb.header.rc == 0x0107, "invalid arcb size 2");
+	arcb->nks = 1;
+
+	arcb->nks = 0;
+	cc = uv_call(0, (u64)&uvcb);
+	report(cc == 1 && uvcb.header.rc == 0x0108, "invalid num key slots");
+	arcb->nks = 1;
+
+	/* possible valid size (when using nonce). However, req_len too small to host a nonce */
+	arcb->sea = 80;
+	cc = uv_call(0, (u64)&uvcb);
+	report(cc == 1 && uvcb.header.rc == 0x0109, "invalid encrypted size 1");
+	arcb->sea = 17;
+	cc = uv_call(0, (u64)&uvcb);
+	report(cc == 1 && uvcb.header.rc == 0x0109, "invalid encrypted size 2");
+	arcb->sea = 64;
+
+	arcb->plaint_att_flags = supported_paf ^ ((u64) -1);
+	cc = uv_call(0, (u64)&uvcb);
+	report(cc == 1 && uvcb.header.rc == 0x010a, "invalid flag");
+	arcb->plaint_att_flags = plaint_att_flags;
+
+	/* reserved value */
+	arcb->meas_alg_id = 0;
+	cc = uv_call(0, (u64)&uvcb);
+	report(cc == 1 && uvcb.header.rc == 0x010b, "invalid measurement algorithm");
+	arcb->meas_alg_id = ARCB_HMAC_SHA512;
+
+	cc = uv_call(0, (u64)&uvcb);
+	report(cc == 1 && uvcb.header.rc == 0x010c, "unable unseal");
 
+	uvcb.measurement_length = 0;
+	cc = uv_call(0, (u64)&uvcb);
+	report(cc == 1 && uvcb.header.rc == 0x010d, "invalid measurement size");
+	uvcb.measurement_length = measurement_size;
+
+	uvcb.add_data_length = 0;
+	cc = uv_call(0, (u64)&uvcb);
+	report(cc == 1 && uvcb.header.rc == 0x010e, "invalid additional size");
+	uvcb.add_data_length = additional_size;
+}
+
+static void test_attest(void)
+{
+	struct uv_cb_attest uvcb = {
+		.header.cmd = UVC_CMD_ATTESTATION,
+		.header.len = sizeof(uvcb),
+	};
+	const struct uv_cb_qui *uvcb_qui = uv_get_query_data();
+	int cc;
+
+	report_prefix_push("attest");
+
+	if (!uv_query_test_call(BIT_UVC_CMD_ATTESTATION)) {
+		report_skip("Attestation not supported.");
+		goto done;
+	}
+
+	/* Verify that the uv supports at least one header version */
+	report(uvcb_qui->supported_att_hdr_versions, "has hdr support");
+
+	memset((void *) page, 0, PAGE_SIZE);
+
+	uvcb.header.len -= 1;
+	cc = uv_call(0, (u64)&uvcb);
+	report(cc && uvcb.header.rc == UVC_RC_INV_LEN, "invalid uvcb size 1");
+	uvcb.header.len += 1;
+
+	uvcb.header.len += 1;
+	cc = uv_call(0, (u64)&uvcb);
+	report(cc && uvcb.header.rc == UVC_RC_INV_LEN, "invalid uvcb size 2");
+	uvcb.header.len -= 1;
+
+	report_prefix_push("v1");
+	if (test_bit_inv(0, &uvcb_qui->supported_att_hdr_versions))
+		test_attest_v1(uvcb_qui->supported_paf);
+	else
+		report_skip("Attestation version 1 not supported");
+	report_prefix_pop();
+done:
 	report_prefix_pop();
 }
 
@@ -179,6 +350,7 @@ int main(void)
 	test_invalid();
 	test_query();
 	test_sharing();
+	test_attest();
 	free_page((void *)page);
 done:
 	report_prefix_pop();
-- 
2.30.2

Re: [kvm-unit-tests PATCH v2 2/4] s390x: lib: Add QUI getter

[Claudio Imbrenda]

On Thu,  3 Feb 2022 09:19:33 +0000
Steffen Eiden <seiden@linux.ibm.com> wrote:

> Some tests need the information provided by the QUI UVC and lib/s390x/uv.c
> already has cached the qui result. Let's add a function to avoid
> unnecessary QUI UVCs.

I'm not against this approach, but I wonder if it's not easier to just
make the QUI buffer public?

> 
> Signed-off-by: Steffen Eiden <seiden@linux.ibm.com>
> ---
>  lib/s390x/uv.c | 8 ++++++++
>  lib/s390x/uv.h | 1 +
>  2 files changed, 9 insertions(+)
> 
> diff --git a/lib/s390x/uv.c b/lib/s390x/uv.c
> index 6fe11dff..602cbbfc 100644
> --- a/lib/s390x/uv.c
> +++ b/lib/s390x/uv.c
> @@ -47,6 +47,14 @@ bool uv_query_test_call(unsigned int nr)
>  	return test_bit_inv(nr, uvcb_qui.inst_calls_list);
>  }
>  
> +const struct uv_cb_qui *uv_get_query_data(void)
> +{
> +	/* Query needs to be called first */
> +	assert(uvcb_qui.header.rc);
> +
> +	return &uvcb_qui;
> +}
> +
>  int uv_setup(void)
>  {
>  	if (!test_facility(158))
> diff --git a/lib/s390x/uv.h b/lib/s390x/uv.h
> index 8175d9c6..44264861 100644
> --- a/lib/s390x/uv.h
> +++ b/lib/s390x/uv.h
> @@ -8,6 +8,7 @@
>  bool uv_os_is_guest(void);
>  bool uv_os_is_host(void);
>  bool uv_query_test_call(unsigned int nr);
> +const struct uv_cb_qui *uv_get_query_data(void);
>  void uv_init(void);
>  int uv_setup(void);
>  void uv_create_guest(struct vm *vm);

Re: [kvm-unit-tests PATCH v2 3/4] s390x: uv-guest: remove duplicated checks

[Claudio Imbrenda]

On Thu,  3 Feb 2022 09:19:34 +0000
Steffen Eiden <seiden@linux.ibm.com> wrote:

> Removing some tests which are done at other points in the code
> implicitly.

I'm not sure I like all of this

> 
> In lib/s390x/uc.c#setup_uv(void) the rc of the qui result is verified
> using asserts.
> The whole test is fenced by lib/s390x/uc.c#os_is_guest(void) that

do you mean "lib/s390x/uv.c#uv_os_is_guest(void)" ?

> checks if SET and REMOVE SHARED is present.
> 
> Signed-off-by: Steffen Eiden <seiden@linux.ibm.com>
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  s390x/uv-guest.c | 22 +++++++---------------
>  1 file changed, 7 insertions(+), 15 deletions(-)
> 
> diff --git a/s390x/uv-guest.c b/s390x/uv-guest.c
> index 44ad2154..97ae4687 100644
> --- a/s390x/uv-guest.c
> +++ b/s390x/uv-guest.c
> @@ -69,23 +69,15 @@ static void test_query(void)
>  	cc = uv_call(0, (u64)&uvcb);
>  	report(cc == 1 && uvcb.header.rc == UVC_RC_INV_LEN, "length");
>  
> -	uvcb.header.len = sizeof(uvcb);
> -	cc = uv_call(0, (u64)&uvcb);
> -	report((!cc && uvcb.header.rc == UVC_RC_EXECUTED) ||
> -	       (cc == 1 && uvcb.header.rc == 0x100),
> -		"successful query");
> -

ok fair enough, an unsuccessful query would have caused an assert in
the setup code, but I don't think it hurts, and I think it would be
nice to have for completeness.

>  	/*
> -	 * These bits have been introduced with the very first
> -	 * Ultravisor version and are expected to always be available
> -	 * because they are basic building blocks.
> +	 * BIT_UVC_CMD_QUI, BIT_UVC_CMD_SET_SHARED_ACCESS and
> +	 * BIT_UVC_CMD_SET_SHARED_ACCESS are always present as they

I think you meant BIT_UVC_CMD_REMOVE_SHARED_ACCESS here ?

> +	 * have been introduced with the first Ultravisor version.
> +	 * However, we only need to check for QUI as
> +	 * SET/REMOVE SHARED are used to fence this test to be only
> +	 * executed by protected guests.

also, what happens if only one of the two bits is set? (which is very
wrong). In that scenario, I would like this test to fail, not skip.
this means that we can't rely on uv_os_is_guest to decide whether to
skip this test.

>  	 */
> -	report(test_bit_inv(BIT_UVC_CMD_QUI, &uvcb.inst_calls_list[0]),
> -	       "query indicated");
> -	report(test_bit_inv(BIT_UVC_CMD_SET_SHARED_ACCESS, &uvcb.inst_calls_list[0]),
> -	       "share indicated");
> -	report(test_bit_inv(BIT_UVC_CMD_REMOVE_SHARED_ACCESS, &uvcb.inst_calls_list[0]),
> -	       "unshare indicated");
> +	report(uv_query_test_call(BIT_UVC_CMD_QUI), "query indicated");
>  	report_prefix_pop();
>  }
>  

Re: [kvm-unit-tests PATCH v2 1/4] s390x: uv-host: Add attestation test

[Claudio Imbrenda]

On Thu,  3 Feb 2022 09:19:32 +0000
Steffen Eiden <seiden@linux.ibm.com> wrote:

> Adds an invalid command test for attestation in the uv-host.
> 
> Signed-off-by: Steffen Eiden <seiden@linux.ibm.com>
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>
> ---
>  lib/s390x/asm/uv.h | 23 ++++++++++++++++++++++-
>  s390x/uv-host.c    |  1 +
>  2 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
> index 97c90e81..7afbcffd 100644
> --- a/lib/s390x/asm/uv.h
> +++ b/lib/s390x/asm/uv.h
> @@ -1,7 +1,7 @@
>  /*
>   * s390x Ultravisor related definitions
>   *
> - * Copyright (c) 2020 IBM Corp
> + * Copyright IBM Corp. 2020, 2022
>   *
>   * Authors:
>   *  Janosch Frank <frankja@linux.ibm.com>
> @@ -47,6 +47,7 @@
>  #define UVC_CMD_UNPIN_PAGE_SHARED	0x0342
>  #define UVC_CMD_SET_SHARED_ACCESS	0x1000
>  #define UVC_CMD_REMOVE_SHARED_ACCESS	0x1001
> +#define UVC_CMD_ATTESTATION		0x1020
>  
>  /* Bits in installed uv calls */
>  enum uv_cmds_inst {
> @@ -71,6 +72,7 @@ enum uv_cmds_inst {
>  	BIT_UVC_CMD_UNSHARE_ALL = 20,
>  	BIT_UVC_CMD_PIN_PAGE_SHARED = 21,
>  	BIT_UVC_CMD_UNPIN_PAGE_SHARED = 22,
> +	BIT_UVC_CMD_ATTESTATION = 28,
>  };
>  
>  struct uv_cb_header {
> @@ -178,6 +180,25 @@ struct uv_cb_cfs {
>  	u64 paddr;
>  }  __attribute__((packed))  __attribute__((aligned(8)));
>  
> +/* Retrieve Attestation Measurement */
> +struct uv_cb_attest {
> +	struct uv_cb_header header;	/* 0x0000 */
> +	u64 reserved08[2];		/* 0x0008 */
> +	u64 arcb_addr;			/* 0x0018 */
> +	u64 continuation_token;		/* 0x0020 */
> +	u8  reserved28[6];		/* 0x0028 */
> +	u16 user_data_length;		/* 0x002e */
> +	u8  user_data[256];		/* 0x0030 */
> +	u32 reserved130[3];		/* 0x0130 */
> +	u32 measurement_length;		/* 0x013c */
> +	u64 measurement_address;	/* 0x0140 */
> +	u8 config_uid[16];		/* 0x0148 */
> +	u32 reserved158;		/* 0x0158 */
> +	u32 add_data_length;		/* 0x015c */
> +	u64 add_data_address;		/* 0x0160 */
> +	u64 reserved168[4];		/* 0x0168 */

please use uint*_t types!

with that fixed: 

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> +}  __attribute__((packed))  __attribute__((aligned(8)));
> +
>  /* Set Secure Config Parameter */
>  struct uv_cb_ssc {
>  	struct uv_cb_header header;
> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
> index 92a41069..946f031e 100644
> --- a/s390x/uv-host.c
> +++ b/s390x/uv-host.c
> @@ -418,6 +418,7 @@ static struct cmd_list invalid_cmds[] = {
>  	{ "bogus", 0x4242, sizeof(struct uv_cb_header), -1},
>  	{ "share", UVC_CMD_SET_SHARED_ACCESS, sizeof(struct uv_cb_share), BIT_UVC_CMD_SET_SHARED_ACCESS },
>  	{ "unshare", UVC_CMD_REMOVE_SHARED_ACCESS, sizeof(struct uv_cb_share), BIT_UVC_CMD_REMOVE_SHARED_ACCESS },
> +	{ "attest", UVC_CMD_ATTESTATION, sizeof(struct uv_cb_attest), BIT_UVC_CMD_ATTESTATION },
>  	{ NULL, 0, 0 },
>  };
>  

Re: [kvm-unit-tests PATCH v2 4/4] s390x: uv-guest: Add attestation tests

[Claudio Imbrenda]

On Thu,  3 Feb 2022 09:19:35 +0000
Steffen Eiden <seiden@linux.ibm.com> wrote:

> Adds several tests to verify correct error paths of attestation.
> 
> Signed-off-by: Steffen Eiden <seiden@linux.ibm.com>
> ---
>  lib/s390x/asm/uv.h |   5 +-
>  s390x/uv-guest.c   | 174 ++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 177 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
> index 7afbcffd..7fe55052 100644
> --- a/lib/s390x/asm/uv.h
> +++ b/lib/s390x/asm/uv.h
> @@ -108,7 +108,10 @@ struct uv_cb_qui {
>  	u8  reserved88[158 - 136];	/* 0x0088 */
>  	uint16_t max_guest_cpus;	/* 0x009e */
>  	u64 uv_feature_indications;	/* 0x00a0 */
> -	u8  reserveda8[200 - 168];	/* 0x00a8 */
> +	u8  reserveda8[224 - 168];	/* 0x00a8 */

please use uint*_t types everywhere. as you notice, we are already
inconsistent, so let's just use the new ones for all new code

> +	u64 supported_att_hdr_versions;	/* 0x00e0 */
> +	u64 supported_paf;		/* 0x00e8 */
> +	u8  reservedf0[256 - 240];	/* 0x00f0 */
>  }  __attribute__((packed))  __attribute__((aligned(8)));
>  
>  struct uv_cb_cgc {
> diff --git a/s390x/uv-guest.c b/s390x/uv-guest.c
> index 97ae4687..3fca9d21 100644
> --- a/s390x/uv-guest.c
> +++ b/s390x/uv-guest.c
> @@ -2,10 +2,11 @@
>  /*
>   * Guest Ultravisor Call tests
>   *
> - * Copyright (c) 2020 IBM Corp
> + * Copyright IBM Corp. 2020, 2022
>   *
>   * Authors:
>   *  Janosch Frank <frankja@linux.ibm.com>
> + *  Steffen Eiden <seiden@linux.ibm.com>
>   */
>  
>  #include <libcflat.h>
> @@ -53,6 +54,15 @@ static void test_priv(void)
>  	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
>  	report_prefix_pop();
>  
> +	report_prefix_push("attest");
> +	uvcb.cmd = UVC_CMD_ATTESTATION;
> +	uvcb.len = sizeof(struct uv_cb_attest);
> +	expect_pgm_int();
> +	enter_pstate();
> +	uv_call_once(0, (u64)&uvcb);

please use uint*_t types :)

> +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
> +	report_prefix_pop();
> +
>  	report_prefix_pop();
>  }
>  
> @@ -111,7 +121,168 @@ static void test_sharing(void)
>  	cc = uv_call(0, (u64)&uvcb);
>  	report(cc == 0 && uvcb.header.rc == UVC_RC_EXECUTED, "unshare");
>  	report_prefix_pop();
> +}
> +
> +#define ARCB_VERSION_1 0x0100
> +#define ARCB_HMAC_SHA512 1
> +/* arcb with one key slot and no nonce */
> +struct uv_arcb_v1 {
> +	uint64_t	reserved0;		/* 0x0000 */
> +	uint32_t	req_ver;		/* 0x0008 */
> +	uint32_t	req_len;		/* 0x000c */
> +	uint8_t		iv[12];			/*
> 0x0010 */
> +	uint32_t	reserved1c;		/* 0x001c */
> +	uint8_t		reserved20[7];		/*
> 0x0020 */
> +	uint8_t		nks;			/* 0x0027
> */
> +	uint32_t	reserved28;		/* 0x0028 */
> +	uint32_t	sea;			/* 0x002c */
> +	uint64_t	plaint_att_flags;	/* 0x0030 */
> +	uint32_t	meas_alg_id;		/* 0x0038 */
> +	uint32_t	reserved3c;		/* 0x003c */
> +	uint8_t		cpk[160];		/* 0x0040 */
> +	uint8_t		key_slot[80];		/*
> 0x00e0 */
> +	uint8_t		meas_key[64];		/*
> 0x0130 */
> +	uint8_t		tag[16];		/* 0x0170 */
> +} __attribute__((packed));
> +
> +static void test_attest_v1(u64 supported_paf)
> +{
> +	struct uv_cb_attest uvcb = {
> +		.header.cmd = UVC_CMD_ATTESTATION,
> +		.header.len = sizeof(uvcb),
> +	};
> +	struct uv_arcb_v1 *arcb = (void *)page;
> +	uint64_t measurement = page + sizeof(*arcb);
> +	size_t measurement_size = 64;
> +	uint64_t additional = measurement + measurement_size;
> +	size_t additional_size = 32;

I wonder it it would be easier to create a struct with an embedded
struct uv_arcb_v1 to represent the measurement and the additional data.
that way you won't need to do all these hacky calculations and magic
numbers, you could just do something like 

measurement = (uint64_t)&arcb_extended.measurement;

which, while longer, is probably easier to understand.

and the sizes maybe could become #defines

> +	uint64_t plaint_att_flags = 1ULL << 61;
> +	int cc;
> +
> +	memset((void *) page, 0, PAGE_SIZE);

please no extra space between the ) and the p

> +
> +	/* create a minimal arcb/uvcb such that FW has everything to start unsealing the request. */
> +	arcb->req_ver = ARCB_VERSION_1;
> +	arcb->req_len = sizeof(*arcb);
> +	arcb->nks = 1;
> +	arcb->sea = sizeof(arcb->meas_key);
> +	arcb->plaint_att_flags = plaint_att_flags;
> +	arcb->meas_alg_id = ARCB_HMAC_SHA512;
> +	uvcb.arcb_addr = page;
> +	uvcb.measurement_address = measurement;
> +	uvcb.measurement_length = measurement_size;
> +	uvcb.add_data_address = additional;
> +	uvcb.add_data_length = additional_size;

are you using those variables somewhere else? could you just assign
directly to the uvcb instead?

> +
> +	uvcb.continuation_token = 0xff;
> +	cc = uv_call(0, (u64)&uvcb);

please use uint*_t types everywhere

> +	report(cc == 1 && uvcb.header.rc == 0x0101, "invalid continuation token");
> +	uvcb.continuation_token = 0;
> +
> +	uvcb.user_data_length = sizeof(uvcb.user_data) + 1;
> +	cc = uv_call(0, (u64)&uvcb);
> +	report(cc == 1 && uvcb.header.rc == 0x0102, "invalid user data size");
> +	uvcb.user_data_length = 0;
> +
> +	uvcb.arcb_addr = 0;

is 0 really not a valid address?

and what about an address outside memory?

> +	cc = uv_call(0, (u64)&uvcb);
> +	report(cc == 1 && uvcb.header.rc == 0x0103, "invalid address arcb");
> +	uvcb.arcb_addr = page;
> +
> +	/* 0104 - 0105 need an unseal-able request */
> +
> +	/* version 0000 is an illegal version number */
> +	arcb->req_ver = 0x0000;

just 0 is ok

> +	cc = uv_call(0, (u64)&uvcb);
> +	report(cc == 1 && uvcb.header.rc == 0x0106, "unsupported version");
> +	arcb->req_ver = ARCB_VERSION_1;
> +
> +	arcb->req_len += 1;
> +	cc = uv_call(0, (u64)&uvcb);
> +	report(cc == 1 && uvcb.header.rc == 0x0107, "invalid arcb size 1");
> +	arcb->req_len -= 1;
> +	arcb->nks = 2;
> +	cc = uv_call(0, (u64)&uvcb);
> +	report(cc == 1 && uvcb.header.rc == 0x0107, "invalid arcb size 2");

you say invalid arcb size, but you are changing the number of nks. I
think I understand why, but maybe it's better to add a comment to
explain what you are doing.

> +	arcb->nks = 1;
> +
> +	arcb->nks = 0;
> +	cc = uv_call(0, (u64)&uvcb);
> +	report(cc == 1 && uvcb.header.rc == 0x0108, "invalid num key slots");
> +	arcb->nks = 1;
> +
> +	/* possible valid size (when using nonce). However, req_len too small to host a nonce */
> +	arcb->sea = 80;
> +	cc = uv_call(0, (u64)&uvcb);
> +	report(cc == 1 && uvcb.header.rc == 0x0109, "invalid encrypted size 1");
> +	arcb->sea = 17;
> +	cc = uv_call(0, (u64)&uvcb);
> +	report(cc == 1 && uvcb.header.rc == 0x0109, "invalid encrypted size 2");
> +	arcb->sea = 64;
> +
> +	arcb->plaint_att_flags = supported_paf ^ ((u64) -1);

so you are trying to flip the lower 32 bits?
why not just supported_paf ^ ~0 ?

although a more readable form would probably be
supported_paf ^ GENMASK_ULL(31, 0)

> +	cc = uv_call(0, (u64)&uvcb);
> +	report(cc == 1 && uvcb.header.rc == 0x010a, "invalid flag");
> +	arcb->plaint_att_flags = plaint_att_flags;
> +
> +	/* reserved value */
> +	arcb->meas_alg_id = 0;
> +	cc = uv_call(0, (u64)&uvcb);
> +	report(cc == 1 && uvcb.header.rc == 0x010b, "invalid measurement algorithm");
> +	arcb->meas_alg_id = ARCB_HMAC_SHA512;
> +
> +	cc = uv_call(0, (u64)&uvcb);
> +	report(cc == 1 && uvcb.header.rc == 0x010c, "unable unseal");
>  
> +	uvcb.measurement_length = 0;
> +	cc = uv_call(0, (u64)&uvcb);
> +	report(cc == 1 && uvcb.header.rc == 0x010d, "invalid measurement size");
> +	uvcb.measurement_length = measurement_size;
> +
> +	uvcb.add_data_length = 0;
> +	cc = uv_call(0, (u64)&uvcb);
> +	report(cc == 1 && uvcb.header.rc == 0x010e, "invalid additional size");
> +	uvcb.add_data_length = additional_size;
> +}
> +
> +static void test_attest(void)
> +{
> +	struct uv_cb_attest uvcb = {
> +		.header.cmd = UVC_CMD_ATTESTATION,
> +		.header.len = sizeof(uvcb),
> +	};
> +	const struct uv_cb_qui *uvcb_qui = uv_get_query_data();
> +	int cc;
> +
> +	report_prefix_push("attest");
> +
> +	if (!uv_query_test_call(BIT_UVC_CMD_ATTESTATION)) {
> +		report_skip("Attestation not supported.");
> +		goto done;
> +	}
> +
> +	/* Verify that the uv supports at least one header version */
> +	report(uvcb_qui->supported_att_hdr_versions, "has hdr support");
> +
> +	memset((void *) page, 0, PAGE_SIZE);

please no extra space between ) and p

> +
> +	uvcb.header.len -= 1;
> +	cc = uv_call(0, (u64)&uvcb);
> +	report(cc && uvcb.header.rc == UVC_RC_INV_LEN, "invalid uvcb size 1");
> +	uvcb.header.len += 1;
> +
> +	uvcb.header.len += 1;
> +	cc = uv_call(0, (u64)&uvcb);
> +	report(cc && uvcb.header.rc == UVC_RC_INV_LEN, "invalid uvcb size 2");
> +	uvcb.header.len -= 1;
> +
> +	report_prefix_push("v1");
> +	if (test_bit_inv(0, &uvcb_qui->supported_att_hdr_versions))
> +		test_attest_v1(uvcb_qui->supported_paf);
> +	else
> +		report_skip("Attestation version 1 not supported");
> +	report_prefix_pop();
> +done:
>  	report_prefix_pop();
>  }
>  
> @@ -179,6 +350,7 @@ int main(void)
>  	test_invalid();
>  	test_query();
>  	test_sharing();
> +	test_attest();
>  	free_page((void *)page);
>  done:
>  	report_prefix_pop();

Re: [kvm-unit-tests PATCH v2 2/4] s390x: lib: Add QUI getter

[Janosch Frank]

On 2/3/22 17:12, Claudio Imbrenda wrote:
> On Thu,  3 Feb 2022 09:19:33 +0000
> Steffen Eiden <seiden@linux.ibm.com> wrote:
> 
>> Some tests need the information provided by the QUI UVC and lib/s390x/uv.c
>> already has cached the qui result. Let's add a function to avoid
>> unnecessary QUI UVCs.
> 
> I'm not against this approach, but I wonder if it's not easier to just
> make the QUI buffer public?

The introduction of that function is on me and I'm not a big fan of 
global variables.


> 
>>
>> Signed-off-by: Steffen Eiden <seiden@linux.ibm.com>
>> ---
>>   lib/s390x/uv.c | 8 ++++++++
>>   lib/s390x/uv.h | 1 +
>>   2 files changed, 9 insertions(+)
>>
>> diff --git a/lib/s390x/uv.c b/lib/s390x/uv.c
>> index 6fe11dff..602cbbfc 100644
>> --- a/lib/s390x/uv.c
>> +++ b/lib/s390x/uv.c
>> @@ -47,6 +47,14 @@ bool uv_query_test_call(unsigned int nr)
>>   	return test_bit_inv(nr, uvcb_qui.inst_calls_list);
>>   }
>>   
>> +const struct uv_cb_qui *uv_get_query_data(void)
>> +{
>> +	/* Query needs to be called first */
>> +	assert(uvcb_qui.header.rc);
>> +
>> +	return &uvcb_qui;
>> +}
>> +
>>   int uv_setup(void)
>>   {
>>   	if (!test_facility(158))
>> diff --git a/lib/s390x/uv.h b/lib/s390x/uv.h
>> index 8175d9c6..44264861 100644
>> --- a/lib/s390x/uv.h
>> +++ b/lib/s390x/uv.h
>> @@ -8,6 +8,7 @@
>>   bool uv_os_is_guest(void);
>>   bool uv_os_is_host(void);
>>   bool uv_query_test_call(unsigned int nr);
>> +const struct uv_cb_qui *uv_get_query_data(void);
>>   void uv_init(void);
>>   int uv_setup(void);
>>   void uv_create_guest(struct vm *vm);
> 

Re: [kvm-unit-tests PATCH v2 3/4] s390x: uv-guest: remove duplicated checks

[Steffen Eiden]

On 2/3/22 17:31, Claudio Imbrenda wrote:
> On Thu,  3 Feb 2022 09:19:34 +0000
> Steffen Eiden <seiden@linux.ibm.com> wrote:
> 
>> Removing some tests which are done at other points in the code
>> implicitly.

[...]
>>
>> diff --git a/s390x/uv-guest.c b/s390x/uv-guest.c
>> index 44ad2154..97ae4687 100644
>> --- a/s390x/uv-guest.c
>> +++ b/s390x/uv-guest.c
>> @@ -69,23 +69,15 @@ static void test_query(void)
>>   	cc = uv_call(0, (u64)&uvcb);
>>   	report(cc == 1 && uvcb.header.rc == UVC_RC_INV_LEN, "length");
>>   
>> -	uvcb.header.len = sizeof(uvcb);
>> -	cc = uv_call(0, (u64)&uvcb);
>> -	report((!cc && uvcb.header.rc == UVC_RC_EXECUTED) ||
>> -	       (cc == 1 && uvcb.header.rc == 0x100),
>> -		"successful query");
>> -
> 
> ok fair enough, an unsuccessful query would have caused an assert in
> the setup code, but I don't think it hurts, and I think it would be
> nice to have for completeness.
>
Janosch explicitly asked me to remove this while I am editing uv_guest.

[...]

> 
> also, what happens if only one of the two bits is set? (which is very
> wrong). In that scenario, I would like this test to fail, not skip.
> this means that we can't rely on uv_os_is_guest to decide whether to
> skip this test.
>
That is true and a test if both bits are present xor none would be a 
great addition. However, if just one bit is set, uv_os_is_guest would
return false and this part will never be reached anyway.

I can add a test before the uv_os_is_guest fence to verify that both
xor none SHARED flags are set.

>>   	 */
>> -	report(test_bit_inv(BIT_UVC_CMD_QUI, &uvcb.inst_calls_list[0]),
>> -	       "query indicated");
>> -	report(test_bit_inv(BIT_UVC_CMD_SET_SHARED_ACCESS, &uvcb.inst_calls_list[0]),
>> -	       "share indicated");
>> -	report(test_bit_inv(BIT_UVC_CMD_REMOVE_SHARED_ACCESS, &uvcb.inst_calls_list[0]),
>> -	       "unshare indicated");
>> +	report(uv_query_test_call(BIT_UVC_CMD_QUI), "query indicated");
>>   	report_prefix_pop();
>>   }
>>   
> 

Steffen