Re: [PATCH] target/i386/sev: add the support to query the attestation report

[Brijesh Singh <brijesh.singh@amd.com>]

On 12/10/20 10:13 AM, James Bottomley wrote:
> On Fri, 2020-12-04 at 15:31 -0600, Brijesh Singh wrote:
>> The SEV FW >= 0.23 added a new command that can be used to query the
>> attestation report containing the SHA-256 digest of the guest memory
>> and VMSA encrypted with the LAUNCH_UPDATE and sign it with the PEK.
>>
>> Note, we already have a command (LAUNCH_MEASURE) that can be used to
>> query the SHA-256 digest of the guest memory encrypted through the
>> LAUNCH_UPDATE. The main difference between previous and this command
>> is that the report is signed with the PEK and unlike the
>> LAUNCH_MEASURE
>> command the ATTESATION_REPORT command can be called while the guest
>> is running.
>>
>> Add a QMP interface "query-sev-attestation-report" that can be used
>> to get the report encoded in base64.
>>
>> Cc: James Bottomley <jejb@linux.ibm.com>
>> Cc: Tom Lendacky <Thomas.Lendacky@amd.com>
>> Cc: Eric Blake <eblake@redhat.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: kvm@vger.kernel.org
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>  linux-headers/linux/kvm.h |  8 ++++++
>>  qapi/misc-target.json     | 38 +++++++++++++++++++++++++++
>>  target/i386/monitor.c     |  6 +++++
>>  target/i386/sev-stub.c    |  7 +++++
>>  target/i386/sev.c         | 54
>> +++++++++++++++++++++++++++++++++++++++
>>  target/i386/sev_i386.h    |  2 ++
>>  6 files changed, 115 insertions(+)
>>
>> diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
>> index 56ce14ad20..6d0f8101ba 100644
>> --- a/linux-headers/linux/kvm.h
>> +++ b/linux-headers/linux/kvm.h
>> @@ -1585,6 +1585,8 @@ enum sev_cmd_id {
>>  	KVM_SEV_DBG_ENCRYPT,
>>  	/* Guest certificates commands */
>>  	KVM_SEV_CERT_EXPORT,
>> +	/* Attestation report */
>> +	KVM_SEV_GET_ATTESTATION_REPORT,
>>  
>>  	KVM_SEV_NR_MAX,
>>  };
>> @@ -1637,6 +1639,12 @@ struct kvm_sev_dbg {
>>  	__u32 len;
>>  };
>>  
>> +struct kvm_sev_attestation_report {
>> +	__u8 mnonce[16];
>> +	__u64 uaddr;
>> +	__u32 len;
>> +};
>> +
>>  #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)
>>  #define KVM_DEV_ASSIGN_PCI_2_3		(1 << 1)
>>  #define KVM_DEV_ASSIGN_MASK_INTX	(1 << 2)
>> diff --git a/qapi/misc-target.json b/qapi/misc-target.json
>> index 1e561fa97b..ec6565e6ef 100644
>> --- a/qapi/misc-target.json
>> +++ b/qapi/misc-target.json
>> @@ -267,3 +267,41 @@
>>  ##
>>  { 'command': 'query-gic-capabilities', 'returns': ['GICCapability'],
>>    'if': 'defined(TARGET_ARM)' }
>> +
>> +
>> +##
>> +# @SevAttestationReport:
>> +#
>> +# The struct describes attestation report for a Secure Encrypted
>> Virtualization
>> +# feature.
>> +#
>> +# @data:  guest attestation report (base64 encoded)
>> +#
>> +#
>> +# Since: 5.2
>> +##
>> +{ 'struct': 'SevAttestationReport',
>> +  'data': { 'data': 'str'},
>> +  'if': 'defined(TARGET_I386)' }
>> +
>> +##
>> +# @query-sev-attestation-report:
>> +#
>> +# This command is used to get the SEV attestation report, and is
>> supported on AMD
>> +# X86 platforms only.
>> +#
>> +# @mnonce: a random 16 bytes of data (it will be included in report)
>> +#
>> +# Returns: SevAttestationReport objects.
>> +#
>> +# Since: 5.2
>> +#
>> +# Example:
>> +#
>> +# -> { "execute" : "query-sev-attestation-report", "arguments": {
>> "mnonce": "aaaaaaa" } }
>> +# <- { "return" : { "data": "aaaaaaaabbbddddd"} }
> It would be nice here, rather than returning a binary blob to break it
> up into the actual returned components like query-sev does.

In past, I have seen that the fields defined in blobs have changed based
on the API versions. So, I tried to stay away from expanding the blob
unless its absolutely required. I would  prefer to stick to that approach.


>
>> +##
>> +{ 'command': 'query-sev-attestation-report', 'data': { 'mnonce':
>> 'str' },
>> +  'returns': 'SevAttestationReport',
>> +  'if': 'defined(TARGET_I386)' }
> [...]
>> diff --git a/target/i386/sev.c b/target/i386/sev.c
>> index 93c4d60b82..28958fb71b 100644
>> --- a/target/i386/sev.c
>> +++ b/target/i386/sev.c
>> @@ -68,6 +68,7 @@ struct SevGuestState {
>>  
>>  #define DEFAULT_GUEST_POLICY    0x1 /* disable debug */
>>  #define DEFAULT_SEV_DEVICE      "/dev/sev"
>> +#define DEFAULT_ATTESATION_REPORT_BUF_SIZE      4096
>>  
>>  static SevGuestState *sev_guest;
>>  static Error *sev_mig_blocker;
>> @@ -490,6 +491,59 @@ out:
>>      return cap;
>>  }
>>  
>> +SevAttestationReport *
>> +sev_get_attestation_report(const char *mnonce, Error **errp)
>> +{
>> +    struct kvm_sev_attestation_report input = {};
>> +    SevGuestState *sev = sev_guest;
>> +    SevAttestationReport *report;
>> +    guchar *data;
>> +    int err = 0, ret;
>> +
>> +    if (!sev_enabled()) {
>> +        error_setg(errp, "SEV is not enabled");
>> +        return NULL;
>> +    }
>> +
>> +    /* Verify that user provided random data length */
> There should be a g_base64_decode here, shouldn't there, so we can pass
> an arbitrary 16 byte binary blob.


Agreed, I will make this field base64 in v2.


>
>> +    if (strlen(mnonce) != sizeof(input.mnonce)) {
> So this if would check the decoded length.
>
>> +        error_setg(errp, "Expected mnonce data len %ld got %ld",
>> +                sizeof(input.mnonce), strlen(mnonce));
>> +        return NULL;
>> +    }
>> +
>> +    /* Query the report length */
>> +    ret = sev_ioctl(sev->sev_fd, KVM_SEV_GET_ATTESTATION_REPORT,
>> +            &input, &err);
>> +    if (ret < 0) {
>> +        if (err != SEV_RET_INVALID_LEN) {
>> +            error_setg(errp, "failed to query the attestation report
>> length "
>> +                    "ret=%d fw_err=%d (%s)", ret, err,
>> fw_error_to_str(err));
>> +            return NULL;
>> +        }
>> +    }
>> +
>> +    data = g_malloc(input.len);
>> +    input.uaddr = (unsigned long)data;
>> +    memcpy(input.mnonce, mnonce, sizeof(input.mnonce));
>> +
>> +    /* Query the report */
>> +    ret = sev_ioctl(sev->sev_fd, KVM_SEV_GET_ATTESTATION_REPORT,
>> +            &input, &err);
>> +    if (ret) {
>> +        error_setg_errno(errp, errno, "Failed to get attestation
>> report"
>> +                " ret=%d fw_err=%d (%s)", ret, err,
>> fw_error_to_str(err));
> report should be set to NULL here to avoid returning uninitialized data
> from the goto.


Noted. thanks

>
>> +        goto e_free_data;
>> +    }
>> +
>> +    report = g_new0(SevAttestationReport, 1);
>> +    report->data = g_base64_encode(data, input.len);
>> +
>> +e_free_data:
>> +    g_free(data);
>> +    return report;
>> +}
>> +
>>  static int
>>  sev_read_file_base64(const char *filename, guchar **data, gsize
>> *len)
> James
>
>