Re: [kvm-unit-tests PATCH v4 5/5] s390x: uv-guest: Add attestation tests

[Steffen Eiden <seiden@linux.ibm.com>]

Hi Janosch,

thanks for your review.

On 4/26/22 11:22, Janosch Frank wrote:
> On 4/21/22 11:45, Steffen Eiden 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/Makefile     |   1 +
>>   s390x/pv-attest.c  | 225 +++++++++++++++++++++++++++++++++++++++++++++
>>   s390x/uv-guest.c   |  13 ++-
>>   4 files changed, 240 insertions(+), 4 deletions(-)
>>   create mode 100644 s390x/pv-attest.c
>>
>> diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
>> index 7c8c399d..38920461 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 */
>> +    uint8_t  reserveda8[224 - 168];    /* 0x00a8 */
>> +    uint64_t supp_att_hdr_ver;    /* 0x00e0 */
>> +    uint64_t supp_paf;        /* 0x00e8 */
>> +    uint8_t  reservedf0[256 - 240];    /* 0x00f0 */
>>   }  __attribute__((packed))  __attribute__((aligned(8)));
>>   struct uv_cb_cgc {
>> diff --git a/s390x/Makefile b/s390x/Makefile
>> index 8ff84db5..5a49d1e7 100644
>> --- a/s390x/Makefile
>> +++ b/s390x/Makefile
>> @@ -29,6 +29,7 @@ tests += $(TEST_DIR)/mvpg-sie.elf
>>   tests += $(TEST_DIR)/spec_ex-sie.elf
>>   tests += $(TEST_DIR)/firq.elf
>>   tests += $(TEST_DIR)/epsw.elf
>> +tests += $(TEST_DIR)/pv-attest.elf
>>   pv-tests += $(TEST_DIR)/pv-diags.elf
>> diff --git a/s390x/pv-attest.c b/s390x/pv-attest.c
>> new file mode 100644
>> index 00000000..e31780a3
>> --- /dev/null
>> +++ b/s390x/pv-attest.c
>> @@ -0,0 +1,225 @@
> [...]
>> +
>> +static void test_attest_v1(uint64_t page)
>> +{
>> +    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();
>> +    struct attest_request_v1 *attest_req = (void *)page;
>> +    struct uv_arcb_v1 *arcb = &attest_req->arcb;
>> +    int cc;
>> +
>> +    report_prefix_push("v1");
>> +    if (!test_bit_inv(0, &uvcb_qui->supp_att_hdr_ver)) {
>> +        report_skip("Attestation version 1 not supported");
>> +        goto done;
>> +    }
>> +
>> +    memset((void *)page, 0, PAGE_SIZE);
>> +
>> +    /*
>> +     * Create a minimal arcb/uvcb such that FW has everything to start
>> +     * unsealing the request. However, this unsealing will fail as the
>> +     * kvm-unit-test framework provides no cryptography functions that
>> +     * would be needed to seal such requests.
>> +     */
>> +    arcb->req_ver = ARCB_VERSION_1;
>> +    arcb->req_len = sizeof(*arcb);
>> +    arcb->nks = 1;
>> +    arcb->sea = sizeof(arcb->meas_key);
>> +    arcb->plaint_att_flags = PAF_PHKH_ATT;
>> +    arcb->meas_alg_id = ARCB_MEAS_HMAC_SHA512;
>> +    uvcb.arcb_addr = (uint64_t)&attest_req->arcb;
>> +    uvcb.measurement_address = (uint64_t)attest_req->measurement;
>> +    uvcb.measurement_length = sizeof(attest_req->measurement);
>> +    uvcb.add_data_address = (uint64_t)attest_req->additional;
>> +    uvcb.add_data_length = sizeof(attest_req->additional);
>> +
>> +    uvcb.continuation_token = 0xff;
>> +    cc = uv_call(0, (uint64_t)&uvcb);
>> +    report(cc == 1 && uvcb.header.rc == 0x0101, "invalid continuation 
>> token");
> 
> Please don't add the 0 to the front of the rc values.
OK
> 
> [...]
> 
>> @@ -111,8 +120,6 @@ static void test_sharing(void)
>>       cc = uv_call(0, (u64)&uvcb);
>>       report(cc == 0 && uvcb.header.rc == UVC_RC_EXECUTED, "unshare");
>>       report_prefix_pop();
>> -
>> -    report_prefix_pop();
> 
> That's unrelated, no?
Right, it is now unrelated. I forgot to remove that change for v4.
  In the previous version this double pop would mess with the output as
`test_sharing` was not the last test in that file
(attestation test followed).
I'll add another fix patch for this.

> 
>>   }
>>   static struct {
> 

Steffen